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

Display stack for uncaught errors in the browser #3952

Closed
wants to merge 4 commits into from

Conversation

prust
Copy link
Contributor

@prust prust commented Jun 12, 2019

Description of the Change

This change captures the stack of uncaught exceptions in the browser, so it will be displayed in Mocha. Before this change, uncaught exceptions in the browser are displayed with a URL and line number, but without a stack:

no-stack

This pull request updates the global error handler for the browser so that the stack is captured:

with-stack

The issue was that the browser-specific hook for uncaughtExceptions in browser-entry.js was using the pre-2014 three-parameter window.onerror, which only provides the error message, the URL and the line number. In 2014, two more parameters were added in both the spec and Chrome, with other browsers following shortly thereafter (including IE11) -- the last of these parameters is the Error object itself, which has a populated stack.

Alternate Designs

The design could be simplified by only supporting the five-parameter onerror(), but since mocha is used in all sorts of environments, I figured it was safest to use the err parameter if it exists and to fall back to the previous functionality if it doesn't.

The design could be a little more complex if it is important to preserve the message provided to the onerror handler, which -- in Chrome, at least -- has an extra "Uncaught Error: " prefix. This would add a couple more lines to the top of onerror in browser-entry.js:

if (err)
  err.message = msg;

Why should this be in core?

This would bring consistency to the quality of error reporting in mocha's core (uncaught exceptions in Node and caught exceptions in Node and the Browser all include stacks).

Benefits

The inclusion of the stack will make it easier for developers to determine the cause of the error.

Possible Drawbacks

With this particular design choice, if a mocha user was grepping or testing for the browser-supplied "Uncaught Error:" prefix in a browser test, this change would cause it to not be found. If this is important, the two lines mentioned above in "Alternate Designs" could be added.

Applicable issues

Regarding semver, I think I would call this an enhancement (providing stack traces for uncaught exceptions). I suppose it could be considered a breaking change, since it is hypothetically possible that someone is relying on the browser-supplied "Uncaught Error:" prefix somehow, but this is highly unlikely. If it's a concern, it could be address by the 2-line fix above.

Tests

I did write a browser-specific test for this (which was orders of magnitude harder than fixing the issue itself), but the uncaught exception is causing the test to fail. I tried passing --allow-uncaught true/false in the relevant mocha.opts, as well as mocha.allowUncaught(), but neither made a difference.

Perhaps I should try putting the test with the allowUncaught or uncaught() tests in runner.spec.js, but it looks like the allowUncaught tests always throw an error somewhere that it will be caught, and the uncaught() tests don't actually throw errors (they just pass an error object directly to uncaught()), the former would probably run into the same issue I'm currently running into and the latter wouldn't exercise the code in question.

If someone with familiarity with the mocha's exception-testing tests could lend a hand or direct me in a direction that is likely to work, I would appreciate it.

Fixes #5106.

@coveralls
Copy link

coveralls commented Jun 12, 2019

Coverage Status

Coverage increased (+0.07%) to 92.925% when pulling 56e2232 on CSNW:master into ac12f2c on mochajs:master.

@prust
Copy link
Contributor Author

prust commented Jun 12, 2019

Update: @boneskull outlines the issue & how to fix it beautifully in #2983 (comment) and this is already being done in throw.spec.js. I'm going to follow suit here (and/or move this test into throw.spec.js)

@stale
Copy link

stale bot commented Oct 10, 2019

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

@stale stale bot added the stale this has been inactive for a while... label Oct 10, 2019
@Lindsay-Needs-Sleep
Copy link
Contributor

@juergba
Can you review this issue?

I have been using this improvement. It is very useful. I would like to see it in the next release.

I have also run ‘npm run test’ on this branch, everything passes.

@stale stale bot removed the stale this has been inactive for a while... label Oct 10, 2019
@juergba
Copy link
Contributor

juergba commented Oct 11, 2019

@Lindsay-Needs-Sleep I don't have the browser knowledge to review this PR.
Mocha is still supporting IE11, this: Browser compatibility doesn't help neither.

@Lindsay-Needs-Sleep
Copy link
Contributor

@juergba Do you know who might have the knowledge (and power to review this?)

Due to the way javascript works, if a browser does not support the 4th and 5th arguments of onerror they will just have a value of undefined when onerror is triggered. This is fine because in that situation, this line:

fn(err || new Error(msg + ' (' + url + ':' + line + ')'));

will evaluate to this:

fn(new Error(msg + ' (' + url + ':' + line + ')'));

Which is exactly what happened before this change.

Basically, this change just results in an improved error object for browsers that do support the 4th and 5th args. Browsers that don't, will be unaffected.

(Also, I did try out IE11. It does support the 4th and 5th args! Yay!)

Copy link
Contributor

@outsideris outsideris left a comment

Choose a reason for hiding this comment

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

I also confirmed IE11 supports onerror with 5 arguments.

@juergba
Copy link
Contributor

juergba commented Jan 30, 2020

@prust Are you still interested in this PR?
I'm trying to understand and finally get it merged.

If yes, could you please rebase? I would like to push your branch to our repo, to make sure all tests do pass correctly. There are additional tests running (eg IE11).

Does this PR solve #1979 ?

@juergba juergba added area: browser browser-specific type: feature enhancement proposal labels Jan 30, 2020
@juergba
Copy link
Contributor

juergba commented Jan 31, 2020

I did some testing in Chrome with fullTrace: true and reporter: 'spec':

describe('uncaught', function() {
  it('throw delayed error', (done) => {
    setTimeout(() => {
      throw new Error('Whoops!');
    }, 10)
    setTimeout(done, 10);
  });

Now there is even less stack information printed than before. I guess skipping the msg is not a good idea. An uncaughtException does not always mean that the runner throws and aborts. In above example the handler recovers from the error (fails the still open test) and then keeps going. In this case we loose stack information now.
I will do more testing tomorrow.

PS: Our browser related CI tests are very weak, so they don't have much relevance.

@JoshuaKGoldberg
Copy link
Member

👋 coming back to this @prust, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

I do see that it was previously approved, so I'm betting either way we can review this soon and hopefully get it merged with minimal changes.

@JoshuaKGoldberg
Copy link
Member

Moving over to #5107 with a co-authored-by attribution. Cheers! 🤎

@JoshuaKGoldberg
Copy link
Member

Ah @prust hello! I'd just made a new PR because I didn't have permissions to push to CSNW:master. If you want to make a new PR from another branch I'm happy to switch to that one, to preserve attribution for you as the author. No worries if you don't have the time though. 🙂

@JoshuaKGoldberg
Copy link
Member

#5107 is merged; we'll release a new version with the fix soon. 🚀

@prust
Copy link
Contributor Author

prust commented Jun 23, 2024

Thanks @JoshuaKGoldberg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug: Browser doesn't include call stack for global uncaught errors
6 participants