Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: catch exceptions from SourceMapConsumer #1098

Merged
merged 1 commit into from
Apr 10, 2015

Conversation

afischer-sfly
Copy link
Contributor

The symptom of this issue is that the test runner will die with the following error:

Fatal error: Line must be greater than or equal to 1, got 0

The way the error happened:

  1. One of my specs was failing when calling getComputedStyle(). In Firefox this resulted in an error message with some weird formatting: "TypeError: Argument 1 of Window.getComputedStyle does not implement interface Element. in http://172.20.132.80:8080/base/app/SilverApp/services/DragAvatarProducer.js?dc125e476a15c0e2f62b00901b3d0c1b6c407af8 (line 85)". The problem with that string is the line number is formatted "(line 85)" when it should be like ":85:7". This might be a bug against Firefox.
  2. Karma's source-map support tried to parse the line & column, but couldn't because the formatting was weird. (see: https://github.com/karma-runner/karma/blob/master/lib/reporter.js#L36 )
  3. Karma uses defaults of 0 for the line & column numbers. (see: https://github.com/karma-runner/karma/blob/master/lib/reporter.js#L45 )
  4. The source-map library throws an exception if line or column is 0. (see: https://github.com/mozilla/source-map/blob/master/lib/source-map/source-map-consumer.js#L277 )

This pull request wraps SourceMapConsumer.originalPositionFor in a try-catch block.

@L1fescape
Copy link

I ran into a similar problem. Karma was just dying with

Fatal error: Line must be greater than or equal to 1, got 0

and I couldn't figure out why. After modifying lib/reporter.js with the code in this pull request I was able to get karma to tell me it was failing on a Maximum call stack size exceeded error from my code, which I was able to fix. After that I reset lib/reporter.js back to what it originally was and everything worked fine.

Side note: I'm using browserify, and I think with Maximum call stack size exceeded a line number isn't returned with the exception.

@jeeyoungk
Copy link

i'm running into the similar error - is there any way this is going to be merged soon?

original.line, original.column);
} catch (e) {
// ignore, fall back to non-source-mapped formatting.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can output to warn log message about failing SourceMapConsumer

@maksimr
Copy link
Contributor

maksimr commented Nov 6, 2014

Some other way to solve this problem:

  1. Set default value '1' for line number
  2. Change regexp which extract line number and column so it works only with line

@paldepind
Copy link

👍

@maksimr
Copy link
Contributor

maksimr commented Dec 21, 2014

\cc @afischer-sfly @akenn @jeeyoungk @paldepind

Guys are you use karma-jasmine?

@vojtajina
Copy link
Contributor

Thanks @afischer-sfly I agree this needs to be fixed.
I think we should correct the regular expression to parse Firefox error stacks, instead of ignoring these errors.

@mtscout6
Copy link
Member

Using this change I see the following:

➜  one-exchange git:(team-4) ✗ karma start
INFO [karma]: Karma v0.12.31 server started at http://localhost:9876/
INFO [launcher]: Starting browser Chrome
INFO [launcher]: Starting browser Firefox
INFO [launcher]: Starting browser PhantomJS
INFO [Chrome 40.0.2214 (Mac OS X 10.10.2)]: Connected on socket KvQfC30d23P_S4-w9Pwz with id 6138033
INFO [PhantomJS 1.9.7 (Mac OS X)]: Connected on socket bOGR9WbBI4Jm2kwc9Pw0 with id 9003323
INFO [Firefox 34.0.0 (Mac OS X 10.10)]: Connected on socket Ub3snpehdDscQ1HY9Pw1 with id 74944863
PhantomJS 1.9.7 (Mac OS X) Directive: addToCart toggles the text FAILED
        ReferenceError: Can't find variable: test
            at /mnt/Resources/dev/one-exchange/src/ExtendHealth.OneExchange/content/scripts/specs/test-main.js:218:0 <- webpack:///src/ExtendHealth.OneExchange/content/scripts/specs/add-to-cart-directive-spec.js:58:0
Firefox 34.0.0 (Mac OS X 10.10) Directive: addToCart toggles the text FAILED
        ReferenceError: test is not defined in /mnt/Resources/dev/one-exchange/src/ExtendHealth.OneExchange/content/scripts/specs/test-main.js (line 218)
        __WEBPACK_AMD_DEFINE_RESULT__</</<@/mnt/Resources/dev/one-exchange/src/ExtendHealth.OneExchange/content/scripts/specs/test-main.js:218:10 <- webpack:///src/ExtendHealth.OneExchange/content/scripts/specs/add-to-cart-directive-spec.js:58:0

Chrome 40.0.2214 (Mac OS X 10.10.2) Directive: addToCart toggles the text FAILED
        ReferenceError: test is not defined
            at null.<anonymous> (/mnt/Resources/dev/one-exchange/src/ExtendHealth.OneExchange/content/scripts/specs/test-main.js:218:10 <- webpack:///src/ExtendHealth.OneExchange/content/scripts/specs/add-to-cart-directive-spec.js:58:0)
PhantomJS 1.9.7 (Mac OS X): Executed 1 of 2002 (1 FAILED) (skipped 2001) ERROR (0.711 secs / 0.023 secs)
Firefox 34.0.0 (Mac OS X 10.10): Executed 1 of 2002 (1 FAILED) (skipped 2001) ERROR (2.245 secs / 0.043 secs)
Chrome 40.0.2214 (Mac OS X 10.10.2): Executed 1 of 2002 (1 FAILED) (skipped 2001) ERROR (0.66 secs / 0.059 secs)

The issue with Firefox here is that it looks like the first line contains a line number, but the stack trace hasn't actually started yet. The next line is the actual stack trace and the source map works as expected. In this case I think just logging a warning that there was an error in the SourceMapConsumer would suffice, and you shouldn't need to change the existing regex at all. Or maybe forego attempting source map line number changes on the first line of an error, but I don't know how well that will play with all the other browsers that I'm not using.

@phun-ky
Copy link

phun-ky commented Mar 25, 2015

When can we get this PR in? +1

@maksimr
Copy link
Contributor

maksimr commented Mar 25, 2015

@afischer-sfly can you rebase on master and logging error

Thanks

@afischer-sfly
Copy link
Contributor Author

no problem, just pushed a revised change that includes logging. Happy to make any other changes too.

\cc @afischer-sfly @akenn @jeeyoungk @paldepind

Guys are you use karma-jasmine?

Yup we use karma-jasmine

@maksimr
Copy link
Contributor

maksimr commented Mar 26, 2015

@afischer-sfly Thanks!

Left to correct jshint errors

@jmcmichael
Copy link

For what it's worth, I'm using karma, mocha, chai, sinon and got the 'TypeError: Line must be greater or equal to 1, got 0' from source-map when setting up sinon-as-promised. Applying this PR fixed it.

@maksimr
Copy link
Contributor

maksimr commented Mar 26, 2015

LGTM

@dazz
Copy link

dazz commented Mar 31, 2015

👍 When can we get this PR in?

@maksimr
Copy link
Contributor

maksimr commented Mar 31, 2015

@dazz
When somebody else from karma team make review this issue

Thanks

@mtscout6
Copy link
Member

/cc @karma-runner/owners Can we get a second sign off on this?

@jerryorr
Copy link

jerryorr commented Apr 1, 2015

I use karma + jasmine and got this error when running against Firefox (not Chrome, interestingly). This patch fixed it.

@Sjors
Copy link

Sjors commented Apr 6, 2015

I ran into the same error with Phantomjs2. I then added the commit from this pull request to the current master in my own fork, ran "grunt build" and the error was replaced by a warning. That and lots of other error messages, but hopefully those are specific to my project.

@BrentWheeldon
Copy link

Can this get merged in, please?

maksimr added a commit that referenced this pull request Apr 10, 2015
fix: catch exceptions from SourceMapConsumer
@maksimr maksimr merged commit 482654c into karma-runner:master Apr 10, 2015
@zzo
Copy link
Contributor

zzo commented Apr 10, 2015

Thanks @maksimr! Still working on getting the release process sorted out.

@maksimr
Copy link
Contributor

maksimr commented Apr 10, 2015

@zoo no problems!

This PR fix only symptom, source of the problem I describe here. Under the task #1274 we can make more robust solution, about which said @vojtajina

I think we should correct the regular expression to parse Firefox error stacks, instead of ignoring these errors.

@dtabuenc
Copy link

I don't like just catching the error, but it seems to me it shouldn't even try to get original location from sourcemap if the regular expression did not match a line number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.