Fix error handling in dedicated worker and worker context.

[Jian Li]

Fix error handling in dedicated worker and worker context to adhere to the Web Worker spec (http://www.whatwg.org/specs/web-workers/current-work/#runtime-script-errors):

  Whenever an uncaught runtime script error occurs in one of the worker's scripts, if the error did not occur while handling a previous script error, the user agent must report the error using the WorkerGlobalScope object's onerror attribute. 

  For dedicated workers, if the error is still not handled afterwards, or if the error occurred while handling a previous script error, the user agent must queue a task to fire a worker error event at the Worker object associated with the worker.

  When the user agent is to fire a worker error event at a Worker object, it must dispatch an event that uses the ErrorEvent interface, with the name error, that doesn't bubble and is cancelable, with its message, filename, and lineno attributes set appropriately. The default action of this event depends on whether the Worker object is itself in a worker. If it is, and that worker is also a dedicated worker, then the user agent must again queue a task to fire a worker error event at the Worker object associated with that worker. Otherwise, then the error should be reported to the user.

[Jian Li]

Created attachment 33268 [details]
Proposed Patch

[David Levin]

Comment on attachment 33268 [details]
Proposed Patch

Thanks for taking this on.  It is a pretty complicated area as such I have quite a few comments some of which may just require educating me :)


> diff --git a/LayoutTests/fast/workers/resources/worker-script-error-not-handled1.js b/LayoutTests/fast/workers/resources/worker-script-error-not-handled1.js

What do you think about
   /LayoutTests/fast/workers/resources/worker-script-error-unhandled.js
?

(Just an idea)


> diff --git a/LayoutTests/fast/workers/resources/worker-script-error-not-handled2.js b/LayoutTests/fast/workers/resources/worker-script-error-not-handled2.js
> @@ -0,0 +1,7 @@
> +onerror = function(message, url, lineno)
> +{
> +    postMessage("onerror invoked for a script that has script error '" + message + "' at line " + lineno);
> +    return true;
> +}
> +
> +foo.bar = 0;

This is handled in some sense but allowed to bubble.  What do you think about 
  LayoutTests/fast/workers/resources/worker-script-error-bubbled.js
?


> diff --git a/LayoutTests/fast/workers/worker-constructor.html b/LayoutTests/fast/workers/worker-constructor.html
...
> +
> +function test10()
> +{
> +    try {
> +        var worker = new Worker("resources/worker-script-error-not-handled2.js");
> +        worker.onerror = function(evt) {
> +            log("PASS: onerror invoked for a script that has script error '" + evt.message + "' at line " + evt.lineno + ".");
> +            runNextTest();
> +        }

Why doesn't this test capture and log the message from "worker-script-error-not-handled2.js"?

> +function test11()

Need to add a test that cancels the event received on the worker object.

I see some advantages of this format (test#), but now that there are 11 tests with this format, I find it a bit less readable.  Here's why:

1. The tests end up with non descript names, and it is hard for me to quickly tell what unique thing each is trying to test. Instead I'd recommend getting rid of runNextTest and just calling each test from the previous tests.  It is not much more fragile (one must check that each test calls the next and that the last test logs DONE and calls layoutTestController.notifyDone();).

2. It feels like it led to more tests to this same file. It would be good if the new tests were in a new file based on what they are testing.  The new tests are about script errors and on error handling.  They should be in a test with a name that represents that as opposed to "worker-constructor.html"



> diff --git a/WebCore/bindings/js/JSEventListener.cpp b/WebCore/bindings/js/JSEventListener.cpp
> +bool JSEventListener::reportError(const String& message, const String& url, int lineNumber)
...
> +
> +    globalObject->globalData()->timeoutChecker.start();
> +    JSValue retval = call(exec, jsFunction, callType, callData, thisValue, args);

retvalue -> returnValue?

> +    globalObject->globalData()->timeoutChecker.stop();
> +
> +    if (exec->hadException()) {
> +        reportCurrentException(exec);

Does this do the following: 
 "if the error occurred while handling a previous script error, the user agent must queue a task to fire a worker error event at the Worker object associated with the worker."
?

If not, why not?

> +        return true;
> +    }
> +    
> +    bool retvalbool;

How about bubbleEvent (instead of retvalbool)?

> +    return retval.getBoolean(retvalbool) && !retvalbool;


> diff --git a/WebCore/bindings/js/JSEventListener.h b/WebCore/bindings/js/JSEventListener.h
> +        virtual bool reportError(const String& message, const String& url, int);

It would be helpful to fill in the param name for the int.

> diff --git a/WebCore/dom/EventListener.h b/WebCore/dom/EventListener.h
> +        // Return true to indicate that the error is handled.
> +        virtual bool reportError(const String& /*message*/, const String& /*url*/, int) { return false; }

It would be helpful to fill in the param name for the int.


> diff --git a/WebCore/workers/WorkerContext.cpp b/WebCore/workers/WorkerContext.cpp
>  void WorkerContext::reportException(const String& errorMessage, int lineNumber, const String& sourceURL)
>  {
> -    m_thread->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);
> +    bool errorHandled = false;
> +    if (m_onerrorListener)
> +        errorHandled = m_onerrorListener->reportError(errorMessage, sourceURL, lineNumber);
> +
> +    if (!errorHandled)
> +        m_thread->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);

For Document::reportException, only logs the message to the console
    if (DOMWindow* window = domWindow())
        window->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, errorMessage, lineNumber, sourceURL);

So when done in a Document callers of ScriptExecutionContext::reportException are simply doing this.  So far in workers, the
message was simply proxies to the Document which did its normal thing. 

Now this calls the on error listener first.  Who calls this api in workers and is this the correct thing to do for all of them? (Seems likely but it would be good to track this down and be sure.  Then can we add tests for the error handling for them to verify that the error is being reported so your fix does get regressed?)


Does every place that called this want to do the dispatch script error event?


> diff --git a/WebCore/workers/WorkerMessagingProxy.cpp b/WebCore/workers/WorkerMessagingProxy.cpp
>      virtual void performTask(ScriptExecutionContext* context)
>      {
> -        if (!m_messagingProxy->askedToTerminate())
> +        Worker* workerObject = m_messagingProxy->workerObject();
> +        if (!workerObject || m_messagingProxy->askedToTerminate())
> +            return;
> +
> +        if (workerObject->onerror())
> +            workerObject->dispatchScriptErrorEvent(m_errorMessage, m_sourceURL, m_lineNumber);
> +        else
>              context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
According to the web workers spec:

"When the user agent is to fire a worker error event at a Worker object, it must dispatch an event that uses the ErrorEvent interface, with the name error, that doesn't bubble and is cancelable, with its message, filename, and lineno attributes set appropriately. The default action of this event depends on whether the Worker object is itself in a worker. If it is, and that worker is also a dedicated worker, then the user agent must again queue a task to fire a worker error event at the Worker object associated with that worker. Otherwise, then the error should be reported to the user."

It sounds like this event should go to ScriptExecutionContext::reportException ("reported to the user") unless it was cancelled.

Thanks again for this and for dealing with my volume of comments here :)

[Jian Li]

Created attachment 33474 [details]
Proposed Patch

[David Levin]

Comment on attachment 33474 [details]
Proposed Patch

Just found out that Jian is doing a few more changes to this , so canceling the review request for him.

[Jian Li]

Created attachment 33475 [details]
Proposed Patch

[Jian Li]

(In reply to comment #2)
> (From update of attachment 33268 [details])
> Thanks for taking this on.  It is a pretty complicated area as such I have
> quite a few comments some of which may just require educating me :)
> 
> 
> > diff --git a/LayoutTests/fast/workers/resources/worker-script-error-not-handled1.js b/LayoutTests/fast/workers/resources/worker-script-error-not-handled1.js
> 
> What do you think about
>    /LayoutTests/fast/workers/resources/worker-script-error-unhandled.js
> ?
> 
> (Just an idea)
> 
Done.
> 
> > diff --git a/LayoutTests/fast/workers/resources/worker-script-error-not-handled2.js b/LayoutTests/fast/workers/resources/worker-script-error-not-handled2.js
> > @@ -0,0 +1,7 @@
> > +onerror = function(message, url, lineno)
> > +{
> > +    postMessage("onerror invoked for a script that has script error '" + message + "' at line " + lineno);
> > +    return true;
> > +}
> > +
> > +foo.bar = 0;
> 
> This is handled in some sense but allowed to bubble.  What do you think about 
>   LayoutTests/fast/workers/resources/worker-script-error-bubbled.js
> ?
Done.
> 
> 
> > diff --git a/LayoutTests/fast/workers/worker-constructor.html b/LayoutTests/fast/workers/worker-constructor.html
> ...
> > +
> > +function test10()
> > +{
> > +    try {
> > +        var worker = new Worker("resources/worker-script-error-not-handled2.js");
> > +        worker.onerror = function(evt) {
> > +            log("PASS: onerror invoked for a script that has script error '" + evt.message + "' at line " + evt.lineno + ".");
> > +            runNextTest();
> > +        }
> 
> Why doesn't this test capture and log the message from
> "worker-script-error-not-handled2.js"?
> 
Fixed.
> > +function test11()
> 
> Need to add a test that cancels the event received on the worker object.
> 
> I see some advantages of this format (test#), but now that there are 11 tests
> with this format, I find it a bit less readable.  Here's why:
> 
> 1. The tests end up with non descript names, and it is hard for me to quickly
> tell what unique thing each is trying to test. Instead I'd recommend getting
> rid of runNextTest and just calling each test from the previous tests.  It is
> not much more fragile (one must check that each test calls the next and that
> the last test logs DONE and calls layoutTestController.notifyDone();).
> 
> 2. It feels like it led to more tests to this same file. It would be good if
> the new tests were in a new file based on what they are testing.  The new tests
> are about script errors and on error handling.  They should be in a test with a
> name that represents that as opposed to "worker-constructor.html"
> 
Changed to maintain a list of meaningful function names, as we discussed.
> 
> 
> > diff --git a/WebCore/bindings/js/JSEventListener.cpp b/WebCore/bindings/js/JSEventListener.cpp
> > +bool JSEventListener::reportError(const String& message, const String& url, int lineNumber)
> ...
> > +
> > +    globalObject->globalData()->timeoutChecker.start();
> > +    JSValue retval = call(exec, jsFunction, callType, callData, thisValue, args);
> 
> retvalue -> returnValue?
Done.
> 
> > +    globalObject->globalData()->timeoutChecker.stop();
> > +
> > +    if (exec->hadException()) {
> > +        reportCurrentException(exec);
> 
> Does this do the following: 
>  "if the error occurred while handling a previous script error, the user agent
> must queue a task to fire a worker error event at the Worker object associated
> with the worker."
> ?
> 
> If not, why not?
Fixed. Also added one more test case for it.
> 
> > +        return true;
> > +    }
> > +    
> > +    bool retvalbool;
> 
> How about bubbleEvent (instead of retvalbool)?
> 
Done.
> > +    return retval.getBoolean(retvalbool) && !retvalbool;
> 
> 
> > diff --git a/WebCore/bindings/js/JSEventListener.h b/WebCore/bindings/js/JSEventListener.h
> > +        virtual bool reportError(const String& message, const String& url, int);
> 
> It would be helpful to fill in the param name for the int.
> 
Done.
> > diff --git a/WebCore/dom/EventListener.h b/WebCore/dom/EventListener.h
> > +        // Return true to indicate that the error is handled.
> > +        virtual bool reportError(const String& /*message*/, const String& /*url*/, int) { return false; }
> 
> It would be helpful to fill in the param name for the int.
> 
Done.
> 
> > diff --git a/WebCore/workers/WorkerContext.cpp b/WebCore/workers/WorkerContext.cpp
> >  void WorkerContext::reportException(const String& errorMessage, int lineNumber, const String& sourceURL)
> >  {
> > -    m_thread->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);
> > +    bool errorHandled = false;
> > +    if (m_onerrorListener)
> > +        errorHandled = m_onerrorListener->reportError(errorMessage, sourceURL, lineNumber);
> > +
> > +    if (!errorHandled)
> > +        m_thread->workerObjectProxy().postExceptionToWorkerObject(errorMessage, lineNumber, sourceURL);
> 
> For Document::reportException, only logs the message to the console
>     if (DOMWindow* window = domWindow())
>         window->console()->addMessage(JSMessageSource, LogMessageType,
> ErrorMessageLevel, errorMessage, lineNumber, sourceURL);
> 
> So when done in a Document callers of ScriptExecutionContext::reportException
> are simply doing this.  So far in workers, the
> message was simply proxies to the Document which did its normal thing. 
> 
> Now this calls the on error listener first.  Who calls this api in workers and
> is this the correct thing to do for all of them? (Seems likely but it would be
> good to track this down and be sure.  Then can we add tests for the error
> handling for them to verify that the error is being reported so your fix does
> get regressed?)
> 
WorkerScriptController::evaluate().
> 
> Does every place that called this want to do the dispatch script error event?
> 
Yes.
> 
> > diff --git a/WebCore/workers/WorkerMessagingProxy.cpp b/WebCore/workers/WorkerMessagingProxy.cpp
> >      virtual void performTask(ScriptExecutionContext* context)
> >      {
> > -        if (!m_messagingProxy->askedToTerminate())
> > +        Worker* workerObject = m_messagingProxy->workerObject();
> > +        if (!workerObject || m_messagingProxy->askedToTerminate())
> > +            return;
> > +
> > +        if (workerObject->onerror())
> > +            workerObject->dispatchScriptErrorEvent(m_errorMessage, m_sourceURL, m_lineNumber);
> > +        else
> >              context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
> According to the web workers spec:
> 
> "When the user agent is to fire a worker error event at a Worker object, it
> must dispatch an event that uses the ErrorEvent interface, with the name error,
> that doesn't bubble and is cancelable, with its message, filename, and lineno
> attributes set appropriately. The default action of this event depends on
> whether the Worker object is itself in a worker. If it is, and that worker is
> also a dedicated worker, then the user agent must again queue a task to fire a
> worker error event at the Worker object associated with that worker. Otherwise,
> then the error should be reported to the user."
> 
> It sounds like this event should go to ScriptExecutionContext::reportException
> ("reported to the user") unless it was cancelled.
> 
Fixed.

> Thanks again for this and for dealing with my volume of comments here :)

[David Levin]

Comment on attachment 33475 [details]
Proposed Patch

Thanks Jian!

[Jian Li]

Committed as http://trac.webkit.org/changeset/46419