Bug 25151 – workers that fail to load scripts not firing error event

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[jason a]

worker js files with syntax errors do not throw exception when worker is created or message is posted to worker.

Expected: exception during construction and/or onerror when message is posted.

[Alexey Proskuryakov]

Exception during construction is definitely not possible - the constructor just starts a network load, and returns before the script is available.

What does the spec say about this situation?

[Dmitry Titov]

Here is what the spec says:

---
Attempt to fetch the resource identified by url.
If the attempt fails, or if the attempt involves any redirects to URIs that do not have the same origin as url (even if the final URI is at the same origin as the original url), then for each Worker or SharedWorker object associated with worker global scope, queue a task to fire a simple event called error at that object. 
---

There is also a description on how exactly to fire this event.

[Jian Li]

Created attachment 32493 [details]
Proposed Patch

[David Levin]

Comment on attachment 32493 [details]
Proposed Patch

Just a few things to address/clarify.  Thanks.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 25151 - workers with syntax errors not firing error event.
> +        https://bugs.webkit.org/show_bug.cgi?id=25151
> +
> +        This fixes the problem that an error event is not fired when failed

Consider this wording:

  This fixes the problem that an error event is not fired when the worker script
  fails to load.  Some reasons this may occur are an invalid URL for the worker 
  script, a cross-origin redirect, or a syntax error in the script.


Another problem is this changelog doesn't say much about why things were done.  When I
first looked at the patch I thought another issue was being fixed but some items were moved
around.  Function level comments in the changelog would be helpful here.

> diff --git a/WebCore/bindings/js/JSWorkerConstructor.cpp b/WebCore/bindings/js/JSWorkerConstructor.cpp
> -    ExceptionCode ec = 0;
> -    RefPtr<Worker> worker = Worker::create(scriptURL, window->document(), ec);
> -    setDOMException(exec, ec);
> +    RefPtr<Worker> worker = Worker::create(scriptURL, window->document());

Why is it ok to stop setting the dom exception?


> diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> -    ExceptionCode ec = 0;
> -    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl), context, ec);
> -
> -    if (ec)
> -        return throwError(ec);
> +    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl), context);

Same question as above (but here it is throwError).

> diff --git a/WebCore/workers/WorkerMessagingProxy.cpp b/WebCore/workers/WorkerMessagingProxy.cpp

>      virtual void performTask(ScriptExecutionContext* context)
>      {
> -        if (!m_messagingProxy->askedToTerminate())
> -            context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
> +        Worker* workerObject = m_messagingProxy->workerObject();
> +        if (!workerObject || m_messagingProxy->askedToTerminate())
> +            return;
> +
> +        context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
> +        workerObject->dispatchErrorEvent();

This changes the behavior for more than just the case that you're fixing.  Any code that calls ScriptExecutionContext->reportException will get this behavior.  Why is that the correct thing to do?

Also, is there a chromium change corresponding to this?

> diff --git a/WebCore/workers/WorkerMessagingProxy.h b/WebCore/workers/WorkerMessagingProxy.h
> namespace WebCore {
...
> +        friend class WorkerExceptionTask;

Why is this being added here?

> diff --git a/WebCore/workers/WorkerScriptLoader.cpp b/WebCore/workers/WorkerScriptLoader.cpp
> +void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)
...
> +    CrossOriginRedirectPolicy crossOriginRedirectPolicy = (crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect : AllowCrossOriginRedirect;
    
> +void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy, WorkerScriptLoaderClient* client)
...
> +    CrossOriginRedirectPolicy crossOriginRedirectPolicy = (crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect : AllowCrossOriginRedirect;

It would be nice to only have one place that does the conversion of CrossOriginLoadPolicy to CrossOriginRedirectPolicy.  Right now there are two in this file.

> +ResourceRequest* WorkerScriptLoader::createResourceRequest(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)

This should return a PassOwnPtr<ResourceRequest>

[Jian Li]

(In reply to comment #4)
> (From update of attachment 32493 [details])
> Just a few things to address/clarify.  Thanks.
> 
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > [phone]  Jian Li  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Bug 25151 - workers with syntax errors not firing error event.
> > +        https://bugs.webkit.org/show_bug.cgi?id=25151
> > +
> > +        This fixes the problem that an error event is not fired when failed
> 
> Consider this wording:
> 
>   This fixes the problem that an error event is not fired when the worker
> script
>   fails to load.  Some reasons this may occur are an invalid URL for the worker 
>   script, a cross-origin redirect, or a syntax error in the script.
> 
> 
> Another problem is this changelog doesn't say much about why things were done. 
> When I
> first looked at the patch I thought another issue was being fixed but some
> items were moved
> around.  Function level comments in the changelog would be helpful here.
> 
Updated.

> > diff --git a/WebCore/bindings/js/JSWorkerConstructor.cpp b/WebCore/bindings/js/JSWorkerConstructor.cpp
> > -    ExceptionCode ec = 0;
> > -    RefPtr<Worker> worker = Worker::create(scriptURL, window->document(), ec);
> > -    setDOMException(exec, ec);
> > +    RefPtr<Worker> worker = Worker::create(scriptURL, window->document());
> 
> Why is it ok to stop setting the dom exception?
> 
This is because the worker constructor will never return an exception code to indicate a DOM exception.
> 
> > diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> > -    ExceptionCode ec = 0;
> > -    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl), context, ec);
> > -
> > -    if (ec)
> > -        return throwError(ec);
> > +    RefPtr<Worker> obj = Worker::create(toWebCoreString(scriptUrl), context);
> 
> Same question as above (but here it is throwError).

This is because the worker constructor will never return an exception code to indicate a DOM exception.
> 
> > diff --git a/WebCore/workers/WorkerMessagingProxy.cpp b/WebCore/workers/WorkerMessagingProxy.cpp
> 
> >      virtual void performTask(ScriptExecutionContext* context)
> >      {
> > -        if (!m_messagingProxy->askedToTerminate())
> > -            context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
> > +        Worker* workerObject = m_messagingProxy->workerObject();
> > +        if (!workerObject || m_messagingProxy->askedToTerminate())
> > +            return;
> > +
> > +        context->reportException(m_errorMessage, m_lineNumber, m_sourceURL);
> > +        workerObject->dispatchErrorEvent();
> 
> This changes the behavior for more than just the case that you're fixing.  Any
> code that calls ScriptExecutionContext->reportException will get this behavior.
>  Why is that the correct thing to do?
> 
This indeed fixes a bug here. Previously we only called "context->reportException(...)" that only reported an exception to the ScriptExecutionContext that put the error message in the console. We need to also call onerror event handler of the worker object.

> Also, is there a chromium change corresponding to this?
> 
Yes, I will have another patch for corresponding chromium change for review after I land this.

> > diff --git a/WebCore/workers/WorkerMessagingProxy.h b/WebCore/workers/WorkerMessagingProxy.h
> > namespace WebCore {
> ...
> > +        friend class WorkerExceptionTask;
> 
> Why is this being added here?

This is because WorkerExceptionTask::performTask needs to access private function WorkerMessagingProxy::workerObject.
> 
> > diff --git a/WebCore/workers/WorkerScriptLoader.cpp b/WebCore/workers/WorkerScriptLoader.cpp
> > +void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)
> ...
> > +    CrossOriginRedirectPolicy crossOriginRedirectPolicy = (crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect : AllowCrossOriginRedirect;
> 
> > +void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy, WorkerScriptLoaderClient* client)
> ...
> > +    CrossOriginRedirectPolicy crossOriginRedirectPolicy = (crossOriginLoadPolicy == DenyCrossOriginLoad) ? DenyCrossOriginRedirect : AllowCrossOriginRedirect;
> 
> It would be nice to only have one place that does the conversion of
> CrossOriginLoadPolicy to CrossOriginRedirectPolicy.  Right now there are two in
> this file.

Fixed.
> 
> > +ResourceRequest* WorkerScriptLoader::createResourceRequest(ScriptExecutionContext* scriptExecutionContext, const String& url, URLCompletionPolicy urlCompletionPolicy, CrossOriginLoadPolicy crossOriginLoadPolicy)
> 
> This should return a PassOwnPtr<ResourceRequest>

Fixed.

[Jian Li]

Created attachment 32747 [details]
Proposed Patch

[Jian Li]

Created attachment 32749 [details]
Proposed Patch

[Jian Li]

Created attachment 32814 [details]
Proposed Patch

Uploaded a new pacth per discussion with levin. Removed the handling of syntax error in the script from this patch.

[David Levin]

Comment on attachment 32814 [details]
Proposed Patch

Looks good.

Please file a bug about the error not being fired on WorkerContext when there is a syntax error in the script.

[Jian Li]

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