Bug 38690 – Submitting a POST that leads to a server redirect causes all cached items to redownload

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Henry]

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.

[Kristof Neirynck]

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

[Kristof Neirynck]

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

[Alexey Proskuryakov]

<rdar://problem/8132262>

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Created attachment 60413 [details]
test case

Same test case, slightly modified to not rely on Apache DirectoryIndex setting.

[Darin Fisher (:fishd, Google)]

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

[Kristof Neirynck]

The problem persists on iOS 4.0.1

[Kristof Neirynck]

The problem persists on iOS 4.1

[Kristof Neirynck]

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.

[Nate Chapin]

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.

[Chris March]

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

[Kristof Neirynck]

@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

[Nate Chapin]

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

[Brady Eidson]

Comment on attachment 78757 [details]
possible patch

Let's not revert this quite yet when we don't know what the consequences are.

[Nate Chapin]

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

[Alexey Proskuryakov]

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.

[Brady Eidson]

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

[Nate Chapin]

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

[Alexey Proskuryakov]

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

[Nate Chapin]

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

[Alexey Proskuryakov]

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.

[Kristof Neirynck]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Brady Eidson]

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

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

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.

[Brady Eidson]

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

[Brady Eidson]

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

I meant "zero regression policy."

[Darin Fisher (:fishd, Google)]

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

[Nate Chapin]

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

[Nate Chapin]

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

[Alexey Proskuryakov]

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

[Nate Chapin]

Created attachment 83584 [details]
patch #2

[Alexey Proskuryakov]

Comment on attachment 83584 [details]
patch #2

No ChangeLogs?

[Nate Chapin]

(In reply to comment #36)
> (From update of attachment 83584 [details])
> No ChangeLogs?

Wow, sorry about that.  Will re-upload momentarily.

[Nate Chapin]

Created attachment 83587 [details]
with ChangeLogs this time

[Eric Seidel (no email)]

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?

[Nate Chapin]

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

[Brady Eidson]

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.

[Brady Eidson]

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

[Nate Chapin]

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.

[Brady Eidson]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 86102 [details]:

http/tests/media/video-cancel-load.html bug 56797 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

I fixed the tests in r81665.