997285 - Put Error.prototype on the proto chain of DOMExceptions

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This will just make instanceof work, but it's a start.  Hopefully no one will expect .stack to magically work as a result.

Note that this is a much more limited fix than bug 960508 and preserves all our Xray goodness.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add JS_GetErrorPrototype to JSAPI.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add [ExceptionClass] support to WebIDL.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make DOMException [ExceptionClass].

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8407681 [details] [diff] [review]
part 1.  Add JS_GetErrorPrototype to JSAPI.

You don't need this. Just call JS_GetClassPrototype(cx, JSProto_Error, ...).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Drop the custom stringifier from DOMException in favor of the default one on Error.prototype.

As far as I can tell, this changes our typical DOMException stringification from:

  [Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (SyntaxError) "  location: ""]

to:

  SyntaxError: An invalid or illegal string was specified

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8407681 [details] [diff] [review]
part 1.  Add JS_GetErrorPrototype to JSAPI.

That would require a bunch of convolutions in the codegen.  I can do that if needed, but this seems much cleaner as API anyway (note that we already have it for Function, Array, and Object).

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8407681 [details] [diff] [review]
part 1.  Add JS_GetErrorPrototype to JSAPI.

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

OK.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Peter, any chance of reviewing that last bit?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 8407693 [details] [diff] [review]
part 4.  Drop the custom stringifier from DOMException in favor of the default one on Error.prototype.

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

::: dom/webidl/DOMException.webidl
@@ +18,5 @@
>  [NoInterfaceObject]
>  interface ExceptionMembers
>  {
> +  // A custom message set by the thrower.  LenientThis so it can be
> +  // gotten on the prototype.

As discussed, please make a note that this is mainly for stringifying DOMException.prototype.

@@ +24,5 @@
>    readonly attribute DOMString               message;
>    // The nsresult associated with this exception.
>    readonly attribute unsigned long           result;
> +  // The name of the error code (ie, a string repr of |result|).
> +  // LenientThis so it can be gotten on the prototype.

And here.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in http://hg.mozilla.org/integration/mozilla-inbound/rev/46d57b8f48ce along with backing out bug 1001157 for various busted things: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=39073608&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39073379&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=39073678&tree=Mozilla-Inbound

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, so there were three failures, all from this patch.

dom/imptests/html/dom/test_interfaces.html (M2) is an unexpected pass; easy to fix by removing the known-fail annotation.

dom/tests/mochitest/bugs/test_onerror_message.html (M3) is due to Error.prototype now being on the DOMException proto chain.  This causes IsDuckTypedErrorObject to find a property named "fileName" on the object, which means it never looks for "filename" and we lose the filename.  Jason, how would you feel about reversing the order of those two checks?  My other option is to list both properties on DOMException, both returning the same thing...

browser/devtools/webconsole/test/browser_webconsole_output_04.js (dt/dt3) is due to the devtools code doing String(exception) where "exception" is an Xray for a DOMException.  This used to call its toString, but now just returns "[XrayWrapper [object DOMException]" or something, since it doesn't see the content-side toString from Error.prototype.  Bobby, any sane ways of dealing with that short of leaving the toString on the interface (possibly [ChromeOnly])?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> browser/devtools/webconsole/test/browser_webconsole_output_04.js (dt/dt3) is
> due to the devtools code doing String(exception) where "exception" is an
> Xray for a DOMException.  This used to call its toString, but now just
> returns "[XrayWrapper [object DOMException]" or something, since it doesn't
> see the content-side toString from Error.prototype.  Bobby, any sane ways of
> dealing with that short of leaving the toString on the interface (possibly
> [ChromeOnly])?

Can we just fix the test?

Comment 13

[Boris Zbarsky [:bzbarsky]]

It's not just the test.  You get the same behavior difference if you just open the devtools console and type print(foo) where foo is an exception object that was thrown by content.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> It's not just the test.  You get the same behavior difference if you just
> open the devtools console and type print(foo) where foo is an exception
> object that was thrown by content.

Yeah, but is that a problem? I mean, print(document) returns [object XrayWrapper [object HTMLDocument]].

Comment 15

[Boris Zbarsky [:bzbarsky]]

That's a good point.  Seems to me like print() is pretty fundamentally busted?  We shouldn't exposing the XrayWrapper bits to web developers...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Switch the duck typing checks to work with the new DOMException setup

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the devtools test to deal with the no-longer-Xrayable stringifier

Comment 18

[Jason Orendorff [:jorendorff]]

Assuming this doesn't actually needinfo from me anymore.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Let's try this again:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d9d03ecfdf
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a949414f5bc
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bceaeae2f7af
   https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef41f4b806d

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4d9d03ecfdf
https://hg.mozilla.org/mozilla-central/rev/3a949414f5bc
https://hg.mozilla.org/mozilla-central/rev/bceaeae2f7af
https://hg.mozilla.org/mozilla-central/rev/1ef41f4b806d

Comment 21

[Jean-Yves Perrier [:teoli]]

Adding dev-doc-needed because, if I understand well, this change a bit the behavior of DOMException.