Bug 92272 - ProgressTracker never completes if iframe detached during parsing
Summary: ProgressTracker never completes if iframe detached during parsing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 94299 95441
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-25 11:17 PDT by Charles Reis
Modified: 2012-08-30 03:06 PDT (History)
10 users (show)

See Also:


Attachments
Two HTML files that repro the bug. (720 bytes, application/octet-stream)
2012-07-25 11:18 PDT, Charles Reis
no flags Details
Patch (2.02 KB, patch)
2012-07-25 11:37 PDT, Charles Reis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (400.78 KB, application/zip)
2012-07-25 13:19 PDT, WebKit Review Bot
no flags Details
Patch (2.64 KB, patch)
2012-07-25 14:14 PDT, Charles Reis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (303.88 KB, application/zip)
2012-07-25 15:27 PDT, WebKit Review Bot
no flags Details
Patch (1.68 KB, patch)
2012-07-26 15:39 PDT, Charles Reis
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (634.81 KB, application/zip)
2012-07-26 18:12 PDT, WebKit Review Bot
no flags Details
Add a helper class to match ProgressTracker calls (5.41 KB, patch)
2012-08-16 13:55 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (5.60 KB, patch)
2012-08-16 15:44 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch (5.28 KB, patch)
2012-08-23 12:09 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
patch (5.33 KB, patch)
2012-08-29 12:57 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Reis 2012-07-25 11:17:25 PDT
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.)
Comment 1 Charles Reis 2012-07-25 11:18:47 PDT
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.
Comment 2 Charles Reis 2012-07-25 11:20:00 PDT
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.
Comment 3 Charles Reis 2012-07-25 11:37:51 PDT
Created attachment 154402 [details]
Patch
Comment 4 WebKit Review Bot 2012-07-25 13:19:39 PDT
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
Comment 5 WebKit Review Bot 2012-07-25 13:19:42 PDT
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
Comment 6 Charles Reis 2012-07-25 14:13:49 PDT
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?
Comment 7 Charles Reis 2012-07-25 14:14:08 PDT
Created attachment 154443 [details]
Patch
Comment 8 Nate Chapin 2012-07-25 14:23:40 PDT
(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.
Comment 9 WebKit Review Bot 2012-07-25 15:27:25 PDT
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
Comment 10 WebKit Review Bot 2012-07-25 15:27:28 PDT
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
Comment 11 Charles Reis 2012-07-26 15:39:33 PDT
Created attachment 154773 [details]
Patch
Comment 12 WebKit Review Bot 2012-07-26 18:11:58 PDT
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
Comment 13 WebKit Review Bot 2012-07-26 18:12:01 PDT
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
Comment 14 Charles Reis 2012-07-27 12:43:14 PDT
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.
Comment 15 jeffharris 2012-08-08 11:27:47 PDT
Hey Charlie - any ideas on who the best people to ask about DocumentLoader would be?
Comment 16 Nate Chapin 2012-08-16 13:55:06 PDT
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.
Comment 17 Adam Barth 2012-08-16 14:01:36 PDT
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.  :)
Comment 18 Nate Chapin 2012-08-16 15:05:27 PDT
(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.
Comment 19 Nate Chapin 2012-08-16 15:44:22 PDT
Created attachment 158927 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-08-16 17:02:39 PDT
Comment on attachment 158927 [details]
Patch for landing

Clearing flags on attachment: 158927

Committed r125829: <http://trac.webkit.org/changeset/125829>
Comment 21 WebKit Review Bot 2012-08-16 17:02:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Charles Reis 2012-08-16 17:03:36 PDT
Thanks, Nate!
Comment 23 WebKit Review Bot 2012-08-16 23:01:16 PDT
Re-opened since this is blocked by 94299
Comment 24 jeffharris 2012-08-21 22:22:27 PDT
Nate - have you had a chance to look into getting this resubmitted? 

I'm sitting here starring longingly at a perpetually spinning Docs tabs :(
Comment 25 Nate Chapin 2012-08-23 10:21:45 PDT
(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.
Comment 26 Nate Chapin 2012-08-23 12:09:20 PDT
Created attachment 160210 [details]
patch

Turns out I accidentally reversed the order of two lines, and that order matters.
Comment 27 Adam Barth 2012-08-23 14:07:20 PDT
Comment on attachment 160210 [details]
patch

ok
Comment 28 WebKit Review Bot 2012-08-23 14:25:45 PDT
Comment on attachment 160210 [details]
patch

Clearing flags on attachment: 160210

Committed r126483: <http://trac.webkit.org/changeset/126483>
Comment 29 WebKit Review Bot 2012-08-23 14:25:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Kenneth Russell 2012-08-23 17:06:30 PDT
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.
Comment 31 Kenneth Russell 2012-08-23 17:09:09 PDT
Reverted r126483 for reason:

Caused two Chromium browser_tests to time out 100% reliably.

Committed r126507: <http://trac.webkit.org/changeset/126507>
Comment 32 Nate Chapin 2012-08-29 12:57:26 PDT
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! :)
Comment 33 Adam Barth 2012-08-29 13:06:43 PDT
Comment on attachment 161293 [details]
patch

Ok.
Comment 34 WebKit Review Bot 2012-08-29 19:58:14 PDT
Comment on attachment 161293 [details]
patch

Clearing flags on attachment: 161293

Committed r127087: <http://trac.webkit.org/changeset/127087>
Comment 35 WebKit Review Bot 2012-08-29 19:58:19 PDT
All reviewed patches have been landed.  Closing bug.