window.onload never fires if page contains a <script src=foo> whose load is cancelled by resource load delegate returning null from willSendRequest

• Component: Page Loading
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. Download the attached test case to LayoutTests/fast/loader
2. run-webkit-tests fast/loader/onload-willSendRequest-null-for-script.html

The test will generate no output. It should generate output that includes the text "PASSED".

[Adam Roben (:aroben)]

<rdar://problem/7543406>

[Adam Roben (:aroben)]

The load of the script is initiated in HTMLTokenizer::scriptHandler with a call
to DocLoader::requestScript. The load fails before requestScript returns, but
requestScript returns a non-null CachedScript* anyway. HTMLTokenizer then never
gets CachedResourceClient::notifyFinished for this script.

I think the right fix might be for requestScript to return null in this case.

[Adam Roben (:aroben)]

Cache.cpp says <http://trac.webkit.org/browser/trunk/WebCore/loader/Cache.cpp?rev=53151#L134>:

// We don't support immediate loads, but we do support immediate failure.

But we're missing this particular "immediate failure" case.

[Adam Roben (:aroben)]

Cache::requestResource already returns null when the resource's load fails, but only if the cache is disabled. Maybe we should return null when the resource's load fails and the cache is enabled, too?

[Adam Roben (:aroben)]

(In reply to comment #4)
> Cache::requestResource already returns null when the resource's load fails, but
> only if the cache is disabled. Maybe we should return null when the resource's
> load fails and the cache is enabled, too?

I'm testing a fix that does just this. It fixes this bug, but I need to see if it causes any regression tests to fail.

[Adam Roben (:aroben)]

The current "immediate failure" code was added in r21732, for what it's worth: <http://trac.webkit.org/changeset/21732#file2>.

[Adam Roben (:aroben)]

Created attachment 46610 [details]
Patch

[Adam Roben (:aroben)]

I'll add the new test to LayoutTests/platform/qt/Skipped, too.

[Dave Hyatt]

Comment on attachment 46610 [details]
Patch

r=me

[Adam Roben (:aroben)]

Committed r53292: <http://trac.webkit.org/changeset/53292>

[Eric Seidel (no email)]

This looks like this broke the Gtk bots:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53294%20(2386)/fast/loader/onload-willSendRequest-null-for-script-diffs.txt

--- layout-test-results/fast/loader/onload-willSendRequest-null-for-script-expected.txt	2010-01-14 15:35:03.995244189 -0800
+++ layout-test-results/fast/loader/onload-willSendRequest-null-for-script-actual.txt	2010-01-14 15:35:03.995244189 -0800
@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: line 1: SyntaxError: Parse error
 Test for Bug 33687: window.onload never fires if page contains a <script src=foo> whose load is cancelled by resource load delegate returning null from willSendRequest. If the test passes, you should see the word "PASSED" below.
 
 PASSED

[Eric Seidel (no email)]

Adam?  Any thoughts on what we should do here for the Gtk bot?  Just skip this test?

[Eric Seidel (no email)]

Re-opening due to the bot failures.

[Adam Roben (:aroben)]

(In reply to comment #12)
> Adam?  Any thoughts on what we should do here for the Gtk bot?  Just skip this
> test?

Yeah, I guess I was too hopeful about this "just working" on other platforms.

The test was adde to GTK+'s Skipped file in r53286.