Bug 33533 – 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

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

Created attachment 46373 [details]
Test case

To reproduce, apply the following patch, build, and run WebKitAPITest.exe.

[Adam Roben (:aroben)]

Created attachment 46374 [details]
Revised test case

[Adam Roben (:aroben)]

This happens on Mac, too.

[Adam Roben (:aroben)]

<rdar://problem/7533333>

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

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).

[Adam Roben (:aroben)]

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();
 }

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

I'm now testing a modified fix that calls checkCompleted() whenever state() != FrameStateComplete. It looks promising.

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

(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::).

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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>.

[Adam Roben (:aroben)]

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>

[Adam Roben (:aroben)]

Created attachment 46590 [details]
Treat all synchronous loads equally in FrameLoader::loadSubframe

[Adam Roben (:aroben)]

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?

[Brady Eidson]

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.

[Adam Roben (:aroben)]

(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!

[Adam Roben (:aroben)]

Committed r53277: <http://trac.webkit.org/changeset/53277>

[Eric Seidel (no email)]

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?

[Adam Roben (:aroben)]

(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.

[Eric Seidel (no email)]

Yes, I believe Qt has their own separate LayoutTestController for historical reasons.  I should file a bug about that.

[Adam Roben (:aroben)]

I ended up skipping the tests because Qt already had entries in the skipped file for, e.g., setCustomPolicyDelegate.

[Csaba Osztrogonác]

(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.

[Adam Roben (:aroben)]

Added onload-policy-ignore-for-frame.html to the GTK+ Skipped file in r53286.

Guess I should have listened to Brady. :-)