Bug 24043 – [GTK] Workers with url 'about:blank' do not load

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

This problem can be seen in fast/events/dispatchEvent-crash.html[1] test that is timing out on the Gtk buildbot. In the test, Worker is given a 'about:blank' URL.

Normally, when loading about:blank through MainResourceLoader, we catch blank URLs and prevent loading them, i.e. we finish loading and do not proceed with actions, which otherwise at some point end up in ResourceHandle::start.
Worker uses Loader and ResourceLoader as its loaders, which do not check for blank URLs. Therefor, we do get to ResourceHandle::start. From there on, it seems that it depends on the network backend how about:blank is handled, but that should not be the case.

Therefor, while the test is only failing for the Gtk port, I believe fix can be applied directly into WebCore, as I tend to blame network backends of non-Gtk ports being able to properly handle about:blank (which I only assume) while LibSoup fails to do so.

As a fix, shouldLoadAsEmptyDocument() should be made available for other loaders and properly planted into the Worker's loaders.

[1]: http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/dispatchEvent-crash.html

[Zan Dobersek]

Created attachment 27820 [details]
Check URL prior to loading it in SubresourceLoader

Moved shouldLoadAsEmptyDocument to a better spot.
Planted a check if request's URL should load as an empty document.

With patch applied, the test causing regression successfully passes.

[Alexey Proskuryakov]

Comment on attachment 27820 [details]
Check URL prior to loading it in SubresourceLoader

I don't think that this is correct - trying to load about:blank as a non-main resource should result in a failure due to unknown protocol in the loader (just like any foo:bar URL would). There is no reason to special-case "about:".

Why does this test time out? It doesn't use layoutTestController.waitUntilDone(), and it doesn't depend on the results of loading the worker script in any way that I could see.

[Zan Dobersek]

Created attachment 27830 [details]
Report errors through an idle function when faced unsupported URL protocols

Catches URLs with unsupported protocol and adds idle function to the main loop that reports error to resource handle's client.

In Loader::Host::servePendingRequests, a subresource loader is created for every pending request with SubresourceLoader::create, but not yet added to the map of loaders with requests that Loader::Host holds.
In this function, the return value of ResourceLoader::load is subsequently checked, which causes to create a resource handle with ResourceHandle::create, which also calls ResourceHandle::start. There, we check the protocol of request's URL. In the case of bogus URLs, such as about:blank or foo:bar, we immediately call didFail on the handle's client. This results in premature return in Loader::Host::didFail and therefor we never get to call error on resource of the request because the latter hasn't yet been added to the map of loaders with requests.

To fix this, instead of immediately calling didFail on an URL with unknown protocol, we add an idle function into the main loop. We also must return true, otherwise there is no handle returned in ResourceHandle::create. This lets things to move on - loader and request are added to the map, idle function is executed and failure is reported, from there the error to the cached resource is announced, which brings worker to the hold with calling notifyFinished.

With patch, test passes with no new regressions.

[Zan Dobersek]

Comment on attachment 27830 [details]
Report errors through an idle function when faced unsupported URL protocols

Forgot to set the review flag the first time.

[Alexey Proskuryakov]

Comment on attachment 27830 [details]
Report errors through an idle function when faced unsupported URL protocols

What happens if the request is canceled while waiting for the idle handler to fire?

[Zan Dobersek]

(In reply to comment #5)
> What happens if the request is canceled while waiting for the idle handler to
> fire?

Utter failure :)
Will post a patch with checks in near future.

[Zan Dobersek]

Created attachment 27841 [details]
Report errors through an idle function when faced unsupported URL protocols

Added checks if client exists and the request is not canceled.

[Zan Dobersek]

Created attachment 27844 [details]
Report errors through an idle function when faced unsupported URL protocols

Also checks for existing handle, properly uses ResourceHandleInternal.

[Alexey Proskuryakov]

Comment on attachment 27844 [details]
Report errors through an idle function when faced unsupported URL protocols

> +    ResourceHandle* handle = static_cast<ResourceHandle*>(callback_data);
> +    if (!handle)
> +        return FALSE;

I don't see how the handle can possibly be null. The callback data is set to "this" in start(), and can't be modified by WebCore.

Looks like you need to ref() the handle when creating an idle handler, and deref it when reportError() is done.

[Zan Dobersek]

Created attachment 27916 [details]
Report errors through an idle function when faced unsupported URL protocols

Now made to rely on reference counting.

[Alexey Proskuryakov]

Comment on attachment 27916 [details]
Report errors through an idle function when faced unsupported URL protocols

> +static gboolean reportError(gpointer callback_data)

I think that this function could use a more descriptive name, like reportUnknownProtocolError. r=me with or without this change.

[Gustavo Noronha (kov)]

Landed as r41226 with the function rename proposed by ap.