Stop eating exceptions in NS_ScriptErrorReporter

RESOLVED FIXED in mozilla35

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

1.02 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
5.03 KB, patch
Details | Diff | Splinter Review
7.12 KB, patch
Details | Diff | Splinter Review
1.44 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.39 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.52 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.21 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.06 KB, patch
paul
: review+
Details | Diff | Splinter Review
7.34 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 4

4 years ago
Created attachment 8484412 [details] [diff] [review]
Part 1 - Fix buggy |browser_SignInToWebsite.js| test. v1
Attachment #8484412 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 5

4 years ago
Created attachment 8484413 [details] [diff] [review]
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.
Attachment #8484413 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 8484415 [details] [diff] [review]
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.
Attachment #8484415 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

4 years ago
Created attachment 8484416 [details] [diff] [review]
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.
Attachment #8484416 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 8

4 years ago
Created attachment 8484417 [details] [diff] [review]
Part 5 - Handle null originatingURI in browser-addons.js. v1
Attachment #8484417 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 9

4 years ago
Created attachment 8484418 [details] [diff] [review]
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.
Attachment #8484418 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 10

4 years ago
Created 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
Attachment #8484419 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 11

4 years ago
Created attachment 8484420 [details] [diff] [review]
Part 8 - Stop leaking observers in search tests. v1
Attachment #8484420 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 12

4 years ago
Created attachment 8484421 [details] [diff] [review]
Part 9 - Handle nuked window references in tilt tests. v1
Attachment #8484421 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 13

4 years ago
Created attachment 8484423 [details] [diff] [review]
Part 10 - Remove exception swallowing code in NS_ScriptErrorReporter. v1

See bug 1062631 comment 0.
Attachment #8484423 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 14

4 years ago
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. :-)
(Assignee)

Comment 15

4 years ago
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.
Attachment #8484423 - Flags: review?(gijskruitbosch+bugs) → review?(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?
Flags: needinfo?(bobbyholley)
Comment on attachment 8484415 [details] [diff] [review]
Part 3 - Fix up content QI security check and tests. v1

r=me
Attachment #8484415 - Flags: review?(bzbarsky) → review+
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?
Attachment #8484423 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

4 years ago
(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.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 20

4 years ago
(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?
Attachment #8484413 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1063271

Comment 23

4 years ago
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.
Attachment #8484412 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 24

4 years ago
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.
Attachment #8484416 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 25

4 years ago
(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)

Updated

4 years ago
Depends on: 1063418

Comment 26

4 years ago
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;
}
Attachment #8484417 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

4 years ago
Attachment #8484418 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 27

4 years ago
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?
Attachment #8484419 - Flags: review?(gijskruitbosch+bugs) → review?(ttaubert)

Comment 28

4 years ago
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...
Attachment #8484420 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 29

4 years ago
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?
Attachment #8484421 - Flags: review?(gijskruitbosch+bugs) → review?(paul)

Comment 30

4 years ago
(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

4 years ago
(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?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 32

4 years ago
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?
Flags: needinfo?(bobbyholley) → needinfo?(gijskruitbosch+bugs)

Comment 33

4 years ago
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.
Attachment #8484421 - Flags: review?(paul) → review+
(Assignee)

Updated

4 years ago
Blocks: 1064508
(Assignee)

Comment 34

4 years ago
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.
Attachment #8484419 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.