Steps to reproduce: 1. Go to bug URL 2. Enter: watson jd[AU] AND crick fh[AU] into search box, and press GO 3. Click on the first result: Molecular structure of nucleic acids: a structure for deoxyribose nucleic acid. 4. Once page has loaded, hit the back button in the browser Expected results: You should simply return to the previous page, with no additional interaction needed. Actual results: You get a dialog that says: "Are you sure you want to send a form again? To open this page again, Safari must resend the form you completed to open the page the first time. This may cause the website to repeat actions it took the first time you sent the form." Moreover, it does it twice! This is a regression from Safari 3.1.2. Tested with r38240 on OS X 10.5.5
Confirmed as a regression.
<rdar://problem/6388378>
This regressed in http://trac.webkit.org/changeset/37317
I can look at this in the next few days. Let me know if this issue requires a quicker response.
I'm exploring it now. We'll see if anything sticky gets in my way of figuring it out.
In FrameLoader::loadItem(), ResourceHandle::willLoadFromCache() is called. On Mac, it asks NSURLConnection if the request would load from the Foundation cache or not. Before 37317, this returned YES, as the resource would be found in the NSURLCache. After 37317, this returns no. Without further exploration, it feels to me like when the page is originally requested, maybe the ResourceRequest does *not* have the origin header set and when we go back, it does get set. Or vice versa.
I was wrong - the difference is the change wrt adding the other extra header fields. Previously, when the "willLoadFromCache()" check was performed, all extra headers had been added to the request already, such as Accept and User-Agent 37317 changed this, so when the willLoadFromCache check was made, the headers were not complete and it resulted in a cache miss. This shouldn't be too hard to fix.
In fact, that seems like a very peculiar part of the original patch, changing the order in which addExtraFieldsToRequest() is called for FrameLoader::loadItem(). I'm going to change it back and run the layout tests to make sure nothing there regresses, but maybe you can chime in with the motivation for that change, Adam?
> I'm going to change it back and run the layout tests to make sure nothing there > regresses, but maybe you can chime in with the motivation for that change, > Adam? The issue is the order that addHTTPOriginIfNeeded and addExtraFieldsToRequest are called. In every other case, they are called in that order. If you revert the move they'll be called in the opposite order. Both functions set the Origin header, but they try to be polite and defer to someone who has already decided what the right value is. addHTTPOriginIfNeeded is smarter and more likely to get the right answer. addExtraFieldsToRequest is the fail-safe that stops us from sending a bad (i.e. non-existent) Origin header for POST requests. Can we order the function so that these methods are called in this order: 1) addHTTPOriginIfNeeded 2) addExtraFieldsToRequest 3) willLoadFromCache It's important that setHTTPMethod("POST") occurs before addHTTPOriginIfNeeded because addHTTPOriginIfNeeded needs to know the correct method before deciding if the Origin header is needed.
Thanks for the explanation! Your ordering suggestion should work fine - testing it right now. That seems kinda fragile and is kinda annoying, but I don't have a better way to fix it, so I might just add a comment to explain it referencing back to this bug. Hopefully a patch will be coming shortly - getting a layout test for this one will be more work than the fix itself - oh well!
Yup, that ordering works just fine. I'm going to either see if DRT already supports catching this case, or hack on it so it does, then post a patch.
> That seems kinda fragile and is kinda annoying, but I don't have a better way > to fix it That's how I feel about most of the FrameLoader code. :) My dream is to some day reorganize this code to be more understandable and robust.
Oh that's how we all feel about it, be assured... ;)
Layout test for this is requiring some gnarly DRT changes, which is why there's no patch yet. Coming pretty soon.
Created attachment 25906 [details] Proposed WebCore change, DRT enhancements, and layout test changes
Comment on attachment 25906 [details] Proposed WebCore change, DRT enhancements, and layout test changes I just realized this will break already working tests on windows until the policy delegate is updated there. qt and gtk already don't run these tests. So I will at least add the changed tests to the skipped list for windows before checking in, then remove them when I make the windows change.
Created attachment 25907 [details] Updated Changelog and Windows skipped list
The FrameLoader.cpp change looks fine. I didn't try to understand the rest of the patch. (Note that I'm not an official reviewer.)
Comment on attachment 25907 [details] Updated Changelog and Windows skipped list > + // See https://bugs.webkit.org/show_bug.cgi?id=22194 for more discussion Should add a period here at the end of the sentence. > +# Needs custom policy delegate enhancement in DRT It's annoying to turn off so many tests on Windows. Is there a way to trigger the new behavior only for your new test for now? How hard is it to do the Windows side of this? > + switch (navType) { > + case WebNavigationTypeLinkClicked: > + typeDescription = "WebNavigationTypeLinkClicked"; > + break; > + case WebNavigationTypeFormSubmitted: > + typeDescription = "WebNavigationTypeFormSubmitted"; > + break; > + case WebNavigationTypeBackForward: > + typeDescription = "WebNavigationTypeBackForward"; > + break; > + case WebNavigationTypeReload: > + typeDescription = "WebNavigationTypeReload"; > + break; > + case WebNavigationTypeFormResubmitted: > + typeDescription = "WebNavigationTypeFormResubmitted"; > + break; > + case WebNavigationTypeOther: > + typeDescription = "WebNavigationTypeOther"; > + break; > + default: > + typeDescription = "Unknown"; > + } For the layout test results I'd suggest using strings that aren't part of the Mac WebKit API, so "link clicked" rather than "WebNavigationTypeLinkClicked". I think it might also make the test results easier to read. I also think "Unknown" is slightly too mild when we get an constant value that's not one of the defined ones. Since we already have "other", it's particularly bad if we get a value that's not one of the legal values passed in. r=me as is; we can consider improvements in the testing machinery later, but lets get the fix in right away
Period added, and strings for the output values changed. Windows DRT changes will be very easy to follow up with - like, I'm almost done with them now that I have my windows machine in front of me :)
Landed in http://trac.webkit.org/changeset/39178 Will keep bug open for upcoming Windows changes
Created attachment 25931 [details] Windows DRT and unload *most* of the skipped list entries I just added Windows DRT and unload *most* of the skipped list entries I just added. The reason the new test is still skipped is because something about how I made the test doesn't agree with cygwin's apache and requires a little more exploration
Comment on attachment 25931 [details] Windows DRT and unload *most* of the skipped list entries I just added r=me
Landed in http://trac.webkit.org/changeset/39191 Still have to figure out the httpd vs layout test issue on Windows before this can be closed for good
Can this be closed with a more specific bug filed about the work that remains?
I still see this problem on another site - the bug url works fine, but this other site shows exactly the bug behavior. Unfortunately, the other site is password restricted (Stanford's grading system), so I don't know exactly what to do to create something reproducible that other's can access. I just wanted to add this to the bug report to make it clear that the problem isn't entirely solved. I tested this in r39882, but this particular site also exhibits the problem in Safari 3.2.1. When I get home from work this evening, I'll see if it's a regression from earlier Webkit nightlies, which might narrow down the problem. Should I open a new bug for this?
Unfortunately I'm unable to determine whether the behavior described in comment #26 is a regression due to bug 23342 which is a WONTFIX :-(
Ok - I found an example on a publicly accessible website where this behavior still occurs, using r40102. Steps to reproduce: 1. Go to: http://www.prophyler.org/cgi-bin/search_form.cgi 2. Enter a value in the box under "Search by UniProt Accession", such as P01023 3. Hit SEARCH 4. After the resulting page has loaded, hit the back button in the browser. You get the "Are you sure you want to send a form again?" dialog twice. Note, the original bug URL, which was fixed, still works correctly, so this must have a slightly different cause. I am unable to determine if it is a regression due to bug 23342, though this behavior is present in Safari 3.2.1. Should I file this example as a new bug, or can it remain tracked here?
Please file a new bug - this one is fixed, and should be marked as such.
I have filed bug 23505, and resolved this as fixed.