Closed Bug 1275704 Opened 4 years ago Closed 4 years ago

Simplify the "reportable" computation in XPCWrappedJSClass::CheckForException

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Blocks: 981209
Attachment #8756527 - Flags: review?(bobbyholley) → review+
Attachment #8756528 - Flags: review?(bobbyholley) → review+
Attachment #8756534 - Flags: review?(bobbyholley) → review+
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.
> 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.
You need to log in before you can comment on or make changes to this bug.