Bug 84614

Created attachment 138387 [details] v1 patch

v1 patch lacks additional layout tests. just posting here for EWS feedback.

Attachment 138387 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

How is this related to bug 32423 and bug 30862?

I need to attach the testcase, but basically whenever you insert an iframe, that causes m_isComplete to toggle back to false for the top-most frame. As a result, subresources fetched in the context of the iframe inherit the cache policy of the top-most frame. So, if 5 minutes passed since the top-most frame was reloaded, the newly injected iframe's subresources will be revalidated when loaded. The bug is that they should not be revalidated. Test case forthcoming. In other words, this bug is about making the cache policy of subresources consistent w.r.t. whether they are loading in the context of a newly added iframe or an already added iframe.

Comment on attachment 138387 [details] v1 patch Attachment 138387 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12490154 New failing tests: http/tests/navigation/anchor-subframeload.html http/tests/navigation/back-to-slow-frame.html fast/forms/radio/state-restore-radio-group.html http/tests/cache/subresource-failover-to-network.html http/tests/inspector/resource-har-conversion.html fast/history/same-document-iframes-changing-pushstate.html http/tests/navigation/error404-frames.html batterystatus/event-after-navigation.html accessibility/aria-disabled.html fast/history/saves-state-after-fragment-nav.html fast/history/history-back-initial-vs-final-url.html fast/history/form-submit-in-frame-via-onclick.html fast/forms/file/recover-file-input-in-unposted-form.html fast/history/history-length.html http/tests/inspector/extensions-useragent.html fast/forms/state-restore-hidden.html http/tests/navigation/dynamic-iframe-dynamic-form-back-entry.html fast/canvas/webgl/shader-precision-format.html fast/history/redirect-via-iframe.html fast/forms/state-restore-to-non-autocomplete-form.html http/tests/navigation/back-twice-without-commit.html fast/history/form-submit-in-frame.html http/tests/history/replacestate-post-to-get-2.html http/tests/inspector/extensions-ignore-cache.html fast/forms/button-state-restore.html fast/history/history-replace-updates-current-item.html fast/history/same-document-iframes-changing-fragment.html http/tests/navigation/anchor-goback.html http/tests/navigation/error404-subframeload.html fast/history/history-subframe-with-name.html

Created attachment 138507 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Does this also fix Workers the case in bug 36399?

Created attachment 138636 [details] v2 patch This patch should fix the layout test failures. More tests still to be added.

Attachment 138636 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 138636 [details] v2 patch Attachment 138636 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12522455 New failing tests: fast/loader/scroll-position-restored-on-back-non-cached.html http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier.html fast/loader/scroll-position-restored-on-back.html http/tests/navigation/scrollstate-after-location-reload.html http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier-2.html http/tests/navigation/scrollstate-after-http-equiv-refresh.html

Created attachment 138772 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 138828 [details] Additional layout tests Here's WIP additional layout tests. There's still an issue with the "within-iframe" one.

Created attachment 139074 [details] v3 patch Getting closer. This should pass all tests on chromium-linux. I am seeing some other failures on my PLATFORM(MAC) build though :-(

Comment on attachment 139074 [details] v3 patch Attachment 139074 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12555168 New failing tests: http/tests/navigation/scrollstate-after-location-reload.html http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier.html http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier-2.html http/tests/navigation/scrollstate-after-http-equiv-refresh.html

Created attachment 139165 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 139232 [details] Patch

> Created an attachment (id=139232) [details] This is a tweak of fishd's patch to get it to pass all the tests. Two remaining things I think we need to consider for this patch are: 1. Test removing this related bit in FrameLoader::load(): > // FIXME: is this the right place to reset loadType? Perhaps this should be done after loading is finished or aborted. > m_loadType = FrameLoadTypeStandard; 2. A test for aborted page load. This also doesn't fix the case where an iframe's src is set to the same URL (which IE and FF handle properly). I believe the fix for that is to remove this setCachePolicy in FrameLoader::load() (or at least restrict it to top level frames), however that's probably best left to a separate patch. > if (shouldTreatURLAsSameAsCurrent(newDocumentLoader->originalRequest().url())) { > r.setCachePolicy(ReloadIgnoringCacheData);

fast/loader/scroll-position-restored-on-back.html fails on the PLATFORM(MAC) build. I traced calls to HistoryController::restoreScrollPositionAndViewState to see which one was getting suppressed with the patch in this bug. I found this callstack: #0 0x0000000101f255cc in WebCore::HistoryController::restoreScrollPositionAndViewState (this=0x109065968) at /Vo lumes/Work/src/webkit-trunk/WebKit/Source/WebCore/loader/HistoryController.cpp:108 #1 0x0000000101e91322 in WebCore::FrameLoader::didFirstLayout (this=0x1090656d8) at /Volumes/Work/src/webkit-tru nk/WebKit/Source/WebCore/loader/FrameLoader.cpp:2233 #2 0x0000000101eb3d11 in WebCore::FrameView::performPostLayoutTasks (this=0x10864e030) at /Volumes/Work/src/webk it-trunk/WebKit/Source/WebCore/page/FrameView.cpp:2352 #3 0x0000000101eb91f4 in WebCore::FrameView::layout (this=0x10864e030, allowSubtree=false) at /Volumes/Work/src/ webkit-trunk/WebKit/Source/WebCore/page/FrameView.cpp:1166 #4 0x0000000101eb9403 in WebCore::FrameView::forceLayout (this=0x10864e030, allowSubtree=false) at /Volumes/Work /src/webkit-trunk/WebKit/Source/WebCore/page/FrameView.cpp:3235 #5 0x0000000101e9804a in WebCore::FrameLoader::commitProvisionalLoad (this=0x1090656d8) at /Volumes/Work/src/web kit-trunk/WebKit/Source/WebCore/loader/FrameLoader.cpp:1707 #6 0x0000000101e98332 in WebCore::FrameLoader::loadProvisionalItemFromCachedPage (this=0x1090656d8) at /Volumes/ Work/src/webkit-trunk/WebKit/Source/WebCore/loader/FrameLoader.cpp:2901 #7 0x0000000101e98687 in WebCore::FrameLoader::continueLoadAfterNavigationPolicy (this=0x1090656d8, formState=@0 x7fff5fbfd0b0, shouldContinue=true) at /Volumes/Work/src/webkit-trunk/WebKit/Source/WebCore/loader/FrameLoader.cp p:2772 It makes sense that we might not see this in the Chromium DRT since the Chromium port does not have the page cache feature enabled.

Created attachment 140077 [details] Alternate solution: FrameLoader::started() no longer modifies ancestors Here's an alternate solution. Studying subresourceCachePolicy(), even if we reset m_loadType at the right time, we still have to deal with the logic that inspects the document's ResourceRequest for request headers that bypass caching. I can't think of a good reason why FrameLoader::started() would need to set m_isComplete to false for all ancestor frames. Everywhere that we check to see if the page load is complete, we walk the children to see if any frame is incomplete, so flagging the top-most frame when only a subframe is navigating should not be necessary. Posting here to see what layout tests break! :-/

Created attachment 140309 [details] Alternate solution: FrameLoader::started() no longer modifies ancestors [take 2]

Comment on attachment 140309 [details] Alternate solution: FrameLoader::started() no longer modifies ancestors [take 2] Attachment 140309 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12633259 New failing tests: http/tests/navigation/javascriptlink-subframeload.html plugins/object-onfocus-mutation-crash.html

Created attachment 140322 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 140729 [details] Patch

This patch gets us past all the layout tests on PLATFORM(MAC) as well as chromium. This is a fragile area and I'm very interested in feedback from the loader experts.

Comment on attachment 140729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140729&action=review > Source/WebCore/loader/FrameLoader.cpp:-1240 > - m_loadType = FrameLoadTypeStandard; it is a bit surprising to delete this line here. i would have expected that upon starting a new load, that we would want to set m_loadType to correspond to whatever the new load is. what is the reasoning for removing this line?

(In reply to comment #26) > (From update of attachment 140729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140729&action=review > > > Source/WebCore/loader/FrameLoader.cpp:-1240 > > - m_loadType = FrameLoadTypeStandard; > > it is a bit surprising to delete this line here. i would have expected that > upon starting a new load, that we would want to set m_loadType to correspond > to whatever the new load is. what is the reasoning for removing this line? It isn't necessary to remove it. Just thought I was taking care of that FIXME since all the tests pass with it performed at completion. Anyway, I'll forget about it and leave it in place since that is probably safer.

Created attachment 140989 [details] Patch

Comment on attachment 140989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140989&action=review > LayoutTests/http/tests/cache/loaded-from-cache-after-reload-within-iframe.html:31 > + done('PASS'); i screwed up the indentation here

Comment on attachment 140989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140989&action=review >> LayoutTests/http/tests/cache/loaded-from-cache-after-reload-within-iframe.html:31 >> + done('PASS'); > > i screwed up the indentation here Fixed

Created attachment 141021 [details] Patch for landing

Comment on attachment 141021 [details] Patch for landing Clearing flags on attachment: 141021 Committed r116584: <http://trac.webkit.org/changeset/116584>

All reviewed patches have been landed. Closing bug.

The cr-linux ews seems to think this test is missing results: http/tests/cache/loaded-from-cache-after-reload-within-iframe.html -> unexpected no expected result found http/tests/cache/loaded-from-cache-after-reload.html -> unexpected no expected result found http://queues.webkit.org/results/12719091