Bug 27770 – Workers need to throw an exception when presented with invalid URLs

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

Currently, if the Worker constructor is passed an invalid URL (i.e. empty string, absolute URL pointing at a different domain) the spec requires that the constructor throw an exception.

We are generating an error event instead. We should address this by passing an ExceptionCode through into WorkerScriptLoader::createResourceRequest().

[Jian Li]

Created attachment 33751 [details]
Proposed Patch

[Darin Adler]

Comment on attachment 33751 [details]
Proposed Patch

> -    RefPtr<Worker> worker = Worker::create(scriptURL, window->document());
> +    ExceptionCode ec = 0;
> +    RefPtr<Worker> worker = Worker::create(scriptURL, window->document(), ec);
> +    setDOMException(exec, ec);

I believe you should have a return 0 statement here. You don't want to call toJS on the result of the function after doing setDOMException.

[Jian Li]

Created attachment 33754 [details]
Proposed Patch

Fixed the problem pointed out by Darin.

[Darin Adler]

Comment on attachment 33754 [details]
Proposed Patch

> +    KURL scriptURL = context->completeURL(url);
> +    if (url.isEmpty() || !scriptURL.isValid()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Seems like the url.isEmpty() check could go before the call to completeURL.

> -        void loadSynchronously(ScriptExecutionContext*, const String& url, URLCompletionPolicy, CrossOriginLoadPolicy);
> -        void loadAsynchronously(ScriptExecutionContext*, const String& url, URLCompletionPolicy, CrossOriginLoadPolicy, WorkerScriptLoaderClient*);
> +        void loadSynchronously(ScriptExecutionContext*, const KURL& url, CrossOriginRedirectPolicy);
> +        void loadAsynchronously(ScriptExecutionContext*, const KURL& url, CrossOriginRedirectPolicy, WorkerScriptLoaderClient*);

Normally we would omit a name like "url" when the type makes it clear what it is. So in addition to changing it from String to KURL, you could remove the argument name here.

r=me

[Jian Li]

All fixed and committed as http://trac.webkit.org/changeset/46570.