Bug 38690 - Submitting a POST that leads to a server redirect causes all cached items to redownload
: Submitting a POST that leads to a server redirect causes all cached items to ...
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-05-06 14:21 PST by
Modified: 2011-03-22 07:08 PST (History)


Attachments
Minimal testcase (requires apache server with php) (2.77 KB, application/octet-stream)
2010-06-24 05:20 PST, Kristof Neirynck
no flags Details
test case (4.37 KB, application/zip)
2010-07-02 15:38 PST, Alexey Proskuryakov
no flags Details
possible patch (6.93 KB, patch)
2011-01-12 16:52 PST, Nate Chapin
beidson: review-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
with ChangeLogs this time (12.39 KB, patch)
2011-02-23 17:14 PST, Nate Chapin
beidson: review-
Review Patch | Details | Formatted Diff | Diff
Set the flag when QuickLook is loaded (17.19 KB, patch)
2011-03-17 15:09 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-06 14:21:45 PST
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 From 2010-06-24 05:18:35 PST -------
I can confirm this problem and have created a minimal testcase.
See it live here 
http://crydust.be/lab/prg/
------- Comment #2 From 2010-06-24 05:20:14 PST -------
Created an attachment (id=59645) [details]
Minimal testcase (requires apache server with php)
------- Comment #3 From 2010-06-25 12:45:49 PST -------
<rdar://problem/8132262>
------- Comment #4 From 2010-07-02 15:32:30 PST -------
Confirmed with r62300. Also reproducible with Safari 4.0.5 on Leopard.
------- Comment #5 From 2010-07-02 15:38:57 PST -------
Created an attachment (id=60413) [details]
test case

Same test case, slightly modified to not rely on Apache DirectoryIndex setting.
------- Comment #6 From 2010-07-08 14:39:44 PST -------
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 From 2010-07-16 01:25:10 PST -------
The problem persists on iOS 4.0.1
------- Comment #8 From 2010-09-14 01:29:15 PST -------
The problem persists on iOS 4.1
------- Comment #9 From 2010-11-29 07:11:59 PST -------
The problem persists on iOS 4.2 (and the latset version of chrome and safari)
------- Comment #10 From 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 From 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 From 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 From 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 From 2011-01-12 16:52:59 PST -------
Created an attachment (id=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 From 2011-01-13 08:27:08 PST -------
(From update of attachment 78757 [details])
Let's not revert this quite yet when we don't know what the consequences are.
------- Comment #16 From 2011-01-13 11:40:01 PST -------
(In reply to comment #15)
> (From update of attachment 78757 [details] [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 From 2011-01-13 11:54:32 PST -------
(From update of attachment 78757 [details])
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 From 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 From 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 From 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 From 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 From 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 From 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 From 2011-02-07 10:51:43 PST -------
(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.

> 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 From 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 From 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 From 2011-02-07 14:08:24 PST -------
Created an attachment (id=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 From 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 From 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 From 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 From 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 From 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 From 2011-02-22 16:27:35 PST -------
(In reply to comment #24)
> (From update of attachment 78757 [details] [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 From 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 From 2011-02-23 17:01:43 PST -------
Created an attachment (id=83584) [details]
patch #2
------- Comment #36 From 2011-02-23 17:08:55 PST -------
(From update of attachment 83584 [details])
No ChangeLogs?
------- Comment #37 From 2011-02-23 17:10:17 PST -------
(In reply to comment #36)
> (From update of attachment 83584 [details] [details])
> No ChangeLogs?

Wow, sorry about that.  Will re-upload momentarily.
------- Comment #38 From 2011-02-23 17:14:30 PST -------
Created an attachment (id=83587) [details]
with ChangeLogs this time
------- Comment #39 From 2011-02-24 02:37:52 PST -------
(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?
------- Comment #40 From 2011-02-24 08:34:00 PST -------
(In reply to comment #39)
> (From update of attachment 83587 [details] [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 From 2011-03-14 12:21:16 PST -------
(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.
------- Comment #42 From 2011-03-14 12:22:06 PST -------
(In reply to comment #41)
> (From update of attachment 83587 [details] [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 From 2011-03-17 15:09:43 PST -------
Created an attachment (id=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 From 2011-03-21 16:47:02 PST -------
(From update of attachment 86102 [details])
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 From 2011-03-21 19:33:02 PST -------
(From update of attachment 86102 [details])
Clearing flags on attachment: 86102

Committed r81635: <http://trac.webkit.org/changeset/81635>
------- Comment #46 From 2011-03-21 19:33:07 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #47 From 2011-03-21 19:39:49 PST -------
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 From 2011-03-22 06:39:11 PST -------
The new tests are failing on Windows: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r81635%20(10626)/results.html
------- Comment #49 From 2011-03-22 06:57:33 PST -------
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 From 2011-03-22 07:08:32 PST -------
I fixed the tests in r81665.