Closed Bug 1260511 Opened 7 years ago Closed 6 years ago

navigator.registerProtocolHandler throws a number as an exception, rather than an Error object


(Core :: DOM: Core & HTML, defect)

45 Branch
Not set



Tracking Status
firefox48 --- fixed


(Reporter:, Assigned: bzbarsky)



(Keywords: dev-doc-complete, testcase, Whiteboard: btpp-fixlater)


(4 files)

try {
  navigator.registerProtocolHandler(0, 0, 0);
} catch (e) {
  alert(typeof e + ":" + e.toString(16));

result: "number:8053000c".

It looks like it's throwing an HRESULT (or another type of 32-bit error code), rather than an Error object.
Adding ddn so that we actually document the exception raised :-) May be useful for web devs.
Keywords: dev-doc-needed
Doug, seems like you touched this code at some point.
Flags: needinfo?(dougt)
Keywords: crash
XPConnect and exceptions, what's not to like?

So the basic structure of registerProtocolHandler is that it has an ErrorResult arg, calls into a JS-implemented XPCOM component to do the work, then throws the resulting nsresult on its ErrorResult.  In this case that's NS_ERROR_DOM_SYNTAX_ERR, indeed.

Then we end up in ErrorResult::SetPendingException to actually throw on the JSContext, see we just have an nsresult, so call SetPendingGenericErrorException, which calls dom::Throw.

Normally, this would do CreateException(), which would see this is a type of error code for which we create DOMExceptions and do so.  But in this case, CycleCollectedJSRuntime::Get()->GetPendingException() returns a non-null value.  And that non-null value has our error code as its error code, so we decide to reuse it.  So we go off to ThrowExceptionObject with that value, which lands in the "Exception*" overload of ThrowExceptionObject and this code block:

62   // If we stored the original thrown JS value in the exception
63   // (see XPCConvert::ConstructException) and we are in a web context
64   // (i.e., not chrome), rethrow the original value. This only applies to JS
65   // implemented components so we only need to check for this on the main
66   // thread.
67   if (NS_IsMainThread() && !nsContentUtils::IsCallerChrome() &&
68       aException->StealJSVal(thrown.address())) {
69     if (!JS_WrapValue(aCx, &thrown)) {
70       return false;
71     }
72     JS_SetPendingException(aCx, thrown);
73     return true;
74   }

So yeah.  Of course the JS value in aException is the number that was thrown.  That gets set up in nsXPCWrappedJSClass::CheckForException after the call into the JS component, when we do XPCConvert::JSValToXPCException which has an entire "is this a number that looks like an nsresult? wrap it up into an nsIException" codepath.  Of course this is all totally broken conceptually, because it ends up explicitly throwing a number if the JS-implemented component did so, instead of constructing a DOMException from it like things normally would.

More generally, though, it seems to me that when calling into dom::Throw from bindings we want to ignore whatever the exception hanging out on the runtime is.  There is pretty much no chance that it's the right thing.  The only way that could happen is if a JS component created a content-side exception object from the right global (how did it get that global?) and threw it.  Then XPCConvert::JSValToXPCException would just leave the exception be and we would pick it up here.  And even then it would have the same nsresult as what we're trying to throw, so the only thing we might miss out on is a custom error message that it has... which I expect none of our JS components are doing.

So a few things we may want to do:

1)  In ThrowExceptionObject (the Exception* overload), if we get a number value as the original value check for it being an nsresult (and in particular the nsresult that the exception claims to be for) and if so go back through the dom::Throw codepath, so the right DOMException gets constructed from it.

2)  In general, skip the entire "check for an existing thing" bit of dom::Throw when calling it from at least the ErrorResult code.  Certainly when calling it from ErrorResult::SetPendingDOMException, since in that case we would otherwise lose our provided custom error message!

3)  While we're here, do SetPendingException(nullptr) in dom::Throw even if we're NOT reusing the existing exception off the runtime, so it won't hang around anymore.  That would more closely match what XPCThrower::CheckForPendingException does...

Kyle, Bobby, any obvious objections to this plan?  I expect either of item 1 or item 2 are enough to fix this particular API on their own, so I'm OK with just doing #2 if we think #1 is extra complexity we don't need...
Flags: needinfo?(khuey)
Flags: needinfo?(dougt)
Flags: needinfo?(bobbyholley)
That sounds fine to me.
Flags: needinfo?(khuey)
Upon further thought, we should only do #2 when we're calling from ErrorResult::SetPendingDOMException.  If we do it that way, this should allow us to have JS components actually throw the right things if they try a bit.
Whiteboard: btpp-fixlater
This is needed for later patches, because right now we have a failure annotation but fail for different reasons in e10s and not: one throws the wrong thing and the other doesn't throw at all.  When I make the former throw the right thing, try gets unhappy on either the e10s or non-e10s tests.... so let's make both codepaths throw the wrong thing in this changeset and the right thing in the next one.
Attachment #8743550 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → bzbarsky
Please let me know if we want to do that on the MIME type mismatch too, now that we can do it
Attachment #8743553 - Flags: review?(gijskruitbosch+bugs)
See Also: → 1258071
Comment on attachment 8743550 [details] [diff] [review]
part 1.  Fix WebContentConverter to behave consistently in both e10s and non-e10s mode in terms of the argument sanity checking registerContentHandler does

Review of attachment 8743550 [details] [diff] [review]:

::: browser/components/feeds/WebContentConverter.js
@@ +478,5 @@
>      if (contentType != TYPE_MAYBE_FEED)
>        return;
>      if (aWindowOrBrowser) {
>        let haveWindow = (aWindowOrBrowser instanceof Ci.nsIDOMWindow);

Nit: remove this line while we're here, so that we don't shadow the earlier variable with the exact same definition.
Attachment #8743550 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8743553 [details] [diff] [review]
part 4.  Fix some of the places where registerProtocolHandler should be throwing a SECURITY_ERR to actually do so

Review of attachment 8743553 [details] [diff] [review]:

::: browser/components/feeds/WebContentConverter.js
@@ +144,4 @@
>        let baseURI = aContentWindow.document.baseURIObject;
>        uri = this.makeURI(aURIString, null, baseURI);
>      } catch (ex) {
>        throw NS_ERROR_DOM_SYNTAX_ERR;

Do we not intend to make these throw statements also throw "sensible" exceptions, whatever that means?

@@ +152,4 @@
>      if (uri.scheme != "http" && uri.scheme != "https")
> +      this.throwSecurityError(
> +        "Permission denied to add " + uri.spec + " as a content or protocol handler",
> +        aContentWindow);

So... we use eslint now. It is reasonably smart, but because javascript, it doesn't really deal well with exception-based control flow that's abstracted away in utility methods. So in this case, it won't understand that we will never go past this statement, which has all kinds of annoying consequences for things like checking that we always (or never) return a value from methods.

Instead of this.throwSecurityError, can we make it something like:

throw this.getSecurityError(...)

and have getSecurityError just return the exception/string, but not throw it?

Orthogonal nit: for multi-line if() bodies, I would prefer braces. We're touching blame enough in this bug anyway that I think that touching the condition doesn't matter too much.

@@ +489,5 @@
>      // We only support feed types at present.
>      // XXX this should be a "security exception" according to spec, but that
>      // isn't defined yet.
> +    // XXXbz Should the security exception be thrown instead of just returning
> +    // here?  Or something else?  See Utils.throwSecurityError.

Yes, please remove the existing comment and just throw as you do elsewhere, with something like "Only feed-based content-type content handlers are supported" - not too picky about the exact wording if you can think of something better. From a naive reading of the test descriptions in content.html.ini it looks like that might mean we can enable more tests.
Attachment #8743553 - Flags: review?(gijskruitbosch+bugs) → review+
As stuck in bugzilla, see also bug 1258071. I'm not too bothered if we fix all the cases in this bug or not (though depending on the cause of comment #0, maybe we should?), but out of curiosity, what would be the "right" fix?
> Do we not intend to make these throw statements also throw "sensible" exceptions, whatever that means?

Well.  The machinery in "part 2" of this bug will make that statement throw a DOMException with name == "SyntaxError" and the default message for that error type.  In this case, "An invalid or illegal string was specified".

We could explicitly throw a DOMException with some custom error message if we wanted, but I didn't really have a good idea of what that error message should be, so...

> Instead of this.throwSecurityError, can we make it something like:

Sure, that makes sense.  I guess that's clearer at the callsite too.

> Orthogonal nit: for multi-line if() bodies, I would prefer braces

Oh, yes.  I'd totally missed that those were unbraced to start with!  Too used to code where everything is always braced.  ;)

> but out of curiosity, what would be the "right" fix?

For an API defined in a specification, the right fix is to throw either an nsresult corresponding to the correct DOMException type (which the part 2 changes from this bug will then convert to a DOMException on the content side) or an explicit DOMException with a custom message.  Here "correct" means "whatever the relevant specification defines".

For an API that's just something we made up, we can do whatever, obviously.  But a general rule of thumb is that we should try to only expose DOMExceptions and JS Error objects to the web, so should at least throw nsresults that would get converted to a DOMException.  In practice that means an nsresult from one of the following modules:

> Yes, please remove the existing comment and just throw as you do elsewhere

OK, done, with "Only content handlers for feed types are supported".
> OK, done, with "Only content handlers for feed types are supported".

Actually, if I do that I have to annotate a bunch more tests as failing due to the security exception.  In fact, there is no call that the tests make to registerContentHandler that can succeed with this change.

I believe this is perfectly valid behavior on our part per spec, though (the spec explicitly says to throw for any type deemed privileged by the UA, and we deem all non-feed types privileged), and that the tests are therefore just wrong...
Flags: needinfo?(james)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bobbyholley)
I spun off the issue of what to do for unsupported types to bug 1266492 and will just keep the no-op behavior for now, with updated comments pointing to that bug.
Flags: needinfo?(james)
Flags: needinfo?(gijskruitbosch+bugs)
Any sensible cleanup of error reporting gets a thumbs-up from me. Sorry I don't have the cycles to get more-deeply involved.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.