Bug 31097

Created attachment 42448 [details] Testcase of quirks mode.

Created attachment 42449 [details] Testcase of standards mode.

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

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; } }

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

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

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

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!

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.

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.

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

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!

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.

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

Comment on attachment 50292 [details] patch after spell checking Thanks.

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

Comment on attachment 50292 [details] patch after spell checking Lets try that again.

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

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.

I am checking this failure

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.

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

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

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

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.

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.

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

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

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.

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

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.