Bug 38690

I can confirm this problem and have created a minimal testcase. See it live here http://crydust.be/lab/prg/

Created attachment 59645 [details] Minimal testcase (requires apache server with php)

<rdar://problem/8132262>

Confirmed with r62300. Also reproducible with Safari 4.0.5 on Leopard.

Created attachment 60413 [details] test case Same test case, slightly modified to not rely on Apache DirectoryIndex setting.

It looks like this is related to the code in MainResourceLoader::willSendRequest. There is a call to isPostOrRedirectAfterPost, and if that returns true, we force the cache policy to ReloadIgnoringCacheData. We then modify the ResourceRequest of the DocumentLoader. Subresources are impacted by this because FrameLoader::subresourceCachePolicy inspects the ResourceRequest of the DocumentLoader. As a result, all of the subresources become conditional. (This loader system is making my head explode.)

The problem persists on iOS 4.0.1

The problem persists on iOS 4.1

The problem persists on iOS 4.2 (and the latset version of chrome and safari)

This defect is responsible for a quarter of our traffic on our high volume image servers.

Unless I'm misunderstanding the bug, I'm unable to reproduce this with Safari 5.0.3, Chrome 8, or a recent (~3 days old) Chromium tip of tree build. I'm consistently seeing If-Modified-Since and If-None-Match headers in the image request in the Post-Redirect-Get cycle (and therefore the server in the test case is returning a 304 for the image request). Are people still seeing this issue with any of those builds? It's possible I'm doing something wrong in my repro attempts.

(In reply to comment #11) > Unless I'm misunderstanding the bug, I'm unable to reproduce this with Safari 5.0.3, Chrome 8, or a recent (~3 days old) Chromium tip of tree build. I'm consistently seeing If-Modified-Since and If-None-Match headers in the image request in the Post-Redirect-Get cycle (and therefore the server in the test case is returning a 304 for the image request). > > Are people still seeing this issue with any of those builds? It's possible I'm doing something wrong in my repro attempts. My question is: Why is the browser sending the web server requests to check the modified date? We use far future expires headers ( 1 year ). I'm testing with Safari 8.0.3 and, after a post and redirect, see a lot of these date check requests resulting in 304s for objects that have been recently downloaded. Firefox and IE do not need to check the dates when performing the same test.

@Nate: The problem is not related to If-Modified-Since or If-None-Match headers. The problem is that there is a second request for the image. Firefox, internet explorer and opera alike don't request the image after the post-redirect-get because they remember the expires header from the first time they requested the image. An image sent with a far-future expires header should NEVER be requested again. Not even after a post-redirect-get. This is especially important on a mobile network which has a huge latency and low bandwidth. the problem persists in chrome 8 with useragent Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.224 Safari/534.10 the problem persists in chrome canari 10 with useragent Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.634.0 Safari/534.16 the problem persists in safari 5 with useragent Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4 the problem persists in safari on ios 4.2

Created attachment 78757 [details] possible patch As mentioned in the ChangeLog, the WebCore part of this patch is more or less a revert of http://trac.webkit.org/changeset/43878 and http://trac.webkit.org/changeset/41425. This patch passes all existing layout tests, as well as the new one for this patch (an adaptation of the test case above). However, I don't know whether it will still cause problems with Quick Look (see r41425 above).

Comment on attachment 78757 [details] possible patch Let's not revert this quite yet when we don't know what the consequences are.

(In reply to comment #15) > (From update of attachment 78757 [details]) > Let's not revert this quite yet when we don't know what the consequences are. I'm not familiar with the process for determining the fallout in non-browser WebKit apps. :( Any chance I could get some help or pointers in the right direction to resolve the uncertainty?

Comment on attachment 78757 [details] possible patch I agree that the risk of regressing QuickLook is pretty high in this case. You (or someone else) will likely need to temporarily replace WebKit frameworks in /System/Library with locally built ones, and see how QuickLook works with that. As a start, this can be done on either Mac OS X 10.5 or 10.6.

In general, running the 3rd party WebKit app with "DYLD_FRAMEWORK_PATH" set to your local build directory will would. Transient system services like QuickLooks are a slight different beast. Fortunately, there is a quicklooks command-line tool you can run against your built WebKit: http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man1/qlmanage.1.html

(In reply to comment #18) > In general, running the 3rd party WebKit app with "DYLD_FRAMEWORK_PATH" set to your local build directory will would. > > Transient system services like QuickLooks are a slight different beast. Fortunately, there is a quicklooks command-line tool you can run against your built WebKit: > http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man1/qlmanage.1.html Thus far, I've tried running both qlmanage and quicklookd from a terminal with DYLD_FRAMEWORK_PATH, as well as running them via Tools/Scripts/run-webkit-app. Neither have picked up my binary. I assume I'm doing something obviously wrong?

This works for me: DYLD_FRAMEWORK_PATH=/Users/ap/Safari/OpenSource/WebKitBuild/Debug /System/Library/Frameworks/QuickLook.framework/Versions/A/Resources/quicklookd.app/Contents/MacOS/qlmanage -p /Users/ap/Desktop/test.html

(In reply to comment #20) > This works for me: > > DYLD_FRAMEWORK_PATH=/Users/ap/Safari/OpenSource/WebKitBuild/Debug /System/Library/Frameworks/QuickLook.framework/Versions/A/Resources/quicklookd.app/Contents/MacOS/qlmanage -p /Users/ap/Desktop/test.html I was indeed doing something obviously wrong before. Thanks for the help! I was not able to reproduce the problem described in http://trac.webkit.org/changeset/41425 when running with my patch. Is there anything else you'd like me to test before re-requesting review?

I just realized that Bugzilla wrapped my reply, and it wasn't clear that this was a single line, not too lines. Looks like you got it right. The best thing to do would be for Brady to try running with your patch, since he knew how to reproduce the QL problem before. I don't know if he has the time for that though.

Just a reminder that this is still a very important issue. It is now being noticed by several developers. Most now have to choose between PRG and reloading the page completely or just not redirecting which breaks the back button. http://www.google.com/support/forum/p/Chrome/thread?tid=72bf3773f7e66d68&hl=en http://stackoverflow.com/questions/3004702/full-page-reload-on-post-redirect-get-ignoring-cache-control http://stackoverflow.com/questions/3114962/post-redirect-get-in-webkit-causes-a-full-page-reload http://stackoverflow.com/questions/4301405/webkit-image-reload-on-post-redirect-get/4896019

Comment on attachment 78757 [details] possible patch View in context: https://bugs.webkit.org/attachment.cgi?id=78757&action=review Nate, I would suggest also addressing a FIXME question in FrameLoader::loadPostRequest() in this patch. We know where this code is, it's in MainResourceLoader::willSendRequest(). The comment can probably be just removed - what do you think? Can you also add a test for POST result, not just redirect-after-POST? Please confirm that tests pass in Firefox. > LayoutTests/http/tests/cache/post-redirect-get.php:15 > +if ($_POST["submit"] == "redirect") { > + header('Location: post-redirect-get.php'); > + exit(); > +} Maybe set status line explicitly, too? > LayoutTests/http/tests/cache/post-redirect-get.php:44 > +function submitForm() > +{ > + if (window.eventSender) { > + var target = document.getElementById("submit"); > + eventSender.mouseMoveTo(target.offsetLeft + 2, target.offsetTop + 2); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > +} Why not just form.submit()? This needs a comment explaining why EventSender is used. > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:14 > + $etag = '"'.$filesize.'-'.$filemtime.'"'; Spaces are missing around dots. > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:16 > + $max_age = 12*31*24*60*60;//one year Spaces. > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:17 > + $expires = gmdate(DATE_RFC1123, time()+$max_age); Space. I personally like weird coding styles in HTML and JavaScript test sources, because this adds ad hoc testing for parsing code, but this is PHP, and we're not developing it here. > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:27 > + header('Etag: '.$etag); Space missing here. > LayoutTests/http/tests/cache/resources/post-redirect-get-cleanup.php:2 > +unlink("image-loaded.tmp"); Where does this file reside? See http/tests/resources/reset-temp-file.php and related files for how we normally implement state in HTTP tests.

It's too bad that <http://trac.webkit.org/changeset/43878> didn't have a test. It shouldn't be too hard to make one! We sorely lack test coverage in cache code, so a test for that would be of extreme value, even though this patch has little to do with that old change.

Sorry it took so long for me to get back to this, but yes - applying this patch does reproduce the quick looks bug.

Created attachment 81516 [details] Test case for quicklooks bug To run the Quick Looks test: 1 - Unzip these two address cards to the same directory. 2 - In a terminal on 10.6.6, run `qlmanage -p test1.vcf test2.vcf` 3 - Go back and forth in the Quick Looks window that comes up, notice the images change. 4 - Now run qlmanage against your local build with this patch - `DYLD_FRAMEWORK_PATH=<path to webkit build> qlmanage -p test1.vcf test2.vcf` 5 - Notice that going back and forth, the images do NOT change.

It sounds like there is a bug in Quick Look. Has a bug been filed against it? Can we consider some kind of run-time option for this that browsers embedding WebKit can flip to enable proper cache behavior? I don't see why the buggyness of Quick Look should hinder web browsers embedding WebKit.

(In reply to comment #28) > It sounds like there is a bug in Quick Look. Has a bug been filed against it? Future QuickLooks will work fine. But we cannot break old QuickLooks. >Can we consider some kind of run-time option for this that browsers embedding WebKit can flip to enable proper cache behavior? We can add a QuickLooks specific quirk to Settings and do a bundle check for it. > I don't see why the buggyness of Quick Look should hinder web browsers embedding WebKit. The WebKit project has traditionally had a zero regression. Even if the bug you're fixing is the "greater good," you can't knowingly break something else in the process. I'd be happy to help Nate apply proper bundle check + quirks here, or I can get to it myself, but not until next week.

(In reply to comment #29) > The WebKit project has traditionally had a zero regression. I meant "zero regression policy."

(In reply to comment #30) > (In reply to comment #29) > > The WebKit project has traditionally had a zero regression. > > I meant "zero regression policy." I understand. I'm glad we don't need to make such a difficult decision. A quirk sounds great to me.

(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #29) > > > The WebKit project has traditionally had a zero regression. > > > > I meant "zero regression policy." > > I understand. I'm glad we don't need to make such a difficult decision. A quirk sounds great to me. Quirks sound good to me too. I'll do that next week, if not sooner.

(In reply to comment #24) > (From update of attachment 78757 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78757&action=review > > Nate, I would suggest also addressing a FIXME question in FrameLoader::loadPostRequest() in this patch. We know where this code is, it's in MainResourceLoader::willSendRequest(). The comment can probably be just removed - what do you think? > > Can you also add a test for POST result, not just redirect-after-POST? Please confirm that tests pass in Firefox. I've addressed all comments except this last one. I'm not sure I fully understand, what are you looking for in the new test? Submit a form that posts to the original url? > > > LayoutTests/http/tests/cache/post-redirect-get.php:15 > > +if ($_POST["submit"] == "redirect") { > > + header('Location: post-redirect-get.php'); > > + exit(); > > +} > > Maybe set status line explicitly, too? > > > LayoutTests/http/tests/cache/post-redirect-get.php:44 > > +function submitForm() > > +{ > > + if (window.eventSender) { > > + var target = document.getElementById("submit"); > > + eventSender.mouseMoveTo(target.offsetLeft + 2, target.offsetTop + 2); > > + eventSender.mouseDown(); > > + eventSender.mouseUp(); > > + } > > +} > > Why not just form.submit()? This needs a comment explaining why EventSender is used. > > > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:14 > > + $etag = '"'.$filesize.'-'.$filemtime.'"'; > > Spaces are missing around dots. > > > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:16 > > + $max_age = 12*31*24*60*60;//one year > > Spaces. > > > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:17 > > + $expires = gmdate(DATE_RFC1123, time()+$max_age); > > Space. I personally like weird coding styles in HTML and JavaScript test sources, because this adds ad hoc testing for parsing code, but this is PHP, and we're not developing it here. > > > LayoutTests/http/tests/cache/resources/post-redirect-get-image.php:27 > > + header('Etag: '.$etag); > > Space missing here. > > > LayoutTests/http/tests/cache/resources/post-redirect-get-cleanup.php:2 > > +unlink("image-loaded.tmp"); > > Where does this file reside? See http/tests/resources/reset-temp-file.php and related files for how we normally implement state in HTTP tests.

>> Can you also add a test for POST result, not just redirect-after-POST? Please confirm that tests pass in Firefox. > > I've addressed all comments except this last one. I'm not sure I fully understand, what are you looking for in the new test? Submit a form that posts to the original url? The included tests verifies what happens after a POST followed by redirect. What happens after a POST that just returns an HTML document without any redirection? > <...> Please don't overquote. This makes it hard to follow bugs.

Created attachment 83584 [details] patch #2

Comment on attachment 83584 [details] patch #2 No ChangeLogs?

(In reply to comment #36) > (From update of attachment 83584 [details]) > No ChangeLogs? Wow, sorry about that. Will re-upload momentarily.

Created attachment 83587 [details] with ChangeLogs this time

Comment on attachment 83587 [details] with ChangeLogs this time View in context: https://bugs.webkit.org/attachment.cgi?id=83587&action=review > Source/WebCore/page/Settings.h:372 > + void setUseQuickLookResourceCachingQuirks(bool flag) { m_useQuickLookResourceCachingQuirks = flag; } Is this a chromium-only thing?

(In reply to comment #39) > (From update of attachment 83587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83587&action=review > > > Source/WebCore/page/Settings.h:372 > > + void setUseQuickLookResourceCachingQuirks(bool flag) { m_useQuickLookResourceCachingQuirks = flag; } > > Is this a chromium-only thing? It's going to be used only for the QuickLook app. I was assuming that that was in private Apple code and that I couldn't actually set it myself, but if I'm wrong, let me know. :)

Comment on attachment 83587 [details] with ChangeLogs this time View in context: https://bugs.webkit.org/attachment.cgi?id=83587&action=review >>> Source/WebCore/page/Settings.h:372 >>> + void setUseQuickLookResourceCachingQuirks(bool flag) { m_useQuickLookResourceCachingQuirks = flag; } >> >> Is this a chromium-only thing? > > It's going to be used only for the QuickLook app. I was assuming that that was in private Apple code and that I couldn't actually set it myself, but if I'm wrong, let me know. :) Where's the WebKit/mac code that does the bundle check and actually sets the flag? Without that code, this patch breaks things in the way previously discussed.

(In reply to comment #41) > (From update of attachment 83587 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83587&action=review > > >>> Source/WebCore/page/Settings.h:372 > >>> + void setUseQuickLookResourceCachingQuirks(bool flag) { m_useQuickLookResourceCachingQuirks = flag; } > >> > >> Is this a chromium-only thing? > > > > It's going to be used only for the QuickLook app. I was assuming that that was in private Apple code and that I couldn't actually set it myself, but if I'm wrong, let me know. :) > > Where's the WebKit/mac code that does the bundle check and actually sets the flag? > > Without that code, this patch breaks things in the way previously discussed. I worded that funny, sorry - I meant to answer your question and say "no, nothing private about it - it can and should be done in WebKit/mac"

Created attachment 86102 [details] Set the flag when QuickLook is loaded Verified that this patch passes both the new layout tests and Brady's quicklook repro steps.

Comment on attachment 86102 [details] Set the flag when QuickLook is loaded We'll just have to keep a close eye out for problems this might cause. I know this is a big performance problem, but am dubious that the Quicklook bug was the only correctness problem.

Comment on attachment 86102 [details] Set the flag when QuickLook is loaded Clearing flags on attachment: 86102 Committed r81635: <http://trac.webkit.org/changeset/81635>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 86102 [details]: http/tests/media/video-cancel-load.html bug 56797 (authors: eric.carlson@apple.com and jamesr@chromium.org) The commit-queue is continuing to process your patch.

The new tests are failing on Windows: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r81635%20(10626)/results.html

http://php.net/manual/en/function.sys-get-temp-dir.php says that sys_get_temp_dir is only available in PHP 5.2.1 and newer. We use PHP 4 on Windows. The tests will have to be rewritten to work with PHP 4.

I fixed the tests in r81665.