HistoryItem not updated properly when a form submission begins before a previous form submission has finished

[Darin Fisher (:fishd, Google)]

HistoryItem not updated properly when a form submission begins before a previous form submission has finished

Given content such as the following:

a.php:
  <form method="post" action="b.php">...</form>

b.php:
  <form method="post" action="c.php">...</form>
  <script>document.forms[0].submit()</script>

c.php:
  <b>hello world</b>

The submission to c.php that occurs while b.php is still loading (following the form submission from a.php to b.php) results in WebKit failing to properly update the HistoryItem.  While the browser will think it is on c.php, navigating away from c.php and then back again will actually cause a form submission to b.php to be repeated!

I believe this occurs because FrameLoader::loadPostRequest() fails to call DocumentLoader::setIsClientRedirect() in the same manner that FrameLoader::loadURL() does.  It should do so after calling loadWithNavigationAction() based on the prior value of m_quickRedirectComing.  As a result, HistoryController::updateForRedirectWithLockedBackForwardList() does not see the isClientRedirect() flag set to true, and it thus fails to call updateCurrentItem().

Incidentally, this bug does not exist if the testcase is placed in a subframe.  That appears to be due to the code in updateForRedirectWithLockedBackForwardList(), which will re-create the HistoryItems for subframes only.  I'm not sure what to make of that.

[Darin Fisher (:fishd, Google)]

Created attachment 131357 [details]
layout tests

Here's a pair of layout tests for this bug.  One uses the GET method and the other uses the POST method.  WebKit currently fails the POST test.  In both cases, the session history entry for the initial page should be replaced with a session history entry for the second page, but that doesn't happen in the POST case.

[Darin Fisher (:fishd, Google)]

The error diff for the POST test looks like this:

 ============== Back Forward List ==============
-        (file test):fast/history/resources/form-submission-before-load-page2.html  **nav target**
+        (file test):fast/history/form-submission-before-load-post.html  **nav target**
 curr->  (file test):fast/history/resources/form-submission-before-load-page3.html  **nav target**
 ===============================================

[Darin Fisher (:fishd, Google)]

Created attachment 132143 [details]
v1 patch

Here's a very simple patch that corrects the bug.  This copies logic from
the end of FrameLoader::loadURL().  I'm not super keen on just copying that
code, but at least it is a small amount of code.  I considered moving this
code into loadWithNavigationAction, but I couldn't convince myself that it
was correct to run it in all cases as loadWithNavigationAction gets called
elsewhere under different conditions.

I've included a layout test for the case that passed before my change,
namely that of using GET instead of POST.

[WebKit Review Bot]

Comment on attachment 132143 [details]
v1 patch

Attachment 132143 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11961070

New failing tests:
fast/loader/form-submission-before-load-get.html
fast/loader/form-submission-before-load-post.html

[Darin Fisher (:fishd, Google)]

Created attachment 132447 [details]
v2 patch

Now, with corrected test expectations.

[Darin Fisher (:fishd, Google)]

Created attachment 132665 [details]
v3 patch

With some comments in the test.

[Nate Chapin]

Comment on attachment 132665 [details]
v3 patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132665&action=review

> Source/WebCore/loader/FrameLoader.cpp:2669
> -    } else
> +    } else {
> +        // must grab this now, since this load may stop the previous load and clear this flag
> +        bool isRedirect = m_quickRedirectComing;
>          loadWithNavigationAction(workingResourceRequest, action, lockHistory, loadType, formState.release());    
> +        if (isRedirect) {
> +            m_quickRedirectComing = false;
> +            if (m_provisionalDocumentLoader)
> +                m_provisionalDocumentLoader->setIsClientRedirect(true);
> +        }
> +    }

Is there any sense in factoring this logic out into a helper? I notice it's identical to code in loadURL().

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> Is there any sense in factoring this logic out into a helper? I notice it's identical to code in loadURL().

I could imagine factoring out the body of the isRedirect branch:

...
    bool isRedirect = m_quickRedirectComing;
    loadWithNavigationAction(workingResourceRequest, action, ...);
    if (isRedirect)
        didClientRedirect();
...

void FrameLoader::didClientRedirect() {
    m_quickRedirectComing = false;
    if (m_provisionalDocumentLoader)
        m_provisionalDocumentLoader->setIsClientRedirect(true);
}

Is that what you had in mind?  Is didClientRedirect a good name for this?

[Collabora GTK+ EWS bot]

Comment on attachment 132665 [details]
v3 patch

Attachment 132665 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11991054

[Darin Fisher (:fishd, Google)]

Comment on attachment 132665 [details]
v3 patch

As discussed offline with Nate, I'm going to go ahead and commit this.  I think
refactoring is best as a separate task.

[WebKit Review Bot]

Comment on attachment 132665 [details]
v3 patch

Clearing flags on attachment: 132665

Committed r112924: <http://trac.webkit.org/changeset/112924>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.