1251518 - Workers should stop using js::ErrorReportToString; it's fundamentally broken

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bobby, you added this in bug 872273 looks like.  Is there a reason you didn't do what we do on the main thread?

The basic problem is that if the exception is not a JS Error subtype (e.g. is a DOMException), js::ErrorReportToString will mess up: it will just use the reportp->ucmessage with ": " prepended.  But in these cases reportp->ucmessage already includes the name and message of the exception.  See what ErrorReport::init does around the "If we have the right fields" comment and the "Note that using |str| for |ucmessage| here is kind of wrong" comment.

In contrast, what mainthread does (in xpc::ErrorReport::Init) is reimplement this stuff correctly: it uses aReport->ucmessage and only prepends the error name and ": " to it if aReport->exnType is something the JS engine knows about.  And if there is no ucmessage in the JSErrorReport, it falls back to the provided fallback message...

We should certainly do the fallback thing.  But it seems like we should also either fix js::ErrorReportToString (e.g. to either use js::GetErrorTypeName like XPConnect currently does or at least to be a bit smarter in the JEXN_NONE case, though might be nice to do the right thing for JSEXN_INTERNALERR as well) or stop using it.

You can see the very ugly we have now if you just have a worker whose first line is:

  throw new DOMException("hello", "NetworkError")

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix js::ErrorReportToString to make more sense and fix its consumers to not use it if the error report has no message

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix js::ErrorReportToString to make more sense and fix its consumers to not use it if the error report has no message

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix js::ErrorReportToString to make a bit more sense, and change worker code to not use it anyway, so it matches the mainthread code

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix js::ErrorReportToString to make a bit more sense, and change worker code to not use it anyway, so it matches the mainthread code

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8724177 [details] [diff] [review]
Fix js::ErrorReportToString to make a bit more sense, and change worker code to not use it anyway, so it matches the mainthread code

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

Works for me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with more test changes

Note that the upshot is to align the error events workers fire with the error events mainthread fires for equivalent exceptions....

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8724208 [details] [diff] [review]
Now with more test changes

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

::: dom/workers/WorkerPrivate.cpp
@@ +5748,5 @@
>    JSExnType exnType = JSEXN_ERR;
>    bool mutedError = aReport && aReport->isMuted;
>  
>    if (aReport) {
> +    // We don't want to use js::ErrorReportToString here, for three reasons:

I think we should stop exposing this from jsfriendapi entirely, and make it static to jsexn.cpp, since that seems to be the only remaining caller. That also means that we don't need to justify not using it here, but rather just comment that we're aligning behavior with xpc::ErrorReport::init.

@@ +5765,5 @@
>        }
> +      message.Append(m);
> +    }
> +
> +    if (message.IsEmpty() && aMessage) {

Is this a fallback message? If so, maybe rename it to aFallbackMessage like it is in the xpc case?

::: js/src/jsexn.cpp
@@ +643,5 @@
> +    /*
> +     * We do NOT want to use GetErrorTypeName() here because it will not do the
> +     * "right thing" for JSEXN_INTERNALERR.  That is, some consumers of this API
> +     * expect that "InternalError: " will be prepended but GetErrorTypeName goes
> +     * out of its way to avoid this.

Again, I'd rather dispense with the 'some consumers' formalism and just make this static.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +177,5 @@
>                            : NS_LITERAL_CSTRING("content javascript");
>      mWindowID = aWindowID;
>  
> +    // We can't use js::ErrorReportToString here because we don't have a
> +    // JSContext.

Same here. Though maybe this means we should actually make a shared helper for this somewhere? Either in jsfriendapi or somewhere in Gecko is fine by me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I think we should stop exposing this from jsfriendapi entirely

Done.

> Is this a fallback message? If so, maybe rename it to aFallbackMessage like it is in the xpc case?

Yes, and done.  Also, I took that block out, because after that big if we already have:

  if (message.IsEmpty()) {
    message = NS_ConvertUTF8toUTF16(aMessage);
  }

> Again, I'd rather dispense with the 'some consumers' formalism and just make this static.

Done.  Comment now says:

     * We do NOT want to use GetErrorTypeName() here because it will not do the
     * "right thing" for JSEXN_INTERNALERR.  That is, the caller of this API
     * expects that "InternalError: " will be prepended but GetErrorTypeName
     * goes out of its way to avoid this.

> Though maybe this means we should actually make a shared helper for this somewhere?

Doing this in jsfriendapi is not viable because we want to produce XPCOM strings.  I added a static xpc::ErrorReport::ErrorReportToMessageString(JSErrorReport* aReport, nsAString& aString).

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/923325f2c566

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/923325f2c566