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 Loading | Assignee: | 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
Adam Roben (:aroben)
2010-01-12 06:43:55 PST
Created attachment 46373 [details]
Test case
To reproduce, apply the following patch, build, and run WebKitAPITest.exe.
Created attachment 46374 [details]
Revised test case
This happens on Mac, too. 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 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 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). 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(); } (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. I'm now testing a modified fix that calls checkCompleted() whenever state() != FrameStateComplete. It looks promising. (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. (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::). 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. 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>. 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> Created attachment 46590 [details]
Treat all synchronous loads equally in FrameLoader::loadSubframe
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 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.
(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! Committed r53277: <http://trac.webkit.org/changeset/53277> 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? (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. Yes, I believe Qt has their own separate LayoutTestController for historical reasons. I should file a bug about that. I ended up skipping the tests because Qt already had entries in the skipped file for, e.g., setCustomPolicyDelegate. (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. Added onload-policy-ignore-for-frame.html to the GTK+ Skipped file in r53286. Guess I should have listened to Brady. :-) |