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.

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.

Created attachment 154402 [details] Patch

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

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

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?

Created attachment 154443 [details] Patch

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

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

Created attachment 154773 [details] Patch

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

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

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?

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

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

Created attachment 158927 [details] Patch for landing

Comment on attachment 158927 [details] Patch for landing Clearing flags on attachment: 158927 Committed r125829: <http://trac.webkit.org/changeset/125829>

All reviewed patches have been landed. Closing bug.

Thanks, Nate!

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

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

Created attachment 160210 [details] patch Turns out I accidentally reversed the order of two lines, and that order matters.

Comment on attachment 160210 [details] patch ok

Comment on attachment 160210 [details] patch Clearing flags on attachment: 160210 Committed r126483: <http://trac.webkit.org/changeset/126483>

All reviewed patches have been landed. Closing bug.

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.

Reverted r126483 for reason: Caused two Chromium browser_tests to time out 100% reliably. Committed r126507: <http://trac.webkit.org/changeset/126507>

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 on attachment 161293 [details] patch Ok.

Comment on attachment 161293 [details] patch Clearing flags on attachment: 161293 Committed r127087: <http://trac.webkit.org/changeset/127087>

All reviewed patches have been landed. Closing bug.