Bug 33533

Summary: window.onload never fires if page contains an <iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, beidson, darin, eric, ggaren, mjs, ossy
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case
none
Revised test case
none
Treat all synchronous loads equally in FrameLoader::loadSubframe beidson: review+

Description Adam Roben (:aroben) 2010-01-12 06:43:55 PST
If a page contains an <iframe> whose load is cancelled by returning null from IWebResourceLoadDelegate::willSendRequest, the load event will never be fired for that page.
Comment 1 Adam Roben (:aroben) 2010-01-12 06:47:14 PST
Created attachment 46373 [details]
Test case

To reproduce, apply the following patch, build, and run WebKitAPITest.exe.
Comment 2 Adam Roben (:aroben) 2010-01-12 07:00:17 PST
Created attachment 46374 [details]
Revised test case
Comment 3 Adam Roben (:aroben) 2010-01-12 07:25:52 PST
This happens on Mac, too.
Comment 4 Adam Roben (:aroben) 2010-01-12 07:26:22 PST
<rdar://problem/7533333>
Comment 5 Adam Roben (:aroben) 2010-01-12 07:57:56 PST
Working backwards:

* When the main document finishes parsing, we don't send onload because we detect that some child frames are still loading (in FrameLoader::checkCompleted [1]). The child that hasn't finished loading is the only subframe in the document: the one whose load the resource load delegate blocked.
* The child frame thinks it's still loading because its m_isComplete member was set to false in FrameLoader::loadSubframe [2].

But in fact the child frame has already finished loading. It finished before the call to FrameLoaderClient::createFrame completed [3], and m_isComplete was set to true. But then it was immediately set back to false in FrameLoader::loadSubframe [2].

Setting m_isComplete to false in loadSubframe seems to date all the way back to r7272 (!) [4]. At one point there was a comment questioning whether this was really needed [5], but it got removed during some refactoring [6].

1. http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=53140#L1098
2. http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=53140#L406
3. http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=53140#L399
4. http://trac.webkit.org/changeset/7272#file1
5. http://trac.webkit.org/changeset/12638#file12
6. http://trac.webkit.org/changeset/17652
Comment 6 Adam Roben (:aroben) 2010-01-12 08:05:49 PST
Given the description in comment 5, I wondered what happens with <iframe src="about:blank">. I would have expected it to behave in a similarly buggy manner. But FrameLoader::loadSubframe knows that about:blank loads synchronously, and calls FrameLoader::checkCompleted again on the subframe before returning [1], which sets m_isComplete back to true.

So maybe we need to teach FrameLoader::checkCompleted that other URLs can potentially load synchronously.

1. http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=53140#L415
Comment 7 Adam Roben (:aroben) 2010-01-12 08:13:40 PST
I wondered if m_isComplete is set to false for asynchronously-loading frames after FrameLoaderClient::createFrame returns (in which case setting it to false in FrameLoader::loadSubframe would be unnecessary). But it is set to true, beneath FrameLoader::init (via Document::cancelParsing -> Document::close -> FrameLoader::checkCompleted).
Comment 8 Adam Roben (:aroben) 2010-01-12 16:18:27 PST
The following change seems to fix the bug and doesn't make any regression tests fail. It seems right to me. Maybe I can get Anders or some similarly loader-educated person to take a look.

@@ -412,16 +412,7 @@ Frame* FrameLoader::loadSubframe(HTMLFrameOwnerElement* ownerElement, const KURL

     checkCallImplicitClose();

-    // In these cases, the synchronous load would have finished
-    // before we could connect the signals, so make sure to send the
-    // completed() signal for the child by hand
-    // FIXME: In this case the Frame will have finished loading before
-    // it's being added to the child list. It would be a good idea to
-    // create the child first, then invoke the loader separately.
-    if (url.isEmpty() || url == blankURL()) {
-        frame->loader()->completed();
-        frame->loader()->checkCompleted();
-    }
+    frame->loader()->checkCompleted();

     return frame.get();
 }
Comment 9 Adam Roben (:aroben) 2010-01-13 10:23:16 PST
(In reply to comment #8)
> The following change seems to fix the bug and doesn't make any regression tests
> fail.

Scratch that. It makes many tests fail.
Comment 10 Adam Roben (:aroben) 2010-01-13 12:08:25 PST
I'm now testing a modified fix that calls checkCompleted() whenever state() != FrameStateComplete. It looks promising.
Comment 11 Adam Roben (:aroben) 2010-01-13 12:44:49 PST
(In reply to comment #10)
> I'm now testing a modified fix that calls checkCompleted() whenever state() !=
> FrameStateComplete. It looks promising.

This approach seems to fix the bug. It also caused a progression in http/tests/loading/bad-scheme-subframe.html. Apparently onload was never firing in that case, either.
Comment 12 Adam Roben (:aroben) 2010-01-13 13:30:08 PST
(In reply to comment #11)
> (In reply to comment #10)
> > I'm now testing a modified fix that calls checkCompleted() whenever state() !=
> > FrameStateComplete. It looks promising.
> 
> This approach seems to fix the bug. It also caused a progression in
> http/tests/loading/bad-scheme-subframe.html. Apparently onload was never firing
> in that case, either.

Looks like bad schemes end up calling [listener ignore] from
-decidePolicyForNavigationAction:::::, which in turn results in a null
ResourceRequest (just like when one returns nil from
-webView:resource:willSendRequest::).
Comment 13 Adam Roben (:aroben) 2010-01-13 13:53:11 PST
I think I'll end up writing three tests:

1) Contains an <iframe> for which the resource load delegate returns nil from willSendRequest
2) Contains an <iframe> for which the policy delegate calls [listener ignore]
3) Contains an <iframe> with a bad URL scheme

In all cases, the test will succeed if onload is fired.

I'm going to have to add new API to make (1) possible. I believe (2) is already possible using layoutTestController.setCustomPolicyDelegate(true), and (3) is clearly already possible as well.
Comment 14 Adam Roben (:aroben) 2010-01-14 10:45:15 PST
For those playing along at home, the checkCompleted() call in the synchronous load case in loadSubframe was added in r8316: <http://trac.webkit.org/changeset/8316>.
Comment 15 Adam Roben (:aroben) 2010-01-14 10:47:52 PST
And setting m_isComplete to false was added in r1035 when khtml_part.cpp was first added to the repository (it was called m_bCompleted then). <http://trac.webkit.org/changeset/1035>
Comment 16 Adam Roben (:aroben) 2010-01-14 11:09:38 PST
Created attachment 46590 [details]
Treat all synchronous loads equally in FrameLoader::loadSubframe
Comment 17 Adam Roben (:aroben) 2010-01-14 11:10:52 PST
Adding some more loader-familiar people to the CC list in hopes that they will have thoughts on this patch!

Really the only question I have is: is checking frame->loader()->state() == FrameStateCompleted the best way to check if a synchronous load completed beneath FrameLoaderClient::createFrame?
Comment 18 Brady Eidson 2010-01-14 11:18:08 PST
Comment on attachment 46590 [details]
Treat all synchronous loads equally in FrameLoader::loadSubframe

Since the new layouttestcontroller voodoo is only on windows and mac for now, you should add the tests that rely on it to the skipped list for the other platforms (unless I'm missing something as to why they wouldn't fail)

r+ with that change.
Comment 19 Adam Roben (:aroben) 2010-01-14 11:21:26 PST
(In reply to comment #18)
> (From update of attachment 46590 [details])
> Since the new layouttestcontroller voodoo is only on windows and mac for now,
> you should add the tests that rely on it to the skipped list for the other
> platforms (unless I'm missing something as to why they wouldn't fail)
> 
> r+ with that change.

onload-bad-scheme-for-frame.html is definitely valid for all platforms (it doesn't require any DRT support).

The other two tests should pass on other platforms, too. The tests only fail if the DRT support exists and the FrameLoader change *is not* present. If DRT doesn't have the required support, the subframe loads will be asynchronous and won't trigger this bug.

Thanks for reviewing!
Comment 20 Adam Roben (:aroben) 2010-01-14 11:31:35 PST
Committed r53277: <http://trac.webkit.org/changeset/53277>
Comment 21 Eric Seidel (no email) 2010-01-14 11:45:52 PST
This caused two failures on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r53277%20(6000)/results.html

It looks like they just need to be skipped because Qt is missing the required layoutTestController method?
Comment 22 Adam Roben (:aroben) 2010-01-14 11:47:24 PST
(In reply to comment #21)
> This caused two failures on Qt:
> http://build.webkit.org/results/Qt%20Linux%20Release/r53277%20(6000)/results.html
> 
> It looks like they just need to be skipped because Qt is missing the required
> layoutTestController method?

I guess Qt isn't using WebKitTools/DumpRenderTree/LayoutTestController.cpp?

I think our policy is to check in failing results and file a bug rather than skip the test for cases like this.
Comment 23 Eric Seidel (no email) 2010-01-14 11:48:26 PST
Yes, I believe Qt has their own separate LayoutTestController for historical reasons.  I should file a bug about that.
Comment 24 Adam Roben (:aroben) 2010-01-14 11:53:57 PST
I ended up skipping the tests because Qt already had entries in the skipped file for, e.g., setCustomPolicyDelegate.
Comment 25 Csaba Osztrogonác 2010-01-14 11:58:41 PST
(In reply to comment #24)
> I ended up skipping the tests because Qt already had entries in the skipped
> file for, e.g., setCustomPolicyDelegate.

Thx for http://trac.webkit.org/changeset/53277 .
You find the best way to solve this problem.
Comment 26 Adam Roben (:aroben) 2010-01-14 12:40:14 PST
Added onload-policy-ignore-for-frame.html to the GTK+ Skipped file in r53286.

Guess I should have listened to Brady. :-)