Bug 31097 – <FORM> sends the form-data to the URL specified in "action" only once.

• Component: Forms
• Status: ASSIGNED

[Hanrui]

Created attachment 42447 [details]
Screenshot for your reference.

Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
     Safari 4: Fail
  Firefox 3.x: OK
         IE 7: OK
         IE 8: OK

What steps will reproduce the problem?
    1. Launch Safari
    2. Load http://blog.sina.com.cn/
    3. Type any words in the input box
    4. Click "搜索". Then it will open a window or tab for results. 
    5. Back to original window or tab, click "搜索" again.

What is the expected result?
It should open a new window or tab to show the search result again.

What happens instead?
No result window nor tab has been opened.

Please provide any additional information below. Attach a screenshot if
possible.
1. When you go back to the original window or tab and press any key, and 
click the "搜索" button again, a new window/tab with search result will be 
shown.
2. When you go back to the original window or tab and press the "Enter" key  
in the input box, the result page could be shown.

So the issue seems to be that the <form> could not send the form-data 
continuously when there's no keyboard event happened between the submit 
button clicking. (Refer to my test-cases attached.)

[Hanrui]

Created attachment 42448 [details]
Testcase of quirks mode.

[Hanrui]

Created attachment 42449 [details]
Testcase of standards mode.

[Hanrui]

This bug is triggered when:
<form> has target attribute set to "_blank"
AND the form has a submit button

[Hanrui]

The second item is not an essential condition, look at the page again (http://blog.sina.com.cn/), the clicking the Google icon will lead you to Google homepage. However it could not trigger two times if you click it continuously (without any keyboard event between the icon clicks).
The Google icon is outside the form, it take the action by the codes below:
<a id="sbox_change_google" href="javascript:void(0)">
  <img height="23" width="94" alt="Google" src="http://i2.sinaimg.cn/lx/deco/2007/1229/standard_header_channel_google.gif"/>
</a>

sina.$("sbox_change_google").onclick = function(){
    submitFormWithChannel("tlogo");
};

function submitFormWithChannel(a) {
    if (document.gform.q.value == "\u8BF7\u8F93\u5173\u952E\u8BCD" ||
        document.gform.q.value == "\u8BF7\u8F93\u5165\u5173\u952E\u5B57" ||
        document.gform.q.value == "") {
        document.gform.q.value = "";
        document.gform.channel.value = a;
        document.gform.submit();
    } else {
        document.gform.channel.value = a;
        document.gform.submit();
        return;
    }
}

[Hanrui]

Created attachment 42451 [details]
The screenshot showing the Google icon cannot work twice.

[Hanrui]

Another page which has same issue: http://flash.17173.com/
The orange "Go" button cannot lead to search result page twice (or even more).

[Hanrui]

Based on our dedicated human testing to chinese web sites, the same issue also
applies to the following sites:

http://club.china.com
http://news.cctv.com
http://pc.2u.com.cn
http://www.52blackberry.com
http://chaxun.1616.net
http://www.abang.com
http://club2.cat898.com/newbbs/dispbbs.asp?boardid=1&id=3130543
http://www.chetx.com
http://www.cn910.net
http://www.cnbeta.com

[Johnny(Jianning) Ding]

After digging in WebCore, I found this problem was because the algorithm of preventing a single form from submitting more than once. (The logic is in FrameLoader::submitForm.)

I am not familiar with the context of the multiple form submission issue. So I had a few questions for this algorithm. Please correct me if those questions looks stupid:)

1. I checked the situation of submitting a single form more than once on both IE and Firefox. They do allow this. Why WebKit disallow this behavior if people explicitly want to do this. Please check comments #5 on http://crbug.com/26662.

2. According to the comments of current algorithm of suppressing multiple form submissions (line 471, FrameLoader.cc), when starting handle a new mouse or key down event, m_submittedFormURL will be reset to empty to allow another submission. Which means this algorithm does allow users to explicitly submit more than once on a single form. Am I right?

3. Although above comments said when handing a new mouse event or key down event, m_submittedFormURL will be reset to empty to allow another submission. But the truth is only key event was taken care.
When testing on http://club.china.com, after the first submission, if you click the search button again, nothing happened. But when you press keyboard on anywhere of webpage (didn't need to focus on the query input control to change the query), then click the search button again, a new tab will be opened for same query (if didn't change the query).  This behavior really makes users confused.

4. In current algorithm, it checked the frame target. If the current frame is descendant of the target frame, m_submittedFormURL would be used to check whether this submission is as same as previous one.(line 479-482, FrameLoader.cc) But the thing also makes me confused is when frame target is "_blank", the above logic also was hit. I think the "_blank" frame is obviously not ancestor of any frame. After checking the code, I think it was because we passed current frame to target frame when not finding out the target frame by name of target frame since findFrameForNavigation will return NULL when input is "_blank". I personally this it should be a error. When I checked the original patch (https://bugs.webkit.org/attachment.cgi?id=29178&action=diff), the original logic of checking target frame looked right.

If you agree with my analysis. I gonna provide a patch to fix this issue.

A immature idea to ignore too frequent submission and allow normal multiple submissions for a simple form is to add a timer to reset the m_submittedFormURL. For example, after submitting a form, a 3 seconds timer will be created to reset the m_submittedFormURL, so in the 3 seconds, users can not re-submit the form until if timer is expired. Just for your reference

So far I don't have other ideas to fix the multiple form submission issue in another better way, What I want to do to fix above problems in current way are.

1) correct the logic of checking frame target, the pseudo code looks like
    bool foundTarget = true; 
    Frame* targetFrame = findFrameForNavigation(targetOrBaseTarget);
    if (!targetFrame) {
        foundTarget = false;
        targetFrame = m_frame;
        frameRequest.setFrameName(targetOrBaseTarget);
    }
    ...
    if (foundTarget && m_frame->tree()->isDescendantOf(targetFrame)) {
    ...

2) add the logic of reset multiple form submission protection when handling a mouse press event
EventHandler::handleMousePressEvent(...) {
   ...
   m_frame->loader()->resetMultipleFormSubmissionProtection();
}

3) add a layout test to test multiple submission.

I am looking forward to your feedback.
Thanks!

[Johnny(Jianning) Ding]

Since now WebCore intends to empty m_submittedFormURL in a mouse or key down event, I personally think it is unnecessary to check the descendant relationship between current frame and target frame before comparing the previous submitted URL (m_submittedFormURL) and current submitting URL. Please refer to my patch.

[Johnny(Jianning) Ding]

Created attachment 47798 [details]
patch to allow the multiple submission of a single form when they are user-initiated and disallow the multiple submission of a single form when they are not user-initiated.

[Eric Seidel (no email)]

Johnny: Any guess from the changed code who should best review this?

[Johnny(Jianning) Ding]

Hi Eric
Actually I have no idea about the reviewers. After digging in the change http://trac.webkit.org/changeset/19940 from trac.webkit.org, I guess Maciej or  Antti may be interest in this patch:)

Thanks!

[Adam Barth]

Comment on attachment 47798 [details]
patch to allow the multiple submission of a single form when they are user-initiated and disallow the multiple submission of a single form when they are not user-initiated.

Looks like I'm as good a reviewer as you're going to get.  :)

This patch looks good.  Our current way of tracking whether we're handling a user input event is super lame, but I don't think we should block this patch on fixing that.

Please spellcheck your comments before landing.  For example, "subssion" is misspelled.

[Johnny(Jianning) Ding]

Created attachment 50292 [details]
patch after spell checking

Thanks Adam!

Sorry for the misspelling. This patch is as same as the patch you reviewed except I have done spell correction in this patch. I have also done the webkit-tests to make sure it didn't break the latest webkit tests.
Unfortunately I am still not a committer, so I have to leave this patch to you to help me land.

Thanks again

[Adam Barth]

Comment on attachment 50292 [details]
patch after spell checking

Thanks.

[WebKit Commit Bot]

Comment on attachment 50292 [details]
patch after spell checking

Rejecting patch 50292 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12468 test cases.
fast/encoding/char-encoding-mac.html -> failed

Exiting early after 1 failures. 6166 tests run.
109.95s total testing time

6165 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/545028

[Adam Barth]

Comment on attachment 50292 [details]
patch after spell checking

Lets try that again.

[WebKit Commit Bot]

Comment on attachment 50292 [details]
patch after spell checking

Rejecting patch 50292 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12474 test cases.
fast/encoding/char-encoding-mac.html -> failed

Exiting early after 1 failures. 6167 tests run.
108.78s total testing time

6166 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/657008

[Eric Seidel (no email)]

Comment on attachment 50292 [details]
patch after spell checking

Looks like a real failure introduced by this patch.  Marking r- since it appears to break a test.

[Johnny(Jianning) Ding]

I am checking this failure

[Johnny(Jianning) Ding]

This is because fast/encoding/resources/char-encoding-utils.js, it had multiple submissions for a single form triggered by script (those submission were not user-initiated). So the WebKit's multiple submissions protection for single form took effect to disallow the multiple submissions, which caused the related charset encoding tests were failed.

Those tests were successful on previous webkit revisions is because int those revisions, the multiple submissions protection check was only triggered when the current frame (frame of sending submission) is descendant of the target frame (or the target frame is "_blank", which was wrong). Actually in the old revisions, the multiple submissions protection was not active when running charset-encoding tests.

In this patch, I removed the unnecessary to check the descendant relationship, so the multiple submissions protection is active and takes effect.
I am sorry I didn't dig in the charset-encoding failures for previous patch. I thought they were just timeout issues.

I am modifying related affected tests to pass them. 
A new patch will be uploaded soon.

[Alexey Proskuryakov]

> I am modifying related affected tests to pass them. 

Is this the right thing to do? The test works in Firefox, so it sounds like the patch actually regresses behavior.

[Johnny(Jianning) Ding]

(In reply to comment #22)
> > I am modifying related affected tests to pass them. 
> 
> Is this the right thing to do? The test works in Firefox, so it sounds like the
> patch actually regresses behavior.

Thanks for comments, Alexey!

I didn't dig in the code of form submission in Firefox, but it looks Firefox does not have the multiple submission protection logic since all above sites work well in Firefox without this issue.

If WebKit community thinks a single form can have multiple non user-initiated submission (such as submission triggered by continuous calling form.submit()), then my patch is unnecessary. Otherwise the test needs to be changed.

Actually user can easily skip the multiple submission protection by adding a random and useless query parameter, this is also how I change the test.
@char-encoding-utils.js
line 57 form.action = "resources/dummy.html?" + i;
There is no test logic changed.

[Adam Barth]

> I didn't dig in the code of form submission in Firefox, but it looks Firefox
> does not have the multiple submission protection logic since all above sites
> work well in Firefox without this issue.

Generally, it's a good idea to match Firefox unless there's a compelling reason not to.

[Dimitri Glazkov (Google)]

I think we should just rip the whole multiple form submission protection out. It's pretty much disabled today and only shows up in the edge cases as an incompatibility, as reported on this bug. I have it on my todo list to make sure this doesn't introduce any regressions, and then sayonara baby.

[Alexey Proskuryakov]

Just as a historical reference, multiple form submission protection has been implemented in the early days of WebKit to fix a particular important site that complained about the user being already logged in. Information in the bug is incomplete, and it's not clear if the change was in fact the best way to fix the problem. There are signs that it was working around some issue with keyboard event handling which made WebKit submit a form twice when a user pressed Enter, as opposed to clicking.

I used to think that multiple form submission was meant to protect against POSTing forms twice when someone double clicks a button (which many users actually do). But it turns out that this wasn't the original motivation for adding the code.

I think that the difference between WebKit and Firefox is captured in bug 13012. Firefox seems to protect against multiple form submission implicitly by replacing a pending submission with a new one.

See also bug 33513, bug 34315, bug 4847, bug 13011, bug 13012, bug 11420, bug 16886, bug 16930. Most of these can probably be fixed with a single check-in, but I hesitated to resolve any as duplicates so far.

[Dimitri Glazkov (Google)]

> I think that the difference between WebKit and Firefox is captured in bug
> 13012. Firefox seems to protect against multiple form submission implicitly by
> replacing a pending submission with a new one.
> 
> See also bug 33513, bug 34315, bug 4847, bug 13011, bug 13012, bug 11420, bug
> 16886, bug 16930. Most of these can probably be fixed with a single check-in,
> but I hesitated to resolve any as duplicates so far.

This is really helpful! Thanks, Alexey.

[Johnny(Jianning) Ding]

Created attachment 50650 [details]
fix the failure of charset-encoding tests and should not affect other tests at least on my mac.

Alexey, thanks for introducing the history and motivation of multiple submission protection.

Since the protection doesn't mean to disallow script-initiated multiple submissions and Dimitri is planning to work on the new approach like you mentioned, I guess I should drop this patch. (But I still upload the latest patch without review request for reference.)

[Dimitri Glazkov (Google)]

Kevin McCullough attempted to do this once before in http://trac.webkit.org/changeset/18011, then it was reverted in http://trac.webkit.org/changeset/19940, when it broke a bunch of sites.

[Dimitri Glazkov (Google)]

Looks like mozilla does something much simpler in this case. They just detect if submission is in progress, stop it, then restart:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLFormElement.cpp#392

[Dimitri Glazkov (Google)]

The actual bit of magic lies in mDeferSubmission and mPendingSubmission (http://mxr.mozilla.org/mozilla-central/ident?i=mDeferSubmission), which is what we kind of have in m_insubmit, m_doingsubmit, and m_submittedFormURL. What we should do is clean up this logic to follow what Mozilla does.