738119 - Components.Exception should provide a way for XPCWrappedJS exceptions to be propagated instead of being reported

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The current policy is that XPCWrappedJSClass reports errors if there is no more JS above it on the callstack. This is good for things like event handlers, because people expect to see exceptions thrown in event handles recorded somewhere.

However, this doesn't work for the case where JS throws an exception designed to be handled by calling native code. For example, when CertUtils.jsm throws, it expects to trigger a cancellation of the HTTP request by necko code.

So the compromise we came up with is to special-case exception thrown via Components.exception as not needing reporting.

Should we publicize this somewhere?

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch. v1

Attaching a patch, flagging jst for review.

jst - this blocks compartment-per-global, so a speedy review would be appreciated. ;-)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 608190 [details] [diff] [review]
patch. v1

Just driving by, don't forget to bump your uuid and it looks like mFlags is not initialized to 0.

Comment 3

[Bobby Holley (:bholley)]

Attachment: patch. v2

> Just driving by, don't forget to bump your uuid and it looks like mFlags is not initialized to 0.

Doh, that's what I get for writing patches in a hurry. Updating and reflagging.

Comment 4

[Bobby Holley (:bholley)]

Attachment: patch. v3

Didn't do a full rebuild last time, so I didn't notice that nsDOMScriptObjectFactory.cpp mucks around with nsIXPCException too. Hopefully, that should be everything.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #0)
> So the compromise we came up with is to special-case exception thrown via
> Components.exception as not needing reporting.
> 
> Should we publicize this somewhere?

I'm not sure this is a great plan. Components.Exception has been widely promoted as "the right way to throw exceptions such that they'll be nicely formatted when they're reported", see e.g. all the exceptions in NetUtil.jsm, WeaveCrypto.js, nsContentPrefService.js, nsSearchService.js, etc.

Comment 6

[Bobby Holley (:bholley)]

Changing the bug title per comment 5.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 608937 [details] [diff] [review]
patch. v3

We're going to do this slightly differently now.

I think jst is pretty busy at the moment, so I'll flag bent for review on the ensuing patches. They should be easy to review.

Comment 8

[Bobby Holley (:bholley)]

So somehow, after I wrote the fix for this, something changed on mozilla-central so that the issue giving rise to this bug (browser_updatessl.js test failure with compartment-per-global) disappeared. As such, there's no longer an easy script-based way to test this the 'propagate' option I created.

So I'm going to get the dependent refactoring landed (split off into bug 743843), which includes all the stuff for an options object. I'll then leave the other patches in this bug, so that we can quickly grab and land them if the issue resurfaces.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add parser support for the 'propagate' option. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hook up the 'propagate' option in XPCWrappedJS. v1

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 613417 [details] [diff] [review]
Part 1 - Add parser support for the 'propagate' option. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +1944,5 @@
>  
> +    bool parsePropagate(jsval v) {
> +        JSBool propagate;
> +        if (!JS_ValueToBoolean(cx, v, &propagate))
> +            return false;

You can assert this doesn't fail.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 613418 [details] [diff] [review]
Part 2 - Hook up the 'propagate' option in XPCWrappedJS. v1

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

::: js/xpconnect/src/xpcprivate.h
@@ +3520,5 @@
>      char*           mFilename;
>      int             mLineNumber;
>      nsIException*   mInner;
>      bool            mInitialized;
> +    PRUint32        mFlags;

Looks like suboptimal packing

Comment 14

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.