1062631 - Stop eating exceptions in NS_ScriptErrorReporter

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a678fd6e16b8

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=fab86083ca9a

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c00043b8e864

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix buggy |browser_SignInToWebsite.js| test. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use SpecialPowers.wrapCallback for QI in more tests. v1

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix up content QI security check and tests. v1

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.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Bail out in browser_cookies_exceptions when |observances| is empty. v1

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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Handle null originatingURI in browser-addons.js. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Watch out for dead object proxies in gXPInstallObserver. v1

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 7 - Handle null ownerDoc in browser.js and content.js for the benefit of browser_newtab_drag_drop_ext.js. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 8 - Stop leaking observers in search tests. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 9 - Handle nuked window references in tilt tests. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove exception swallowing code in NS_ScriptErrorReporter. v1

See bug 1062631 comment 0.

Comment 14

[Bobby Holley (:bholley)]

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 15

[Bobby Holley (:bholley)]

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 16

[Boris Zbarsky [:bzbarsky]]

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 17

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[Bobby Holley (:bholley)]

(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.

Comment 20

[Bobby Holley (:bholley)]

(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 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Bobby Holley (:bholley)]

(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 23

[:Gijs (he/him)]

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 24

[:Gijs (he/him)]

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.

Comment 25

[:Gijs (he/him)]

(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 26

[:Gijs (he/him)]

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 27

[:Gijs (he/him)]

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 28

[:Gijs (he/him)]

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 29

[:Gijs (he/him)]

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?

Comment 30

[:Gijs (he/him)]

(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)

Comment 31

[:Gijs (he/him)]

(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?

Comment 32

[Bobby Holley (:bholley)]

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 33

[Paul Rouget [:paul]]

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 34

[Bobby Holley (:bholley)]

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.

Comment 35

[Bobby Holley (:bholley)]

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

Comment 36

[Carsten Book [:Tomcat]]

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