Bug 92272 – ProgressTracker never completes if iframe detached during parsing

• Component: Page Loading
• Status: RESOLVED FIXED

[Charles Reis]

If an iframe is removed from its parent after it finishes loading but while parsing is still in progress, ProgressTracker::progressCompleted is never called for it.  This leaves m_numProgressTrackedFrames non-zero, which means the parent page will never reach ProgressTracker::finalProgressComplete, and the loading icon will spin forever in that tab.

This happens in practice for Google Docs, as discovered at http://crbug.com/120321.

I've put together a simplified repro case (attached), but I've only been able to repro if I attach a debugger and set a breakpoint in MainResourceLoader::didFinishLoading.  If the breakpoint gets hit when adding the iframe, the bug occurs, and the loading icon spins forever.  (I'm not sure how to make the test more deterministic.  The bug is inconsistent on Google Docs as well, suggesting a race.)

[Charles Reis]

Created attachment 154399 [details]
Two HTML files that repro the bug.

never-finish-loading.html has a button to load self-delete.html as an iframe.  It deletes itself while the iframe is still being parsed.

[Charles Reis]

Some observations about why this seems to be happening:

We call ProgressTracker::progressStarted() when the iframe starts loading, but we don't call ProgressTracker::progressCompleted() when the iframe gets to MainResourceLoader::didFinishLoading().  This is because FrameLoader::checkLoadCompleteForThisFrame() calls DocumentLoader::isLoadingInAPISense(), which returns true because we're still parsing the document.

The iframe later calls the parent's removeChild() on itself during parsing, so we get to FrameLoader::frameDetached.  From there, we get to DocumentLoader::stopLoading(), which returns early because isLoading() is false (even though isLoadingInAPISense() is true).

One way to fix this bug is by checking isLoadingInAPISense() instead of isLoading() in DocumentLoader::stopLoading().  I'm not familiar enough with this code to be certain, but that does seem like a reasonable approach to me.

[Charles Reis]

Created attachment 154402 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154402 [details]
Patch

Attachment 154402 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13348341

New failing tests:
http/tests/security/feed-urls-from-remote.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
editing/execCommand/apply-style-command-crash.html
http/tests/navigation/changing-frame-hierarchy-in-onload.html
http/tests/misc/xslt-bad-import.html
http/tests/xmlhttprequest/frame-unload-abort-crash.html
fast/dom/onload-open.html

[WebKit Review Bot]

Created attachment 154423 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Charles Reis]

It appears that DocumentLoader::m_frame->settings() is null in some of the tests, so I can add a null check in isLoadingInAPISense() if that's expected.

I'm not sure why http/tests/navigation/image-load-in-subframe-unload-handler.html is crashing.  Something about FrameLoader::userAgent during a redirect, where m_client appears to be null.  I don't see how my change would cause that, though.

Nate, any ideas about a better way to fix this, or a way to improve this patch?

[Charles Reis]

Created attachment 154443 [details]
Patch

[Nate Chapin]

(In reply to comment #6)
> It appears that DocumentLoader::m_frame->settings() is null in some of the tests, so I can add a null check in isLoadingInAPISense() if that's expected.
> 
> I'm not sure why http/tests/navigation/image-load-in-subframe-unload-handler.html is crashing.  Something about FrameLoader::userAgent during a redirect, where m_client appears to be null.  I don't see how my change would cause that, though.
> 
> Nate, any ideas about a better way to fix this, or a way to improve this patch?

m_frame->settings() should only happen if the Frame is detached from the Page. I would guess we still need to be able to call stopLoading() at that point, but I'd need to look more carefully to be absolutely sure.

FrameLodaer::m_client should never be null....which makes me think there's a use-after-free being introduced, but I don't see anything obvious.

[WebKit Review Bot]

Comment on attachment 154443 [details]
Patch

Attachment 154443 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13361189

New failing tests:
http/tests/security/feed-urls-from-remote.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
http/tests/misc/xslt-bad-import.html
http/tests/xmlhttprequest/frame-unload-abort-crash.html

[WebKit Review Bot]

Created attachment 154461 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Charles Reis]

Created attachment 154773 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154773 [details]
Patch

Attachment 154773 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13368148

New failing tests:
http/tests/security/feed-urls-from-remote.html
http/tests/loading/pdf-commit-load-callbacks.html
http/tests/navigation/changing-frame-hierarchy-in-onload.html
perf/document-contains.html
platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-blocked.html
http/tests/loading/bad-scheme-subframe.html
http/tests/loading/bad-server-subframe.html

[WebKit Review Bot]

Created attachment 154802 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Charles Reis]

Does anyone with more knowledge of DocumentLoader than me have an idea for how to fix this issue?

I've tried not returning early if isLoadingInAPISense() returns true, not returning early at all, and returning early but first setting m_isStopping to true and calling FrameLoader::checkLoadComplete().  All of these fix the bug but seem to trigger a large number of test failures, which I've had trouble deciphering.

Somehow, we need to update ProgressTracker if a frame is detached after loading finishes but while parsing is still in progress.

Hey Charlie - any ideas on who the best people to ask about DocumentLoader would be?

[Nate Chapin]

Created attachment 158893 [details]
Add a helper class to match ProgressTracker calls

This patch cheats a bit: Instead of trying to ensure DocumentLoader does the right thing, solve the problem more generally by adding a helper class to FrameLoader that counts progressStarted()/progressCompleted() calls for a given frame and makes sure they're matched before frame detach.

[Adam Barth]

Comment on attachment 158893 [details]
Add a helper class to match ProgressTracker calls

View in context: https://bugs.webkit.org/attachment.cgi?id=158893&action=review

> Source/WebCore/loader/FrameLoader.h:376
> +    class FrameProgressTracker {

I would have forward declared the class and then put the definition in FrameLoader.cpp, but this is fine too.  :)

[Nate Chapin]

(In reply to comment #17)
> (From update of attachment 158893 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158893&action=review
> 
> > Source/WebCore/loader/FrameLoader.h:376
> > +    class FrameProgressTracker {
> 
> I would have forward declared the class and then put the definition in FrameLoader.cpp, but this is fine too.  :)

I like your way better, will change before landing.

[Nate Chapin]

Created attachment 158927 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158927 [details]
Patch for landing

Clearing flags on attachment: 158927

Committed r125829: <http://trac.webkit.org/changeset/125829>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Charles Reis]

Thanks, Nate!

[WebKit Review Bot]

Re-opened since this is blocked by 94299

Nate - have you had a chance to look into getting this resubmitted? 

I'm sitting here starring longingly at a perpetually spinning Docs tabs :(

[Nate Chapin]

(In reply to comment #24)
> Nate - have you had a chance to look into getting this resubmitted? 
> 
> I'm sitting here starring longingly at a perpetually spinning Docs tabs :(

Yeah, if all goes well I'll post the fix today.

[Nate Chapin]

Created attachment 160210 [details]
patch

Turns out I accidentally reversed the order of two lines, and that order matters.

[Adam Barth]

Comment on attachment 160210 [details]
patch

ok

[WebKit Review Bot]

Comment on attachment 160210 [details]
patch

Clearing flags on attachment: 160210

Committed r126483: <http://trac.webkit.org/changeset/126483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Good news and bad news. The bad news is that this patch caused two of Chromium's iframe-related browser tests to fail. The good news is that the failure is 100% reliable and definitely caused by this patch (I rolled it out locally and the tests stopped timing out and started passing again), so there is the possibility that we will be able to distill a regression test for this change from those tests.

Here are the first two builds that failed:

http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/13026
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/23330

The failing tests are ErrorPageTest.IFrameDNSError_GoBackAndForward and ErrorPageTest.IFrameDNSError_GoBack.

I reproduced the failure locally on Linux by building the browser_tests target in Release mode and running:

browser_tests --gtest_filter=ErrorPageTest.IFrameDNSError\*

After talking with japhet, I'm going to roll out this patch so he can sort it out next week.

[Kenneth Russell]

Reverted r126483 for reason:

Caused two Chromium browser_tests to time out 100% reliably.

Committed r126507: <http://trac.webkit.org/changeset/126507>

[Nate Chapin]

Created attachment 161293 [details]
patch

Ok, I ran all of chromium's browser_tests on this patch and they passed. Let's see what breaks this time! :)

[Adam Barth]

Comment on attachment 161293 [details]
patch

Ok.

[WebKit Review Bot]

Comment on attachment 161293 [details]
patch

Clearing flags on attachment: 161293

Committed r127087: <http://trac.webkit.org/changeset/127087>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.