Cannot resubmit form after submission fails

• Component: Forms
• Status: NEW ---

[Joe Mason]

Created attachment 52790 [details]
Manual test case

If a form submission fails due to a server error, the m_submittedUrl is not cleared properly and so clicking "submit" again does nothing.

Attached is a small html file that demonstrates this problem.  You'll need to use a client that opens a dialog on error instead of rendering an error page, so that it stays on the same page after the submission fails.  I used QtLauncher, edited to turn off ErrorPageExtension (

The criteria are:
- a form which loads in the current frame (no target attribute)
- the method is GET
- the action url returns an error (in the test file, it's a nonexistant file, for a file not found error)
- the client remains on the same page after the error is handled (eg. it displays the error in a dialog instead of an error page)

When this happens, clicking Submit the first time will display the error dialog, and after this clicking Submit again will do nothing, and not submit the form until the page is reloaded.

The reason is that FrameLoader::submitForm contains this code, to guard against submitting the form multiple times:

    if (m_frame->tree()->isDescendantOf(targetFrame)) {
        if (m_submittedFormURL == u)
            return;
        m_submittedFormURL = u;
    }

It's expected that after submitting the form, m_submittedFormURL is cleared.  But the code to do this on error, in FrameLoader::receivedMainResourceError, is:

        if (m_submittedFormURL == m_provisionalDocumentLoader->originalRequestCopy().url())
            m_submittedFormURL = KURL();

When the method is GET, the form data is added to the url as a query string, which is included in m_submittedFormURL.  But originalRequestCopy().url() is the basic url included in the action parameter, before the query string is added, so this test never passes and m_submittedFormURL is not reset.

[Joe Mason]

I'm having a hard time making the attached file into a layout test.  It's easy to automatically submit the form after the page is loaded with javascript, but then the only way I can figure to wait for the response is to add a timeout, which is ugly and unreliable.  The best thing would be to catch some sort of "load failed" event and call form.submit again when it's received, but I can't find any such event.  The closest I could find was to set the form's target attribute to point to an iframe, and put an onload handler on the iframe - but that breaks the conditions of the bug, which only manifests when loading in the same frame.

[Joe Mason]

Created attachment 52794 [details]
suggested patch

This patch will fix the problem.  It seems to me that if an error is received, always clearing m_submittedFormURL should be safe.  Under what circumstances would it NOT match the original URL for a FrameLoader?

If this is too invasive, an alternative is to compare only the base URL's but not the query parts.

I'll gladly add a layout test if someone can help me figure out how.

[Darin Adler]

Comment on attachment 52794 [details]
suggested patch

Bug fixes require a test case. If you can't make an automated test case please include a manual one. But I think you can make an automated test case using http.

And I'm not sure it's safe to just clear multiple submission protection for failures that don't match the form URL. It might require more study of the patches that introduced this mechanism and some further thought about how a load of a different main resource might come through.

[Joe Mason]

Comment on attachment 52790 [details]
Manual test case

This attachment IS a manual test case

[Joe Mason]

A comment in FrameLoader.cpp says:

    // FIXME: Frame targeting is only one of the ways the submission could end
up doing something other
    // than replacing this frame's content, so this check is flawed. On the
other hand, the check is hardly
    // needed any more now that we reset m_submittedFormURL on each mouse or
key down event.

The root problem here is that, when manually submitting a form, you can click
on submit, wait for an error response, and then click on submit again and
nothing will happen.  The user would expect the form to be resubmitted in this
case.

But according to the comment above, since clicking on "submit" is a mouse press
or keydown, m_submittedFormURL should have been cleared by the user
interaction.  So now I think that's were the bug lies.

(Although the test in receivedMainResourceError is also incorrect for GET urls,
it shouldn't matter.)

[Alexey Proskuryakov]

I'm also fairly confident that this can (and should) be tested automatically. There aren't a lot of forms tests in http/tests to serve as example currently, but there are tests with resources that change their state, e.g. returning 404 on one access, and a success on another.

[Joe Mason]

The problem I'm having in creating an automated test is in deciding when to send the second submit.

It's easy to call form.submit on first load.  However:

- if we call form.submit again immediately, before FrameLoader::receivedMainResourceError has been called, m_submittedFormURL has not yet been reset so nothing happens - correctly, in this case
- we can't put an onload handler to submit again after the frame reloads, because it *doesn't* reload - we're testing an error condition
- we can't use an onerror handler as we would to test failure to load an image, because this has no meaning for forms
- we can't set the submit target to another frame and then watch the frame contents for an error message, because this bypasses the isDescendentOf check

The only thing I can think to do is to set a timeout after the first submit, and send the second submit after a reasonable time.  I can do that, I was just hoping somebody had a better suggestion

[Alexey Proskuryakov]

You might be able poll the resource with XMLHttpRequest asking it if it has already received the failing submission. Although issues like bug 23933	could stand in the way.

[Joe Mason]

Created attachment 55765 [details]
Attempt at layout test (not working)

Still unable to make a layout test for this.  I've attached my best attempt.  However, this code fails because after submitting a form with document.form.submit, the result replaces the current document even if it's an error code with no body.

This confuses me because it's not what happens when I run QtLauncher by hand, after editing WebPage::supportsExtension to turn off the ErrorPageExtension.  And LayoutTestControllerQt contains "m_handleErrorPages = false", so I think error pages are turned off for it as well.  So the error 500 I'm returning shouldn't cause a new page load - but it does.  After submitting postForm1, the output to the initial document ("Submitting postForm1"), "Waiting for postForm1") is cleared, and the final result is:

CONSOLE MESSAGE: line 68: TypeError: Result of expression 'form' [undefined] is not an object.
FAIL: Timed out waiting for notifyDone to be called
Got response: submitted=value1

Which tells me that it's gotten the response fine and gone on to submit the second form, which fails because "document.postForm2" is no longer an existing element: the document's been overwritten.

If anyone can help me get around this final hurdle I think this layout test will work.

[Joe Mason]

Created attachment 55768 [details]
updated patch

This fixes the error in a safer way than the first patch.

[Dimitri Glazkov (Google)]

Comment on attachment 55768 [details]
updated patch


> +    // (Nested FIXME: except we don't reset it on mouse events, only keydown.
> +    // This is important because part of the point is to prevent the user from
> +    // accidentally clicking twice on important forms like 1-click orders.)

But... we do reset it on mouse events. Look at EventHandlerMac.mm or WebKit/chromium/WebViewImpl.cpp.

Do you mean it's not done in Qt port?

[WebKit Review Bot]

Attachment 55768 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/loader/FrameLoader.cpp:520:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joe Mason]

Created attachment 55771 [details]
updated patch, with updated copyright

Missed a copyright string in the last patch.

[WebKit Review Bot]

Attachment 55771 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/loader/FrameLoader.cpp:520:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 55771 [details]
updated patch, with updated copyright

Why not just resetMultipleFormSubmission protection on click, like other ports do?

[Joe Mason]

Sigh. I only grepped WebCore for use of resetMultipleFormSubmission.  Why is it done in the cross-platform EventHandler.cpp for keyboard events, and in port-specific code for mouse events?

[Dimitri Glazkov (Google)]

(In reply to comment #16)
> Sigh. I only grepped WebCore for use of resetMultipleFormSubmission.  Why is it done in the cross-platform EventHandler.cpp for keyboard events, and in port-specific code for mouse events?

That's a good question. The only reason I know is because I added this to Chromium port a short while ago.

[Joe Mason]

Created attachment 55773 [details]
removed bogus comment

Here it is again, without the bogus "we don't clear it on mouse click" comment.  I also fixed the strcmp style error the stylebot was complaining about.

I think this is still a good patch even if the port does clear the multiple form submission on mouse click, because  it fixes a clear bug that's usually hidden by clearing the flag on click.

[Darin Adler]

Comment on attachment 55773 [details]
removed bogus comment

Needs a regression test or an explanation of why one can't be created.

[Joe Mason]

There's an explanation of why a regression test can't be created in comment #9.

[Joe Mason]

resetMultipleFormSubmission was moved to core in bug 28633, which I think will affect this bug.

[Joe Mason]

bug 68185, to move resetMultipleFormSubmission to another abstraction layer, will certainly affect this.