[webkit-reviews] review denied: [Bug 27525] Fix error handling in dedicated worker and worker context. : [Attachment 33268] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 27525: Fix error handling in dedicated worker and worker context.
https://bugs.webkit.org/show_bug.cgi?id=27525

Attachment 33268: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=33268&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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 :)