1275704 - Simplify the "reportable" computation in XPCWrappedJSClass::CheckForException

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In practice, the JS::DescribeScriptedCaller call will always return false if we're calling into an XPCWrappedJS backed by something from a JS component (because our AutoEntryScript set aside the JS stack due to us pushing a non-window-associated JSContext; note that this is still in a world in which JS_SaveFrameChain and multiple JSContexts both exist).  So the only cases when it might return true are when we're calling into an XPCWrappedJS backed by a function from a chrome window.

I tried looking for cases like this by having this code fatally assert if DescribeScriptedCaller returns true.  That found a few cases in a Linux64 debug try run, some in browser code and some in test code, but they were all GetInterface calls on an interface requestor implemented in window context.  Since those are set to "not reportable" anyway immediately below the DescribeScriptedCaller() call, it doesn't matter whether there's a scripted caller for those cases.

So I think we can nuke the use of DescribeScriptedCaller here entirely.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Simplify the computation of the 'reportable' boolean in nsXPCWrappedJSClass::CheckForException by taking out the special case of not reporting it if there is a scripted caller (since the only case when there is one is a JS-implemented XPCOM inter

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the now-unused aForceReport argument of nsXPCWrappedJSClass::CheckForException

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the nsXPConnect::ReportAllJSExceptions mechanism, since it's dead code now

Comment 4

[Boris Zbarsky [:bzbarsky]]

Try run I describe above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=703809cf9929

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8756527 [details] [diff] [review]
part 1.  Simplify the computation of the 'reportable' boolean in nsXPCWrappedJSClass::CheckForException by taking out the special case of not reporting it if there is a scripted caller (since the only case when there is one is a JS-implemented XPCOM inter

Review of attachment 8756527 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ -826,5 @@
> -                // See if an environment variable was set or someone has told us
> -                // that a user pref was set indicating that we should report all
> -                // exceptions.
> -                if (!reportable)
> -                    reportable = nsXPConnect::ReportAllJSExceptions();

When did these get hoisted under this branch so as to make them dead code? I'm happy for the simplification but we should check if it "regressed" recently. If we shipped it, fine by me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> When did these get hoisted under this branch so as to make them dead code?

It's not _quite_ dead code.  It's just that "ReportAllJSExceptions" doesn't include nsIInterfaceRequestor.getInterface and the NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED thing.  And if we're willing to basically accept that JS::DescribeScriptedCaller should return false here, then reportable always ends up true anyway, so no need to have a way to force it true.

It never included getInterface, going back to the initial landing of this stuff in bug 415498.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc284339ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72a83afc1b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d98afa234a

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6fc284339ae3
https://hg.mozilla.org/mozilla-central/rev/f72a83afc1b4
https://hg.mozilla.org/mozilla-central/rev/85d98afa234a