Bug 38690 - Submitting a POST that leads to a server redirect causes all cached items to redownload
Summary: Submitting a POST that leads to a server redirect causes all cached items to ...
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: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-06 14:21 PDT by Henry
Modified: 2011-03-22 07:08 PDT (History)
12 users (show)

See Also:


Attachments
Minimal testcase (requires apache server with php) (2.77 KB, application/octet-stream)
2010-06-24 05:20 PDT, Kristof Neirynck
no flags Details
test case (4.37 KB, application/zip)
2010-07-02 15:38 PDT, Alexey Proskuryakov
no flags Details
possible patch (6.93 KB, patch)
2011-01-12 16:52 PST, Nate Chapin
beidson: review-
Details | Formatted Diff | Diff
Test case for quicklooks bug (5.32 KB, application/zip)
2011-02-07 14:08 PST, Brady Eidson
no flags Details
patch #2 (10.29 KB, patch)
2011-02-23 17:01 PST, Nate Chapin
ap: commit-queue-
Details | Formatted Diff | Diff
with ChangeLogs this time (12.39 KB, patch)
2011-02-23 17:14 PST, Nate Chapin
beidson: review-
Details | Formatted Diff | Diff
Set the flag when QuickLook is loaded (17.19 KB, patch)
2011-03-17 15:09 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 Henry 2010-05-06 14:21:45 PDT
When you submit a form to a webpage that then responds with a header location redirect (ie 302) it causes webkit to load the page from scratch.  None of the previously cached images,css,html,etc are used.  Instead it will redownload everything from the server again.  This pattern is highly prevalent in web applications and leads to lower performance of those applications.

As far as I can tell, this affects all Webkit versions and derivatives such as Chrome and all windows platforms.  I have not tested it on the Mac.

Here is the flow.

1. Submit POST to http://127.0.0.1/firststep.php
2. Server responds.  Status: 302\nLocation: http://127.0.0.1/nextstep.php
3. Webkit loads http://127.0.0.1/nextstep.php without using any cached items.
Comment 1 Kristof Neirynck 2010-06-24 05:18:35 PDT
I can confirm this problem and have created a minimal testcase.
See it live here 
http://crydust.be/lab/prg/
Comment 2 Kristof Neirynck 2010-06-24 05:20:14 PDT
Created attachment 59645 [details]
Minimal testcase (requires apache server with php)
Comment 3 Alexey Proskuryakov 2010-06-25 12:45:49 PDT
<rdar://problem/8132262>
Comment 4 Alexey Proskuryakov 2010-07-02 15:32:30 PDT
Confirmed with r62300. Also reproducible with Safari 4.0.5 on Leopard.
Comment 5 Alexey Proskuryakov 2010-07-02 15:38:57 PDT
Created attachment 60413 [details]
test case

Same test case, slightly modified to not rely on Apache DirectoryIndex setting.
Comment 6 Darin Fisher (:fishd, Google) 2010-07-08 14:39:44 PDT
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.)
Comment 7 Kristof Neirynck 2010-07-16 01:25:10 PDT
The problem persists on iOS 4.0.1
Comment 8 Kristof Neirynck 2010-09-14 01:29:15 PDT
The problem persists on iOS 4.1
Comment 9 Kristof Neirynck 2010-11-29 07:11:59 PST
The problem persists on iOS 4.2 (and the latset version of chrome and safari)
Comment 10 jrgeller 2011-01-10 13:50:13 PST
This defect is responsible for a quarter of our traffic on our high volume image servers.
Comment 11 Nate Chapin 2011-01-10 16:55:28 PST
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.
Comment 12 Chris March 2011-01-11 08:06:53 PST
(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.
Comment 13 Kristof Neirynck 2011-01-11 08:07:54 PST
@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
Comment 14 Nate Chapin 2011-01-12 16:52:59 PST
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 15 Brady Eidson 2011-01-13 08:27:08 PST
Comment on attachment 78757 [details]
possible patch

Let's not revert this quite yet when we don't know what the consequences are.
Comment 16 Nate Chapin 2011-01-13 11:40:01 PST
(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 17 Alexey Proskuryakov 2011-01-13 11:54:32 PST
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.
Comment 18 Brady Eidson 2011-01-13 13:55:43 PST
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
Comment 19 Nate Chapin 2011-01-18 14:24:20 PST
(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?
Comment 20 Alexey Proskuryakov 2011-01-18 14:39:57 PST
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
Comment 21 Nate Chapin 2011-01-18 15:57:52 PST
(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?
Comment 22 Alexey Proskuryakov 2011-01-18 16:06:28 PST
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.
Comment 23 Kristof Neirynck 2011-02-04 00:44:33 PST
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 24 Alexey Proskuryakov 2011-02-07 10:51:43 PST
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.
Comment 25 Alexey Proskuryakov 2011-02-07 10:59:23 PST
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.
Comment 26 Brady Eidson 2011-02-07 14:00:22 PST
Sorry it took so long for me to get back to this, but yes - applying this patch does reproduce the quick looks bug.
Comment 27 Brady Eidson 2011-02-07 14:08:24 PST
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.
Comment 28 Darin Fisher (:fishd, Google) 2011-02-07 22:30:33 PST
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.
Comment 29 Brady Eidson 2011-02-08 13:04:35 PST
(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.
Comment 30 Brady Eidson 2011-02-08 13:05:08 PST
(In reply to comment #29)
> The WebKit project has traditionally had a zero regression.

I meant "zero regression policy."
Comment 31 Darin Fisher (:fishd, Google) 2011-02-08 13:19:54 PST
(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.
Comment 32 Nate Chapin 2011-02-08 13:52:37 PST
(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.
Comment 33 Nate Chapin 2011-02-22 16:27:35 PST
(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.
Comment 34 Alexey Proskuryakov 2011-02-22 16:36:41 PST
>> 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.
Comment 35 Nate Chapin 2011-02-23 17:01:43 PST
Created attachment 83584 [details]
patch #2
Comment 36 Alexey Proskuryakov 2011-02-23 17:08:55 PST
Comment on attachment 83584 [details]
patch #2

No ChangeLogs?
Comment 37 Nate Chapin 2011-02-23 17:10:17 PST
(In reply to comment #36)
> (From update of attachment 83584 [details])
> No ChangeLogs?

Wow, sorry about that.  Will re-upload momentarily.
Comment 38 Nate Chapin 2011-02-23 17:14:30 PST
Created attachment 83587 [details]
with ChangeLogs this time
Comment 39 Eric Seidel (no email) 2011-02-24 02:37:52 PST
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?
Comment 40 Nate Chapin 2011-02-24 08:34:00 PST
(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 41 Brady Eidson 2011-03-14 12:21:16 PDT
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.
Comment 42 Brady Eidson 2011-03-14 12:22:06 PDT
(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"
Comment 43 Nate Chapin 2011-03-17 15:09:43 PDT
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 44 Brady Eidson 2011-03-21 16:47:02 PDT
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 45 WebKit Commit Bot 2011-03-21 19:33:02 PDT
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>
Comment 46 WebKit Commit Bot 2011-03-21 19:33:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 WebKit Commit Bot 2011-03-21 19:39:49 PDT
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.
Comment 48 Adam Roben (:aroben) 2011-03-22 06:39:11 PDT
The new tests are failing on Windows: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r81635%20(10626)/results.html
Comment 49 Adam Roben (:aroben) 2011-03-22 06:57:33 PDT
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.
Comment 50 Adam Roben (:aroben) 2011-03-22 07:08:32 PDT
I fixed the tests in r81665.