For a long time, we've had code in NS_ScriptErrorReporter that avoids reporting when various hand-wavy heuristics indicate that there might be more script on the stack [1]. Sometime along the way, the JS engine changed such that it always cleared the pending exception after passing it off to the error reporter. So today, this machinery serves purely to arbitrarily swallow exceptions. Bill mildly improved the observable behavior with yet another hack in bug 895340, but it doesn't cover all the cases. I think we should remove this stuff wholesale, which is the first step down the long road to making our exception story sane. The hardest part of this bug is that fixing it uncovers (and propagates) a number of preexisting (and previously swallowed) exceptions in browser and test code. I'll try to fix bugs when they're obvious, but where they're not, I'm just going to preserve existing behavior (i.e. silent failure). If you're reviewing one of those patches and are interested in digging deeper into the failure, please file a followup. [1] Note that this is distinct from the separate set of heuristics used by XPCWrappedJSClass when reporting exceptions.

Otherwise the QI throws as soon as it tries to pass the IID into content code.

This stuff is broken in a bunch of ways. The test isn't doing the right thing because it's using SpecialPowers. When we pass the content object to xpconnectArgument(), it gets wrapped in a SpecialPowers wrapper, so the check succeeds, only to fail down the line when the QI method is actually invoked with an IID (which we can't pass to content scopes anyway). This whole security check is probably useless given the above, but let's be safe, make it a bit more robust, and fix up the test while we're at it.

This test says this case shouldn't happen, but it does. If this test included an ok(false) at this point, it would be failing on tinderbox. This patch preserves the existing behavior of bailing out, and just does it explicitly rather than with an unreported exception. If this behavior is something that needs to be investigated, please file a followup.

In browser_navigateaway2.js we end up with an XPCWrappedJS double-wrapped DeadObjectProxy here, which triggers an exception when it's inserted into the WeakMap.

See bug 1062631 comment 0.

Gijs - I used you as a catch-all for all of the browser stuff so that I don't have to track down an army of people who might be on PTO or might not work on this stuff anymore. Feel free to reassign any of them if there's someone more appropriate. :-)

Comment on attachment 8484423 [details] [diff] [review] Part 10 - Remove exception swallowing code in NS_ScriptErrorReporter. v1 Er, meant to flag bz on this one.

Comment on attachment 8484413 [details] [diff] [review] Part 2 - Use SpecialPowers.wrapCallback for QI in more tests. v1 Why doesn't content/base/test/test_bug375314.html, say, need this? Or the various other tests that have QI functions that this patch is not changing?

Comment on attachment 8484415 [details] [diff] [review] Part 3 - Fix up content QI security check and tests. v1 r=me

Comment on attachment 8484423 [details] [diff] [review] Part 10 - Remove exception swallowing code in NS_ScriptErrorReporter. v1 Pretty happy to see this code go... and I assume you've tested that the browser console isn't just full of gunk with this patch?

(In reply to Boris Zbarsky [:bz] from comment #16) > Comment on attachment 8484413 [details] [diff] [review] > Part 2 - Use SpecialPowers.wrapCallback for QI in more tests. v1 > > Why doesn't content/base/test/test_bug375314.html, say, need this? It uses it below, see the line: policy = SpecialPowers.wrapCallbackObject(policy); > Or the > various other tests that have QI functions that this patch is not changing? I fixed most of them when I introduced the callback wrapping machinery a while ago. These are just the stragglers that were failing silently while the test still passed.

(In reply to Boris Zbarsky [:bz] from comment #18) > tested that the browser console isn't just full of gunk with this patch? A rough smoketest indicates no change in behavior.

Comment on attachment 8484413 [details] [diff] [review] Part 2 - Use SpecialPowers.wrapCallback for QI in more tests. v1 Alright, looks pretty good. Why does uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml not need any changes?

(In reply to Boris Zbarsky [:bz] from comment #21) > Why does uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml not > need any changes? Because it uses enablePrivilege.

Comment on attachment 8484412 [details] [diff] [review] Part 1 - Fix buggy |browser_SignInToWebsite.js| test. v1 Review of attachment 8484412 [details] [diff] [review]: ----------------------------------------------------------------- I have a feeling I'm going to be saying "wow, this is silly" a lot when doing these reviews... Anyway, filed bug 1063404 for this one. ::: browser/modules/test/browser_SignInToWebsite.js @@ -254,5 @@ > }, "test-identity-auth-window", false); > > let winObs = new WindowObserver(function(authWin) { > ok(authWin, "Authentication window opened"); > - ok(authWin.contentWindow.location); Please just comment out the original instead; truthy-checking authWin.location doesn't make much sense here.

Comment on attachment 8484416 [details] [diff] [review] Part 4 - Bail out in browser_cookies_exceptions when |observances| is empty. v1 Review of attachment 8484416 [details] [diff] [review]: ----------------------------------------------------------------- Filed bug 1063410.

(In reply to :Gijs Kruitbosch from comment #24) > Comment on attachment 8484416 [details] [diff] [review] > Part 4 - Bail out in browser_cookies_exceptions when |observances| is empty. > v1 > > Review of attachment 8484416 [details] [diff] [review]: > ----------------------------------------------------------------- > > Filed bug 1063410. (for both this and the previous patch, please add a comment referencing the bug)

Comment on attachment 8484417 [details] [diff] [review] Part 5 - Handle null originatingURI in browser-addons.js. v1 Review of attachment 8484417 [details] [diff] [review]: ----------------------------------------------------------------- Umm... I have a different suggestion here, and filed bug 1063418. :-) ::: browser/base/content/browser-addons.js @@ +79,5 @@ > PopupNotifications.show(browser, notificationID, messageString, anchorID, > action, null, options); > break; > case "addon-install-blocked": > + let originatingHost = installInfo.originatingURI ? installInfo.originatingURI.host : ""; This isn't good enough because originatingURI might not be an nsStandardURL, which means getting .host will still throw (in particular, this will be the case for data: and about: URIs). We should also add a string here for "unknown host" or something similar, because the composed formatted string here ("Firefox prevented this site () from asking you to install software on your computer") is going to look broken. I'm also not sure under what circumstances the full URI is null... So for now, I'd recommend: let originatingHost; try { originatingHost = installInfo.originatingURI.host; } catch (ex) { // Need to deal with missing originatingURI and with about:/data: URIs more gracefully, // see bug 1063418 - but for now, bail: return; }

Comment on attachment 8484419 [details] [diff] [review] Part 7 - Handle null ownerDoc in browser.js and content.js for the benefit of browser_newtab_drag_drop_ext.js. v1 Review of attachment 8484419 [details] [diff] [review]: ----------------------------------------------------------------- Tim, do you know why the newtab dnd test is sending dead stuff here?

Comment on attachment 8484420 [details] [diff] [review] Part 8 - Stop leaking observers in search tests. v1 Review of attachment 8484420 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_searchSuggestionUI.js @@ +142,5 @@ > // Wait for Satchel to say it's been added to form history. > let deferred = Promise.defer(); > Services.obs.addObserver(function onAdd(subj, topic, data) { > if (data == "formhistory-add") { > + Services.obs.removeObserver(onAdd, "satchel-storage-changed"); Can you make these function expression names unique, please? Even if it does work as-is (I think? fn expression rather than statement, so doesn't get hoisted, so they hopefully maybe each refer to their own thing because the yields guarantee execution order?), do it for my sanity, because the second onAdd should really be called onRemove, looking at what it does...

Comment on attachment 8484421 [details] [diff] [review] Part 9 - Handle nuked window references in tilt tests. v1 Review of attachment 8484421 [details] [diff] [review]: ----------------------------------------------------------------- Paul, seems like tilt shouldn't be sending notifications for dead windows? Can you look if this warrants investigation/fixing outside this test?

(FWIW, if I could have all exceptions visible all the time everywhere yesterday, I would say yes, so if my review redirects start slowing you down in landing this, please re-ping me and I can chase people / rs where necessary)

(In reply to Bobby Holley (:bholley) from comment #1) > https://tbpl.mozilla.org/?tree=Try&rev=a678fd6e16b8 This already had attachment 8484417 [details] [diff] [review] - over in bug 1063418, we're wondering how/where the browser-addons null originatingURI case happens. Are there older trypushes and/or do you know where we hit this in automated testing?

The problems were in toolkit/mozapps/extensions/test/xpinstall/browser_localfile3.js and toolkit/mozapps/extensions/test/xpinstall/browser_whitelist7.js . See https://tbpl.mozilla.org/?tree=Try&rev=531613bd40cc I'd really like to get this landed so that I can move on with other stuff and not worry about people adding more swallowed bugs. Can you rs the last two patches (which preserve the existing behavior), and then we can worry about any further investigation in a followup?

Comment on attachment 8484421 [details] [diff] [review] Part 9 - Handle nuked window references in tilt tests. v1 I don't want to block you on this. I'll file a follow-up bug to address this test issue.

Comment on attachment 8484419 [details] [diff] [review] Part 7 - Handle null ownerDoc in browser.js and content.js for the benefit of browser_newtab_drag_drop_ext.js. v1 ttaubert and I spoke on IRC. I filed bug 1064508 for the followup investigation and am landing this fix as-is for now.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1342aa69239b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5379a1c9ef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef955db1f0d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/82ae715bdabe remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1169552dd7d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a073f33687f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc9356e94ed remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e12070d26f6f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/abc137b5b527 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb9eb2a6fcb

https://hg.mozilla.org/mozilla-central/rev/1342aa69239b https://hg.mozilla.org/mozilla-central/rev/bf5379a1c9ef https://hg.mozilla.org/mozilla-central/rev/1ef955db1f0d https://hg.mozilla.org/mozilla-central/rev/82ae715bdabe https://hg.mozilla.org/mozilla-central/rev/f1169552dd7d https://hg.mozilla.org/mozilla-central/rev/5a073f33687f https://hg.mozilla.org/mozilla-central/rev/3fc9356e94ed https://hg.mozilla.org/mozilla-central/rev/e12070d26f6f https://hg.mozilla.org/mozilla-central/rev/abc137b5b527 https://hg.mozilla.org/mozilla-central/rev/bfb9eb2a6fcb