1096341 - ToJSValue overload returns false without setting exception

Status: RESOLVED FIXED

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

Description

[Tom Tromey :tromey]

The ToJSValue contract says:

// If ToJSValue returns false, it must set an exception on the
// JSContext.


However the last overload in ToJSValue.cpp does:

  if (!JS_GetPendingException(aCx, aValue)) {
    return false;
  }

... returning false when no exception is pending.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So if JS_GetPendingException returned false then either:

1)  ThrowMethodFailedWithDetails somehow failed to throw an exception

or

2)  JSContext::getPendingException returned false without throwing.

The latter can only happen if wrap() returns false without throwing, which it shouldn't.

So I expect that JS_GetPendingException can never actually return false here... but falling back to undefined and returning true if it does return false might not be insane.  Can't think of any better options.

Comment 2

[Tom Tromey :tromey]

Another option might be to simply assert that ThrowMethodFailedWithDetails
did what it said it was going to do.  It also seems to me that
JS_GetPendingException can't return false here.

Comment 3

[Tom Tromey :tromey]

Attachment: require an exception to be set in ToJSValue overload

Here's what I was thinking for the assertion approach.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, that would make sense to me.

Comment 5

[Tom Tromey :tromey]

https://tbpl.mozilla.org/?tree=Try&rev=3ab4e7725195

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8520011 [details] [diff] [review]
require an exception to be set in ToJSValue overload

The test results look good.
I wasn't sure if that previous comment constituted a review,
so marking for review on the assumption it was not :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8520011 [details] [diff] [review]
require an exception to be set in ToJSValue overload

r=me

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cec52c43496

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cec52c43496