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

NEW
Unassigned

Status

()

Core
XPConnect
6 years ago
2 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Unassigned)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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?
Created attachment 608190 [details] [diff] [review]
patch. v1

Attaching a patch, flagging jst for review.

jst - this blocks compartment-per-global, so a speedy review would be appreciated. ;-)
Attachment #608190 - Flags: review?(jst)
Keywords: dev-doc-needed
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.
Created attachment 608361 [details] [diff] [review]
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.
Attachment #608190 - Attachment is obsolete: true
Attachment #608190 - Flags: review?(jst)
Attachment #608361 - Flags: review?(jst)
Created attachment 608937 [details] [diff] [review]
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.
Attachment #608361 - Attachment is obsolete: true
Attachment #608361 - Flags: review?(jst)
Attachment #608937 - Flags: review?(jst)
(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.
Changing the bug title per comment 5.
Summary: Exceptions thrown from XPCWrappedJS with Components.Exception should not be reported → Components.Exception should provide a way for XPCWrappedJS exceptions to be propagated instead of being reported
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.
Attachment #608937 - Attachment is obsolete: true
Attachment #608937 - Flags: review?(jst)
Depends on: 743843
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.
Created attachment 613417 [details] [diff] [review]
Part 1 - Add parser support for the 'propagate' option. v1
Created attachment 613418 [details] [diff] [review]
Part 2 - Hook up the 'propagate' option in XPCWrappedJS. v1
No longer blocks: 650353
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 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
Duplicate of this bug: 380169
Assignee: bobbyholley → nobody
You need to log in before you can comment on or make changes to this bug.