Bug 10199 – returning to a POST result within a frame does a GET instead of a POST

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Trey Matteson]

Go to http://www.teachingperspectives.com/html/tpi_frames.htm.  Hit "Begin the TPI" then hit "Take the TPI now".  You should now have a long form displayed.

Pick anything in the first radio group section.  Pick any random answers for the first 3 questions.  Now hit the submit button near the bottom.  Note that it complains about questions 4-45 being unanswered.

Go now to any other page (google.com).  Hit Back.  Note that it is now complaining about all questions being unanswered.  The reason is that instead of reperforming the POST that originally generated that frame's content (which would also have involved presenting the "Do you really want to rePOST nag panel"), Safari did a GET of the same URL, which passed none of the original form params.

Firefox does a rePOST (including the nag).

This is on 10.4.7, and I have strong reason to believe TOT behaves the same way.  Not sure if this case ever worked.  Might have.

(FWIW, in the non-frames case, assuming the POST result has "Cache-Control: no-store, no-cache, must-revalidate", FireFox does the same thing as in this frames case, Safari just redisplays the POST result using data from the b/f cache.  This is behaving as originally intended, in an effort to avoid the miserable UE of the repost nag.  If you turn off the b/f cache, Safari nags and reposts, same as FFox.  If you let the POST result be cached, they both Safari and FFox will redisplay the POST result without any nagging or refetching.)

[Trey Matteson]

I will be checking in some automated tests that are blocked by this bug.  When fixed, look through LayoutTests/http/tests/navigation/*disabled for this bug number.

[Alexey Proskuryakov]

I don't understand why cache control directives should affect b/f cache at all. This appears to be explicitly prohibited by the HTTP spec itself: <http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.13>.

[Alexey Proskuryakov]

<rdar://problem/5938020>

[Mihai Parparita]

Darin and I just ran into this while investigating another bug. Doing a GET instead of a POST seems like pretty bad/confusing behavior, I'll look into fixing this.

[Mihai Parparita]

It looks like this happens because of this part of FrameLoader::loadURLIntoChildFrame where we replace the URL we're about to load with the URL from a history item (if we find one that matches the frame):

// If we're moving in the back/forward list, we might want to replace the content
// of this child frame with whatever was there at that point.
if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType)) {
    HistoryItem* childItem = parentItem->childItemWithTarget(childFrame->tree()->uniqueName());
    if (childItem) {
        // Use the original URL to ensure we get all the side-effects, such as
        // onLoad handlers, of any redirects that happened. An example of where
        // this is needed is Radar 3213556.
        workingURL = KURL(ParsedURLString, childItem->originalURLString());
        childLoadType = loadType;
        childFrame->loader()->history()->setProvisionalItem(childItem);
    }
}
(from http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp#L928)

This ignores the presence of FormData on the childItem, which should make this into a POST.

I think the right thing is to use a similar codepath to FrameLoader::navigateToDifferentDocument (ideally by refactoring that) which creates a POST request if the HistoryItem has FormData.

[Mihai Parparita]

Would any Apple person mind looking at Radar 3213556 (mentioned in the snippet in comment 5) and see what the test case was? For normal cases where we do a navigation to a HistoryItem (e.g. FrameLoader::navigateToDifferentDocument) we don't use the original URL, so I was wondering why we do use it for the case where we load it into a child frame.

[Mihai Parparita]

Created attachment 74813 [details]
Patch

[Mihai Parparita]

+Adam and Nate since this is Loader Land.

[Adam Barth]

Comment on attachment 74813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74813&action=review

> LayoutTests/http/tests/navigation/post-frames-goback1.html:12
> +onload = function()

please use "window.onload =" so we can run these tests in other browsers.

> LayoutTests/http/tests/navigation/post-frames-goback1.html:22
> +        setTimeout(function() {document.getElementById('the-form').submitwithpost.click();}, 0);

You can just call "submit()" on the for itself.

> WebCore/loader/FrameLoader.cpp:923
> +    

I'd skip this blank line, but whatever.

> WebCore/loader/FrameLoader.cpp:927
> +    if (subframeArchive) {
> +        childFrame->loader()->loadArchive(subframeArchive.release());
> +        return;
> +    }

I'm not sure I quite fully understand why you moved the archive logic above the stanza below, but archives are pretty mysterious to me.

[Adam Barth]

(In reply to comment #6)
> Would any Apple person mind looking at Radar 3213556 (mentioned in the snippet in comment 5) and see what the test case was? For normal cases where we do a navigation to a HistoryItem (e.g. FrameLoader::navigateToDifferentDocument) we don't use the original URL, so I was wondering why we do use it for the case where we load it into a child frame.

I wanted to R+ this change, but I don't have enough information about the above.  It seems like we should be consistent about either using the original or final URL when doing history navigations, but it's unclear to me which is best.  (It's an expectation game with sites.)  Maybe test that question in other browsers to see what they do?  I'd expect the final URL to be the right thing, but I don't really know.

[Alexey Proskuryakov]

<rdar://problem/3213556> was a problem on an important site - and at the time (early 2003), Firefox had the same problem. I can't reproduce it with the original URL or the saved sources, but the explanation makes sense.

This site had a subframe (say, sub.html) that was setting a global variable in top frame, and then navigated itself to blank.html. Top frame's onload function needed the variable. On reload we used to immediately load blank.html, bypassing the side effect of loading sub.html.

Note that the original fix for <rdar://problem/3213556> broke going back to IMDB search results, because we weren't saving form submission data. The follow-up fix was in <rdar://problem/3245625>, and I didn't try to hunt down its revision number.

> It seems like we should be consistent

Sounds like we should load original URL for both reloads and history loads to be consistent with first load case. But testing in other browsers is essential nonetheless.

[Mihai Parparita]

This is a test case of the history behavior: http://persistent.info/webkit/test-cases/crbug-32815/initial-vs-final-url.html. If you press "go away (and come back)", you will either see "initial frame contents loaded" (original URL was used) or "subsequent page" (final URL was used) alerted first.

Current behavior:
- Chrome dev channel: initial URL
- Safari 5.0.2: initial URL
- WebKit ToT: initial URL
- Firefox 3.6: final URL
- Firefox 4.0b7: final URL
- Opera 10.63: final URL (I disabled their fastback cache in the prefs to make sure it wasn't hitting that)
- IE 8.0 on Windows 7: final URL

Similarly, here's the behavior when hitting reload on that URL (without going away and coming bac):
- Chrome dev channel: initial URL
- Safari 5.0.2: initial URL
- WebKit ToT: initial URL
- Firefox 3.6: final URL
- Firefox 4.0b7: final URL
- Opera 10.63: initial URL
- IE 8.0 on Windows 7: final URL

With my patch, WebKit would use the final URL for going back (which would bring us in line with all the other browsers), but still use the initial URL for reloads. Is there a preference about making those consistent?

[Mihai Parparita]

Created attachment 75076 [details]
Patch

[Mihai Parparita]

(In reply to comment #9)
> > LayoutTests/http/tests/navigation/post-frames-goback1.html:12
> > +onload = function()
> 
> please use "window.onload =" so we can run these tests in other browsers.

As discussed, this actually works in all browsers.
 
> > LayoutTests/http/tests/navigation/post-frames-goback1.html:22
> > +        setTimeout(function() {document.getElementById('the-form').submitwithpost.click();}, 0);
> 
> You can just call "submit()" on the for itself.

Changed.

> > WebCore/loader/FrameLoader.cpp:923
> > +    
> 
> I'd skip this blank line, but whatever.

Removed.
 
> > WebCore/loader/FrameLoader.cpp:927
> > +    if (subframeArchive) {
> > +        childFrame->loader()->loadArchive(subframeArchive.release());
> > +        return;
> > +    }
>
> I'm not sure I quite fully understand why you moved the archive logic above the stanza below, but archives are pretty mysterious to me.

The HistoryItem and archive paths seemed to be entirely independent, I was just cleaning up this code (the archive path seemed simpler, so I put it before the history one).

(In reply to comment #10)
> I wanted to R+ this change, but I don't have enough information about the above.  It seems like we should be consistent about either using the original or final URL when doing history navigations, but it's unclear to me which is best.  (It's an expectation game with sites.)  Maybe test that question in other browsers to see what they do?  I'd expect the final URL to be the right thing, but I don't really know.

As mentioned in comment 12, switching to use the final URL would make us consistent with other browsers, so it seems like a step in the right direction. No layout tests failed when I made this change, so I added fast/history/history-back-initial-vs-final-url.html to explicitly test for it.

[Alexey Proskuryakov]

Created attachment 75081 [details]
test case

> As mentioned in comment 12, switching to use the final URL would make us
> consistent with other browsers

Hmm, I cannot reproduce this. On my test, IE matches WebKit (and common sense), so this looks like a pure Firefox bug.

To test: unpack the attachment, open test.cpp, try reloading it or navigating away and back. In WebKit and IE, the document shows a "bar" alert every time it's loaded or reloaded.

[Adam Barth]

Comment on attachment 75076 [details]
Patch

Hum...  The evidence seems to be that we want to use the final URL in this case.  I'm inclined to make this change and be on the lookout for broken sites, but I'm pretty biased towards matching other browsers.  I'm going to mark this R+, but give Alexey some time to respond before landing in case he thinks we should do something different.

Thanks Alexey for digging the info out of rdar and thanks Mihai for cross-testing.

[Adam Barth]

Comment on attachment 75076 [details]
Patch

Oops.  Just saw Alexey's comment.

[Mihai Parparita]

(In reply to comment #15)
> Created an attachment (id=75081) [details]
> test case
> 
> > As mentioned in comment 12, switching to use the final URL would make us
> > consistent with other browsers
> 
> Hmm, I cannot reproduce this. On my test, IE matches WebKit (and common sense), so this looks like a pure Firefox bug.
> 
> To test: unpack the attachment, open test.cpp, try reloading it or navigating away and back. In WebKit and IE, the document shows a "bar" alert every time it's loaded or reloaded.

I copied this to: http://persistent.info/webkit/test-cases/crbug-32815/ap-test.html. In sub.html, you're navigating to about:blank, which is generally not allowed, so IE is most likely not using it as a final URL for the history item. 

I modified the test case to navigate to another dummy page instead (ap-sub-other.html): http://persistent.info/webkit/test-cases/crbug-32815/ap-test-2.html

In this case, IE displays a script error saying that foo is not defined (as expected) when coming back from another page.

[Alexey Proskuryakov]

> so IE is most likely not using it as a final URL for the history item.

I did test that fames[0].document.URL was "about:blank" after loading my original test case.

[Mihai Parparita]

(In reply to comment #19)
> > so IE is most likely not using it as a final URL for the history item.
> 
> I did test that fames[0].document.URL was "about:blank" after loading my original test case.

OK, IE does seem to navigate to the about:blank URL, but it doesn't record it as the final URL in their HistoryItem equivalent. However, given IE's behavior on http://persistent.info/webkit/test-cases/crbug-32815/ap-test-2.html, do you agree that with regular URLs IE matches Firefox?

[Alexey Proskuryakov]

Perhaps - it's confusing now.

I wonder if there is a logical reason to use the final URL for subframes when reloading.

[Mihai Parparita]

(In reply to comment #21)
> I wonder if there is a logical reason to use the final URL for subframes when reloading.

If it's a frame that was POSTed to, which then did a redirect to a regular URL in the response (to avoid the "do you want to resubmit" warning), then I assume we'd want to use the final URL.

[Mihai Parparita]

Thus far I'm still unable to find a case (besides about:blank - http://persistent.info/webkit/test-cases/crbug-32815/about-blank/initial-vs-final-url.html) where IE uses the final URL when going back.

I hypothesized that cacheability might explain the difference in behavior in IE between using about:blank and a regular page, but the test case at http://persistent.info/webkit/test-cases/crbug-32815/cacheable/initial-vs-final-url.html (where everything is cacheable for a week) seems to behave the same (final URL is used when coming back).

Alexey sent me a copy of the site from <rdar://problem/3213556>. It looks like it's doing the navigation with location.replace during a frame's onload handler. I made a modified test case that does that at: http://persistent.info/webkit/test-cases/crbug-32815/location-replace/initial-vs-final-url.html. In that case, IE is still using the final URL.

The actual site with the problem from the Radar bug was http://www.halcyondays.co.uk/viewer.shtml. IE 8 has the same behavior as WebKit (with my patch) and Firefox. If I load the page, go to google.com and come back*, the logo is not displayed (it is displayed when doing a reload, which as far as I can tell was the problem that the Radar bug was about). Given that we're mirroring the behavior of all the other browsers, I don't see how this could lead to compat problems.

* with the page cache/backforward cache disabled

[Mihai Parparita]

(In reply to comment #23)
> Thus far I'm still unable to find a case (besides about:blank - http://persistent.info/webkit/test-cases/crbug-32815/about-blank/initial-vs-final-url.html) where IE uses the final URL when going back.

This should have said "where IE uses the initial URL (for frames) when going back."

[Alexey Proskuryakov]

I guess we have to match other browsers then, even though that regresses <rdar://problem/3213556>. Still wondering why about:blank is different in IE, and whether there are other cases like that, but I'm not sure how to find an answer to that.

Do all the new tests in the bug pass in both IE and Firefox?

[Alexey Proskuryakov]

s/in the bug/in the patch/

[Mihai Parparita]

(In reply to comment #25)
> Do all the new tests in the patch pass in both IE and Firefox?

fast/history/history-back-initial-vs-final-url.html passes everywhere. The ones from http/tests/navigation work in IE, but don't seem to in Firefox since it just uses the initial URL of the frame (about:blank) when going back, instead of resubmitting the POST request.

[Mihai Parparita]

Comment on attachment 75076 [details]
Patch

Any other issues that would prevent this from being landed?

[Alexey Proskuryakov]

I don't have any other concerns (note that I didn't review the patch).

[Adam Barth]

Comment on attachment 75076 [details]
Patch

Thanks for investigating the behavior of other browsers and the history behind our behavior.  It's very valuable to be careful when changing these sort of subtle behaviors.  It sounds we've come to agreement about what behavior we want and this patch looks like a good way of achieving that behavior.  Thanks everyone.

[WebKit Commit Bot]

Comment on attachment 75076 [details]
Patch

Rejecting patch 75076 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
/XPathNamespace.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/XPathParser.o /Projects/CommitQueue/WebCore/xml/XPathParser.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/XPathPath.o /Projects/CommitQueue/WebCore/xml/XPathPath.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(6 failures)


Full output: http://queues.webkit.org/results/6746099

[Mihai Parparita]

Comment on attachment 75076 [details]
Patch

Seems like a bot issue:

error: couldn't run '/Developer/Library/PrivateFrameworks/DevToolsCore.framework/Resources/xcexec' because an internal error occurred (Resource temporarily unavailable)

Trying again.

[Mihai Parparita]

The commit queue seems to be refusing to pick up this patch again, will land by hand.

[Mihai Parparita]

Committed r73486: <http://trac.webkit.org/changeset/73486>

[Eric Seidel (no email)]

The queue infrastructure all died today due to a critical machine running out of disk. Should be back functioning normal shortly.