Closed Bug 1251518 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 4 obsolete files)

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")
Flags: needinfo?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8724144 - Attachment is obsolete: true
Attachment #8724144 - Flags: review?(bobbyholley)
Attachment #8724150 - Attachment is obsolete: true
Attachment #8724150 - Flags: review?(bobbyholley)
Attachment #8724170 - Attachment is obsolete: true
Attachment #8724170 - Flags: review?(terrence)
Attachment #8724170 - Flags: review?(bobbyholley)
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.
Attachment #8724177 - Flags: review?(terrence) → review+
Whiteboard: btpp-active
Note that the upshot is to align the error events workers fire with the error events mainthread fires for equivalent exceptions....
Attachment #8724208 - Flags: review?(bobbyholley)
Attachment #8724177 - Attachment is obsolete: true
Attachment #8724177 - Flags: review?(bobbyholley)
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.
Attachment #8724208 - Flags: review?(bobbyholley) → review+
Flags: needinfo?(bobbyholley)
> 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).
https://hg.mozilla.org/mozilla-central/rev/923325f2c566
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.