Bug 84614 – Subresources loaded after a reload completes shouldn't be revalidated.

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

Subresources loaded after a reload completes shouldn't be revalidated.

[Darin Fisher (:fishd, Google)]

Created attachment 138387 [details]
v1 patch

[Darin Fisher (:fishd, Google)]

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

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

How is this related to bug 32423 and bug 30862?

[Darin Fisher (:fishd, Google)]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Gentilcore]

Does this also fix Workers the case in bug 36399?

[Darin Fisher (:fishd, Google)]

Created attachment 138636 [details]
v2 patch

This patch should fix the layout test failures.  More tests still to be added.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

Created attachment 138828 [details]
Additional layout tests

Here's WIP additional layout tests.  There's still an issue with the "within-iframe" one.

[Darin Fisher (:fishd, Google)]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Gentilcore]

Created attachment 139232 [details]
Patch

[Tony Gentilcore]

> 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);

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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! :-/

[Darin Fisher (:fishd, Google)]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Gentilcore]

Created attachment 140729 [details]
Patch

[Tony Gentilcore]

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.

[Darin Fisher (:fishd, Google)]

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?

[Tony Gentilcore]

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

[Tony Gentilcore]

Created attachment 140989 [details]
Patch

[Darin Fisher (:fishd, Google)]

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

[Tony Gentilcore]

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

[Tony Gentilcore]

Created attachment 141021 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141021 [details]
Patch for landing

Clearing flags on attachment: 141021

Committed r116584: <http://trac.webkit.org/changeset/116584>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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