Bug 25223 - REGRESSION: Back button after form submission to the same URL fails to navigate
Summary: REGRESSION: Back button after form submission to the same URL fails to navigate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL: http://www.uni-nets.com/chrometests/b...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-15 16:29 PDT by Darin Fisher (:fishd, Google)
Modified: 2009-04-24 14:47 PDT (History)
0 users

See Also:


Attachments
v1 patch (6.09 KB, patch)
2009-04-16 10:53 PDT, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp (6.84 KB, patch)
2009-04-16 10:58 PDT, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2009-04-15 16:29:28 PDT
Back button after form submission to the same URL fails to navigate

This has been broken for a while.  I can confirm it in Safari 4 beta as well as WebKit nightly r42377.

The bug occurs when a form posts to the same URL as the page that contains the form.  Upon going back, FrameLoader thinks that it should scroll the page since the URLs match and the page we are going to does not have any FormData.  The problem is that it fails to check if the page we are leaving has FormData, which it does in the case I've described.
Comment 1 Darin Fisher (:fishd, Google) 2009-04-15 16:29:57 PDT
Here's the corresponding Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=8976
Comment 2 Darin Fisher (:fishd, Google) 2009-04-15 16:51:35 PDT
Chrome 1.0 and Safari 3.2 do not have this bug.
Comment 3 Darin Fisher (:fishd, Google) 2009-04-16 10:53:45 PDT
Created attachment 29537 [details]
v1 patch

The fix is a very simple change to FrameLoader.  I verified that this does not regress any existing layout tests.

However, in order to write a test for this, I discovered a bug in the WorkQueue implementation.  This issue is detailed in the WebKitTools/ChangeLog portion of my patch.
Comment 4 Darin Fisher (:fishd, Google) 2009-04-16 10:58:04 PDT
Created attachment 29538 [details]
v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp
Comment 5 Darin Adler 2009-04-17 06:30:50 PDT
Comment on attachment 29538 [details]
v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp

>      // We also do not do anchor-style navigation if we're posting a form.
>      
>  #if ENABLE(WML)
> -    if (!formData && urlsMatchItem(item) && !m_frame->document()->isWMLDocument()) {
> +    if (!formData && !m_currentHistoryItem->formData() && urlsMatchItem(item) && !m_frame->document()->isWMLDocument()) {
>  #else
> -    if (!formData && urlsMatchItem(item)) {
> +    if (!formData && !m_currentHistoryItem->formData() && urlsMatchItem(item)) {
>  #endif
>          // Must do this maintenance here, since we don't go through a real page reload
>          saveScrollPositionAndViewStateToItem(m_currentHistoryItem.get());

It seems that if you're changing the code you also should change the comment before it, which describes the condition in a high-level way.

Not your fault, and maybe best to not do this in this patch, but that WMLDocument check is done in a particuarly ugly way. Maybe we could just set up a boolean before the if statement instead of repeating the whole if statement twice. The extra open brace inside an #if also tends to confuse syntax highlighters.

> +    // if another load started, then wait for it to complete.
> +    if (topLoadingFrame != nil)
> +        return;

Normally we wouldn't say != nil here.

r=me
Comment 6 Darin Fisher (:fishd, Google) 2009-04-17 13:16:11 PDT
> It seems that if you're changing the code you also should change the comment
> before it, which describes the condition in a high-level way.

Yes, will do.


> Not your fault, and maybe best to not do this in this patch, but that
> WMLDocument check is done in a particuarly ugly way. Maybe we could just set up
> a boolean before the if statement instead of repeating the whole if statement
> twice. The extra open brace inside an #if also tends to confuse syntax
> highlighters.

I agree.  I spent a little time twiddling with this before uploading the patch, but I
wasn't happy with the results.  This current form is surprisingly readable since the
variant is last.  That said, duplicating code sucks.  I'll change it.


> > +    // if another load started, then wait for it to complete.
> > +    if (topLoadingFrame != nil)
> > +        return;
> 
> Normally we wouldn't say != nil here.

Good to know.  I'm not very familiar with ObjC coding conventions.
Comment 7 Darin Fisher (:fishd, Google) 2009-04-17 13:31:52 PDT
Landed as:  http://trac.webkit.org/changeset/42623
Comment 8 Darin Adler 2009-04-24 14:47:55 PDT
Lack of a null check in this patch caused bug 25355.