Bug 22194 - REGRESSION: Dialog when going back to a page from whence you submitted a form
Summary: REGRESSION: Dialog when going back to a page from whence you submitted a form
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Brady Eidson
URL: http://www.ncbi.nlm.nih.gov/sites/ent...
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-11-11 20:03 PST by Gavin Sherlock
Modified: 2009-01-23 09:39 PST (History)
3 users (show)

See Also:


Attachments
Proposed WebCore change, DRT enhancements, and layout test changes (32.29 KB, patch)
2008-12-09 17:07 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Updated Changelog and Windows skipped list (31.57 KB, patch)
2008-12-09 17:42 PST, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Windows DRT and unload *most* of the skipped list entries I just added (7.90 KB, patch)
2008-12-10 17:38 PST, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Sherlock 2008-11-11 20:03:22 PST
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
Comment 1 Alexey Proskuryakov 2008-11-12 23:16:50 PST
Confirmed as a regression.
Comment 2 Alexey Proskuryakov 2008-11-20 05:48:56 PST
<rdar://problem/6388378>
Comment 3 Brady Eidson 2008-12-08 18:05:14 PST
This regressed in http://trac.webkit.org/changeset/37317
Comment 4 Adam Barth 2008-12-08 19:08:18 PST
I can look at this in the next few days.  Let me know if this issue requires a quicker response.
Comment 5 Brady Eidson 2008-12-08 19:53:52 PST
I'm exploring it now.  We'll see if anything sticky gets in my way of figuring it out.
Comment 6 Brady Eidson 2008-12-08 20:16:28 PST
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.
Comment 7 Brady Eidson 2008-12-08 20:34:35 PST
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. 
Comment 8 Brady Eidson 2008-12-08 20:41:08 PST
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?
Comment 9 Adam Barth 2008-12-08 22:30:20 PST
> 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.


Comment 10 Brady Eidson 2008-12-08 22:32:50 PST
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!
Comment 11 Brady Eidson 2008-12-08 22:51:00 PST
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.
Comment 12 Adam Barth 2008-12-08 22:52:25 PST
> 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.

Comment 13 Brady Eidson 2008-12-08 22:56:18 PST
Oh that's how we all feel about it, be assured...  ;)
Comment 14 Brady Eidson 2008-12-09 15:34:48 PST
Layout test for this is requiring some gnarly DRT changes, which is why there's no patch yet.  Coming pretty soon.
Comment 15 Brady Eidson 2008-12-09 17:07:49 PST
Created attachment 25906 [details]
Proposed WebCore change, DRT enhancements, and layout test changes
Comment 16 Brady Eidson 2008-12-09 17:24:34 PST
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.
Comment 17 Brady Eidson 2008-12-09 17:42:34 PST
Created attachment 25907 [details]
Updated Changelog and Windows skipped list
Comment 18 Adam Barth 2008-12-09 21:39:13 PST
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 19 Darin Adler 2008-12-10 09:15:10 PST
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
Comment 20 Brady Eidson 2008-12-10 09:52:10 PST
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  :)
Comment 21 Brady Eidson 2008-12-10 12:04:16 PST
Landed in http://trac.webkit.org/changeset/39178

Will keep bug open for upcoming Windows changes
Comment 22 Brady Eidson 2008-12-10 17:38:03 PST
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 23 Darin Adler 2008-12-10 17:39:34 PST
Comment on attachment 25931 [details]
Windows DRT and unload *most* of the skipped list entries I just added

r=me
Comment 24 Brady Eidson 2008-12-10 17:53:03 PST
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

Comment 25 Mark Rowe (bdash) 2009-01-12 23:35:36 PST
Can this be closed with a more specific bug filed about the work that remains?
Comment 26 Gavin Sherlock 2009-01-14 08:00:12 PST
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?
Comment 27 Gavin Sherlock 2009-01-14 22:42:32 PST
Unfortunately I'm unable to determine whether the behavior described in comment #26 is a regression due to bug 23342 which is a WONTFIX :-(
Comment 28 Gavin Sherlock 2009-01-22 13:57:18 PST
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?
Comment 29 Alexey Proskuryakov 2009-01-23 07:46:24 PST
Please file a new bug - this one is fixed, and should be marked as such.
Comment 30 Gavin Sherlock 2009-01-23 09:39:25 PST
I have filed bug 23505, and resolved this as fixed.