Bug 22194 – REGRESSION: Dialog when going back to a page from whence you submitted a form

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

[Gavin Sherlock]

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

[Alexey Proskuryakov]

Confirmed as a regression.

[Alexey Proskuryakov]

<rdar://problem/6388378>

[Brady Eidson]

This regressed in http://trac.webkit.org/changeset/37317

[Adam Barth]

I can look at this in the next few days.  Let me know if this issue requires a quicker response.

[Brady Eidson]

I'm exploring it now.  We'll see if anything sticky gets in my way of figuring it out.

[Brady Eidson]

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.

[Brady Eidson]

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. 

[Brady Eidson]

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?

[Adam Barth]

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

[Brady Eidson]

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!

[Brady Eidson]

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.

[Adam Barth]

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

[Brady Eidson]

Oh that's how we all feel about it, be assured...  ;)

[Brady Eidson]

Layout test for this is requiring some gnarly DRT changes, which is why there's no patch yet.  Coming pretty soon.

[Brady Eidson]

Created attachment 25906 [details]
Proposed WebCore change, DRT enhancements, and layout test changes

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 25907 [details]
Updated Changelog and Windows skipped list

[Adam Barth]

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

[Darin Adler]

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

[Brady Eidson]

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

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/39178

Will keep bug open for upcoming Windows changes

[Brady Eidson]

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

[Darin Adler]

Comment on attachment 25931 [details]
Windows DRT and unload *most* of the skipped list entries I just added

r=me

[Brady Eidson]

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

[Mark Rowe (bdash)]

Can this be closed with a more specific bug filed about the work that remains?

[Gavin Sherlock]

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?

[Gavin Sherlock]

Unfortunately I'm unable to determine whether the behavior described in comment #26 is a regression due to bug 23342 which is a WONTFIX :-(

[Gavin Sherlock]

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?

[Alexey Proskuryakov]

Please file a new bug - this one is fixed, and should be marked as such.

[Gavin Sherlock]

I have filed bug 23505, and resolved this as fixed.