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.

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.

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.

(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

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