Closed Bug 1393743 Opened 4 years ago Closed 4 years ago

Mochitest promise rejection errors report the stack of the promise rejection catcher instead of the stack for the error used as a rejection

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Gijs, Assigned: jryans)

References

Details

Attachments

(1 file)

https://treeherder.mozilla.org/logviewer.html#?job_id=125840434&repo=mozilla-inbound&lineNumber=3246

[task 2017-08-25T10:12:47.529920Z] 10:12:47     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_lazy.js | A promise chain failed to handle a rejection: Invalid sidebar broadcaster specified:  - stack: null
[task 2017-08-25T10:12:47.532686Z] 10:12:47     INFO - Rejection date: Fri Aug 25 2017 10:12:44 GMT+0000 (UTC) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 265
[task 2017-08-25T10:12:47.534799Z] 10:12:47     INFO - Stack trace:
[task 2017-08-25T10:12:47.538376Z] 10:12:47     INFO -     resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:265
[task 2017-08-25T10:12:47.541953Z] 10:12:47     INFO -     chrome://mochikit/content/browser-test.js:Tester_execTest/<:818
[task 2017-08-25T10:12:47.546248Z] 10:12:47     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
[task 2017-08-25T10:12:47.550287Z] 10:12:47     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
[task 2017-08-25T10:12:47.553862Z] 10:12:47     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-08-25T10:12:47.560627Z] 10:12:47     INFO - Leaving test bound 

This is useless. The code in question ( https://dxr.mozilla.org/mozilla-central/rev/892c8916ba32b7733e06bfbfdd4083ffae3ca028/browser/base/content/browser-sidebar.js#340 ) does:

        reject(new Error("Invalid sidebar broadcaster specified: " + commandID));

so the error itself has a stack, and it should be displaying that instead.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment on attachment 8957359 [details]
Bug 1393743 - Use reason stack when present for promise rejection in tests.

https://reviewboard.mozilla.org/r/226288/#review232312

Thanks for doing this, that's very helpful!

I'll take another look at the patch after some small simplification and better null handling.

::: toolkit/modules/tests/modules/PromiseTestUtils.jsm:135
(Diff revision 1)
>    },
>  
>    // UncaughtRejectionObserver
>    onLeftUncaught(promise) {
>      let message = "(Unable to convert rejection reason to string.)";
> +    let reason;

"let reason = null;", so it's clear what happens if PromiseDebugging.getState() fails.

::: toolkit/modules/tests/modules/PromiseTestUtils.jsm:150
(Diff revision 1)
>      // We should convert the rejection stack to a string immediately. This is
>      // because the object might not be available when we report the rejection
>      // later, if the error occurred in a context that has been unloaded.
>      let stack = "(Unable to convert rejection stack to string.)";
>      try {
> -      stack = "" + PromiseDebugging.getRejectionStack(promise);
> +      let rejectionStack = PromiseDebugging.getRejectionStack(promise);

Simply:

stack = "" + (PromiseDebugging.getRejectionStack(promise) ||
              (reason && reason.stack) ||
              "(No stack available.)");

Last message optional, if you think printing either "null" or "undefined" is good enough.
Attachment #8957359 - Flags: review?(paolo.mozmail)
Comment on attachment 8957359 [details]
Bug 1393743 - Use reason stack when present for promise rejection in tests.

https://reviewboard.mozilla.org/r/226288/#review232312

> "let reason = null;", so it's clear what happens if PromiseDebugging.getState() fails.

Okay, fixed!

> Simply:
> 
> stack = "" + (PromiseDebugging.getRejectionStack(promise) ||
>               (reason && reason.stack) ||
>               "(No stack available.)");
> 
> Last message optional, if you think printing either "null" or "undefined" is good enough.

Seems reasonable!  I did something similar, but moved the `"" +` to a separate line.
Comment on attachment 8957359 [details]
Bug 1393743 - Use reason stack when present for promise rejection in tests.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Seems reasonable!  I did something similar, but moved the `"" +` to a
> separate line.

Eh, that's not okay though :-( I know it's tricky code...

If an exception occurs while converting one of the objects to a string, we would still end up with an object assigned to the "stack" variable, and as noted in the comment before this code block, we may fail when trying to use the objects later. In some edge cases, we may also have leaks related to circular references or closures in the exception object.
Attachment #8957359 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #5)
> Comment on attachment 8957359 [details]
> Bug 1393743 - Use reason stack when present for promise rejection in tests.
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> > Seems reasonable!  I did something similar, but moved the `"" +` to a
> > separate line.
> 
> Eh, that's not okay though :-( I know it's tricky code...
> 
> If an exception occurs while converting one of the objects to a string, we
> would still end up with an object assigned to the "stack" variable, and as
> noted in the comment before this code block, we may fail when trying to use
> the objects later. In some edge cases, we may also have leaks related to
> circular references or closures in the exception object.

Hmm, I see...  I just dislike the style of so many inline expressions. :)

Anyway, I'll change to what you suggest.
Comment on attachment 8957359 [details]
Bug 1393743 - Use reason stack when present for promise rejection in tests.

https://reviewboard.mozilla.org/r/226288/#review232408
Attachment #8957359 - Flags: review?(paolo.mozmail) → review+
I mean, you could use an intermediate "let" statement, but I think the style I suggested is equally readable...
(In reply to :Paolo Amadini from comment #9)
> I mean, you could use an intermediate "let" statement, but I think the style
> I suggested is equally readable...

Eh, it's quite a small thing in the end.  Thanks for the review! :)
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/827bf0695bcf
Use reason stack when present for promise rejection in tests. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/827bf0695bcf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.