908699 - Generate parse errors and warnings when off the main thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch (ed4429af6905)

Currently, when parsing off the main thread the parse either succeeds or fails, with no reports generated for the user.  This is fine for XUL documents, which ignore errors in the scripts they parse, but we should report parse warnings and errors correctly when handling scripts in HTML documents.  The attached patch records any reports made when parsing off thread, which are then generated when the script is finished back on the main thread.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 794715 [details] [diff] [review]
patch (ed4429af6905)

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

Everything in JS looks good, but I think there's something wrong in the nsXULElement.cpp changes (noted below). Can you post a new patch (or correct me if I'm wrong)? If the code for handling the errors is at all complicated, maybe bz should review it.

::: content/xul/content/src/nsXULElement.cpp
@@ +2574,5 @@
>      NS_ENSURE_TRUE(svc, NS_ERROR_FAILURE);
>      JSRuntime *rt;
>      svc->GetRuntime(&rt);
>      NS_ENSURE_TRUE(svc, NS_ERROR_FAILURE);
> +    JSScript *script = JS::FinishOffThreadScript(rt, mToken);

Does this compile? I thought FinishOffThreadScript was supposed to take a cx so that errors can be reported.

::: js/src/jsapi.h
@@ +4090,5 @@
> + * for the compilation. The callback will be invoked while off the main thread,
> + * so must ensure that its operations are thread safe. Afterwards,
> + * FinishOffThreadScript must be invoked on the main thread to get the result
> + * script or NULL and report any error or warnings generated during the parse
> + * (if maybecx is specified).

Could you put the last part ("report any error...") in a separate sentence? I think it would be a little clearer that way.

::: js/src/jscntxt.h
@@ +106,5 @@
>  class RegExpStatics;
>  class ForkJoinSlice;
>  
> +namespace frontend {
> +    struct CompileError;

This shouldn't be indented.

::: js/src/jsexn.cpp
@@ +856,5 @@
>      return errorProto;
>  }
>  
>  const JSErrorFormatString*
> +js_GetLocalizedErrorMessage(ExclusiveContext* cx, void *userRef, const char *locale,

Please move the * over.

::: js/src/jsexn.h
@@ +53,5 @@
>  extern JSErrorReport *
>  js_ErrorFromException(jsval exn);
>  
>  extern const JSErrorFormatString *
> +js_GetLocalizedErrorMessage(js::ExclusiveContext* cx, void *userRef, const char *locale,

Please move the * over.

::: js/src/jsstr.cpp
@@ +4089,5 @@
>      return true;
>  }
>  
>  bool
> +js::InflateStringToBuffer(ExclusiveContext *maybecx, const char *src, size_t srclen,

It looks like you changed this because it's called by StringBuffer::appendInflated. However, that function statically guarantees that the buffer is big enough, so we'll never fail here. Therefore, I think you can pass in NULL for maybecx and just not change anything here. (Also, this function is really weird and we should fix it, but that's neither here nor there.)

::: js/src/jsworkers.cpp
@@ +493,2 @@
>  {
>      ParseTask *parseTask = NULL;

Could we use a ScopedJSDeletePtr here and then remove the js_deletes?

@@ +522,5 @@
>          // Parsing failed and there is nothing to finish, but there still may
>          // be lingering ParseTask instances holding roots which need to be
>          // cleaned up. The ParseTask which we picked might not be the right
>          // one but this is ok as finish calls will be 1:1 with calls that
>          // create a ParseTask.

The last part of the comment is no longer true.

@@ +524,5 @@
>          // cleaned up. The ParseTask which we picked might not be the right
>          // one but this is ok as finish calls will be 1:1 with calls that
>          // create a ParseTask.
>          js_delete(parseTask);
> +        return NULL;

Is it possible to assert that there were errors?

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #1)
> Everything in JS looks good, but I think there's something wrong in the
> nsXULElement.cpp changes (noted below). Can you post a new patch (or correct
> me if I'm wrong)? If the code for handling the errors is at all complicated,
> maybe bz should review it.

Oops, the cx passed in should be NULL.  XUL documents don't report parse errors or warnings in scripts anywhere, as far as I can tell.

Comment 3

[Tom S [:evilpie]]

I think that was actually fixed quite recently in Bug 898811.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hrm.  We _should_ be reporting parse errors for this stuff, even if we're not right now.  Bobby?

Comment 5

[Boris Zbarsky [:bzbarsky]]

In particular, seems like we used the nsIScriptContext's error reporter until bug 901106 made us use some random safe context which doesn't do error reporting?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> In particular, seems like we used the nsIScriptContext's error reporter

The nsIScriptContext associated with the compilation scope, sure. But there was never an associated DOM window.

> until bug 901106 made us use some random safe context

There is only one safe JSContext in gecko. I don't think the "some random" modifier is accurate.

> which doesn't do error reporting?

It reports as much as JSMs or any other system JS. There's no DOM window, sure, but as noted above, there wasn't one before, either.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> It reports as much as JSMs or any other system JS.

As in, to the error console?

Is comment 2 just wrong then?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> > It reports as much as JSMs or any other system JS.
> 
> As in, to the error console?
> 
> Is comment 2 just wrong then?

It might have been true in the old world, because we never set up an error reporter on the compilation scope's nsIScriptContext. But with the SafeJSContext we'll just get uniform treatment everywhere.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: patch (ca06d27f049f)

Updated patch that uses AutoSafeJSContext, per IRC discussion.

Comment 10

[Brian Hackett [Laid off!]]

I actually had to remove the AutoSafeJSContext thing, as its mere presence in NotifyOffThreadScriptCompletedRunnable::Run causes some mochitests to time out, such as toolkit/mozapps/extensions/test/browser/browser_bug562797.js.  This seems like an issue with AutoSafeJSContext unrelated to the error reporting itself, as getting the context and merely getting its runtime will also cause the test to time out.  I'll file a followup about this, and after that's fixed error reporting can be turned on for XUL scripts parsed off thread.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f45469113804

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f45469113804