Bug 25223 – REGRESSION: Back button after form submission to the same URL fails to navigate

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

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Here's the corresponding Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=8976

[Darin Fisher (:fishd, Google)]

Chrome 1.0 and Safari 3.2 do not have this bug.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 29538 [details]
v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp

[Darin Adler]

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

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42623

[Darin Adler]

Lack of a null check in this patch caused bug 25355.