Bug 28633 - Submitting a form with target=_blank works only once
: Submitting a form with target=_blank works only once
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To: Jon Lee
: HasReduction, InRadar, Regression, ReviewedForRadar
Depends on: 68033 68309
Blocks: 37236 39021
  Show dependency treegraph
 
Reported: 2009-08-21 15:15 PDT by jasneet
Modified: 2011-09-17 01:01 PDT (History)
19 users (show)

See Also:


Attachments
testcase (280 bytes, text/html)
2009-08-21 15:15 PDT, jasneet
no flags Details
more detailed bug information (937 bytes, text/html)
2009-09-23 12:22 PDT, Derek Krzanowski
no flags Details
Patch (7.83 KB, patch)
2011-09-13 15:59 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (9.90 KB, patch)
2011-09-14 14:06 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (10.42 KB, patch)
2011-09-15 11:36 PDT, Jon Lee
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2009-08-21 15:15:15 PDT
Created attachment 38403 [details]
testcase

I Steps:
1. Create a form with the target=_blank attribute
2. Submit the form
3. Close the new opened tab
4. The form won't submit anymore

II Other Browsers:
IE7: ok
FF3: ok

III Nightly tested: 46809

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=16528
Comment 1 Derek Krzanowski 2009-09-23 12:16:01 PDT
I have confirmed that this is a regression issue and can be reduced to submission to the exact same URI when targeting a blank window in both Safari 4.0.3 and Chrome 3.0.  If you submit to a different URI you can continue to target a blank window without issue.  I have confirmed this in the attached document using 2 distinct methods.

I tested Safari 3.2.1 on OSX 10.5.6 and Safari 3.2.3 on WinXP and this bug did not happen.  Please add the Regression tag as I do not have the permission to do so.

Also, to add to the creators "Other Browsers", Opera 10 and IE8 are not affected by this bug.
Comment 2 Derek Krzanowski 2009-09-23 12:22:59 PDT
Created attachment 40011 [details]
more detailed bug information
Comment 3 Derek Krzanowski 2009-09-23 12:24:20 PDT
Comment on attachment 40011 [details]
more detailed bug information

I have confirmed that this is a regression issue and can be reduced to
submission to the exact same URI when targeting a blank window in both Safari
4.0.3 and Chrome 3.0.  If you submit to a different URI you can continue to
target a blank window without issue.  I have confirmed this in the attached
document using 2 distinct methods.

I tested Safari 3.2.1 on OSX 10.5.6 and Safari 3.2.3 on WinXP and this bug did
not happen.  Please add the Regression tag as I do not have the permission to
do so.

Also, to add to the creators "Other Browsers", Opera 10 and IE8 are not
affected by this bug.
Comment 4 Mark Rowe (bdash) 2009-11-02 12:13:10 PST
<rdar://problem/7357787>
Comment 5 Simon Fraser (smfr) 2009-11-20 13:10:55 PST
Does this affect any real website?
Comment 6 Derek Krzanowski 2009-11-20 13:17:13 PST
Maybe not but it does affect the bookmarklet I wrote for our website.  Our bookmarklet allows you to clip items from a page and then submit them to our service using a form.  We don't want to take you away from the page you are on so we use target=_blank.  I had to put in the workaround of changing the uri in order to allow clipping from the same page more than once in the same page instance.  It is somewhat rare but it does happen.
Comment 7 Adele Peterson 2009-12-11 17:38:13 PST
I can't reproduce this in Safari 4.0.4 - can anyone else?
Comment 8 Maciej Stachowiak 2010-01-07 16:11:28 PST
I can't reproduce the problem either with the latest nightly.
Comment 9 Rob 2010-04-03 19:49:47 PDT
Hi, is there any news about this problem? I just reproduced it with Chrome 4.1.249.1045 and Safari 4.0.5.

Best Regards.
Comment 10 Alexey Proskuryakov 2010-04-10 15:08:33 PDT
I could reproduce with r57388 on Windows using the original test case (opening from Bugzilla directly).
Comment 11 Sachin Sharma 2010-04-15 05:05:22 PDT
I too just reproduced this Issue on Safari 4.0.4 (Windows).
It could not be reproduced in Safari 4 on Mac though.
Comment 12 Alexey Proskuryakov 2010-04-17 21:25:55 PDT
See also: bug 37756.
Comment 13 Tom Clift 2010-05-16 23:03:21 PDT
 * Reproduced on Chromium 5.0.342.9 (Developer Build 43360) Ubuntu, WebKit 533.2.
 * Could NOT reproduce on a Windows Chrome build, WebKit 533.4.

This issue was significant enough for our application that we implemented a workaround.  See the first screenshot on this page:

http://www.papercut.com/products/ng/tour/report/

The icons next to each report are image submits that open the selected report in a new tab.  This bug means that the user can only click one report, after which all other links will fail to work (until the page has been refreshed).

In general this bug would seem to apply to any form that submits to a new tab/window.

Some special notes about reproducing this bug:
 * If using a keyboard shortcut to close the new tab/window (Ctrl-w, Ctrl-F4) the bug will not occur. (If using the mouse to close the tab the bug will be present).
 * After closing the new window/tab, pressing any key while the original tab is in focus will result in the bug not occurring. (If simply clicking to submit the form  again the bug will occur).

Here is a quick hacky JavaScript workaround we used globally (t=<timestamp> is appended to the form action each time it is submitted to keep the action unique):
----
/*
 * Workaround for WebKit bug preventing a form submitting twice to the same action.
 * https://bugs.webkit.org/show_bug.cgi?id=28633
 */
// $.browser.webkit on jQuery 1.4+
if ($.browser.safari) {
  /*
   * Could test $.browser.version here if we knew which versions the bug affected.
   *  - confirmed on 533.2
   *  - NOT on 533.4
   */
  this.action += (this.action.indexOf('?') == -1 ? '?' : '&');
  this.action += 't=' + new Date().getTime();
}
----

In response to <a href="#c5">Comment #5</a>: yes, plenty of real sites.

Given that I couldn't reproduce this on 533.4 my guess is that it's been fixed.
Comment 14 Dimitri Glazkov (Google) 2010-05-17 09:04:11 PDT
This is a multiple-form submission bug. Should only manifest itself in Safari Win now, I think. I just looked and I don't see the place where the multiple form protection is reset on mouse click. I could be mistaken, but In page/win/EventHandlerWin.cpp;EventHandler::passMousePressEventToSubFrame(...), there should be a call to : subrame->loader()->resetMultipleFormSubmissionProtection();

Adam, can you check on this? I don't have a Win build handy.
Comment 15 Adam Roben (:aroben) 2010-05-17 09:25:36 PDT
(In reply to comment #14)
> Adam, can you check on this? I don't have a Win build handy.

Yes, this does affect Apple's Windows port.
Comment 16 Derk-Jan Hartman 2010-05-26 07:40:06 PDT
Is this issue related to bug 37756 ? Because I still see that problem in the latest Webkit nightly builds on my Mac. (Version 4.0.5 (6531.22.7, r60144))
Comment 17 Jon Lee 2011-09-13 15:59:14 PDT
Created attachment 107248 [details]
Patch
Comment 18 Darin Adler 2011-09-13 16:30:39 PDT
Comment on attachment 107248 [details]
Patch

This moves the code from Mac-specific to platform-independent code. What effect does this have on other platforms?
Comment 19 Andy Estes 2011-09-13 16:43:24 PDT
Comment on attachment 107248 [details]
Patch

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

I'm going to r- this because of the setTimeout() issue.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:68
> +            setTimeout(checkForFlag, 500);

The 500ms timeout is unfortunate. Did you try this with a 0ms timeout?

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:79
> +            setTimeout(checkForFlag, 500);

Same question as above. It looks like this test will take at least 2 seconds to run with each of these timers being called twice. This seems unnecessarily long.

> Source/WebCore/page/mac/EventHandlerMac.mm:-481
> -    m_frame->loader()->resetMultipleFormSubmissionProtection();

Are there other ports where this call can be removed now that it is in a common place?
Comment 20 Jon Lee 2011-09-13 17:55:21 PDT
I looked for other calls to resetMultiple..() in a similar situation, and only found it called in the chromium port. efl and qt make the calls too, but not as a result of a mouse down event.

I think it would be safe to remove the call in the chromium code. Kent and/or Dmitri, your thoughts?

This bug was exhibited on both Mac and Windows, so pushing this down to platform-independent code fixes it in both cases.

I will update the timeout.
Comment 21 Jon Lee 2011-09-14 14:06:04 PDT
Created attachment 107389 [details]
Patch
Comment 22 Jon Lee 2011-09-14 14:06:43 PDT
I reduced the timeout to 100ms and removed the call in the chromium port.
Comment 23 Jon Lee 2011-09-14 14:08:21 PDT
BTW, the new test fails if I set the timeout to 0.
Comment 24 Ryosuke Niwa 2011-09-14 21:19:03 PDT
Comment on attachment 107389 [details]
Patch

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

While I don't intend to block your patch from landing since this is a progression, I think we're solving this problem in a wrong abstraction layer.

> Source/WebCore/page/EventHandler.cpp:1345
> +    m_frame->loader()->resetMultipleFormSubmissionProtection();
> +    

I don't think we should be doing calling these functions in EventHandler.  We're in the wrong abstraction layer.

> Source/WebCore/page/EventHandler.cpp:-2521
> -    // FIXME: what is this doing here, in keyboard event handler?

Please do not remove this comment.  We're clearly in the wrong abstraction layer, and we should be moving this code elsewhere.
Comment 25 Andy Estes 2011-09-15 00:42:28 PDT
Comment on attachment 107389 [details]
Patch

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

I still think we can do better with the test. Writing tests that rely on arbitrary timeouts is sub-optimal since it requires either using an unnecessarily high time interval or risking flakiness. Can you instead write this in a way that doesn't require timeouts for the test to fail? For instance, you could queue the submissions on 0-delay timers, then add script to submit-to-blank-multiple-times-form-action.html that logs a message and calls notifyDone() if it's the last page to load (you could use the query string to indicate which page load should trigger notifyDone()).

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:68
> +        function pressEnter() {

Technically you're pressing the space bar, not the enter key. I'd rename this to reflect the correct key press.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:70
> +            log('Reset global flag to false, pressing enter in second form');

Ditto.
Comment 26 Jon Lee 2011-09-15 11:30:04 PDT
(In reply to comment #24)
> (From update of attachment 107389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107389&action=review
> 
> While I don't intend to block your patch from landing since this is a progression, I think we're solving this problem in a wrong abstraction layer.
> 
> > Source/WebCore/page/EventHandler.cpp:1345
> > +    m_frame->loader()->resetMultipleFormSubmissionProtection();
> > +    
> 
> I don't think we should be doing calling these functions in EventHandler.  We're in the wrong abstraction layer.

I'll put in comments, but in which layer do you think these calls should be made?
Comment 27 Jon Lee 2011-09-15 11:36:55 PDT
Created attachment 107519 [details]
Patch
Comment 28 Jon Lee 2011-09-15 11:38:03 PDT
For the test, I was trying to do this before, but didn't realize I had to use window.opener.

I also put in amended comments per Ryosuke's request.
Comment 29 Andy Estes 2011-09-15 12:58:01 PDT
Comment on attachment 107519 [details]
Patch

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

Nice, r=me with comments.

> LayoutTests/ChangeLog:9
> +        New test that simulates mouse clicking submit button twice (which didn't work), as well as using the keyboard twice (which did work)

This sentence should end with a period.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:8
> +            This test will click the first submit button twice, then press enter on the second submit button twice. Both should popup two blank windows.

Should read 'then press the space bar ...'

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:61
> +                document.getElementById("nextOpKey").value = "space";
> +            else
> +                document.getElementById("nextOpKey").value = "notifyDone";

You set 'nextOpKey' here but you never check it in submit-to-blank-multiple-times-form-action.html (you only check 'nextOp'). This happens to not matter since 'nextOp' already has the value of 'space', so you probably don't need 'nextOpKey'.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:67
> +        function notifyDone() {
> +            layoutTestController.setCloseRemainingWindowsWhenComplete();
> +            layoutTestController.notifyDone();

I see how notifyDone() is called in the passing case, but I don't in the failing case. It looks like this test will time out if this bug is re-introduced in the future. Instead of timing out after 35 seconds (ORWT's default), we could time out after a more reasonable two seconds or so by adding a setTimeout() that calls notifyDone().

> Source/WebCore/page/EventHandler.cpp:1344
> +    // FIXME (bug 28633): this call should be made at another abstraction layer

Bug 28633 is this bug, which will be closed after you land your patch. We should file a new bug to track cleaning this up, and you can link to that bug in this FIXME.

> Source/WebCore/page/EventHandler.cpp:2524
> +    // FIXME (bug 28633): this call should be made at another abstraction layer

Ditto.
Comment 30 Jon Lee 2011-09-15 13:06:43 PDT
(In reply to comment #29)
> > LayoutTests/fast/forms/submit-to-blank-multiple-times.html:61
> > +                document.getElementById("nextOpKey").value = "space";
> > +            else
> > +                document.getElementById("nextOpKey").value = "notifyDone";
> 
> You set 'nextOpKey' here but you never check it in submit-to-blank-multiple-times-form-action.html (you only check 'nextOp'). This happens to not matter since 'nextOp' already has the value of 'space', so you probably don't need 'nextOpKey'.

I believe that the name attribute is what's used in form submissions, not the id. I use the id for convenience to set the value. You'll notice both forms use the same names.

I will integrate the rest of your comments into the landed patch. Thanks, Andy!
Comment 31 Andy Estes 2011-09-15 13:08:45 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > > LayoutTests/fast/forms/submit-to-blank-multiple-times.html:61
> > > +                document.getElementById("nextOpKey").value = "space";
> > > +            else
> > > +                document.getElementById("nextOpKey").value = "notifyDone";
> > 
> > You set 'nextOpKey' here but you never check it in submit-to-blank-multiple-times-form-action.html (you only check 'nextOp'). This happens to not matter since 'nextOp' already has the value of 'space', so you probably don't need 'nextOpKey'.
> 
> I believe that the name attribute is what's used in form submissions, not the id. I use the id for convenience to set the value. You'll notice both forms use the same names.

Oh you're right! I didn't even see the name attr.
Comment 32 Jon Lee 2011-09-15 13:25:02 PDT
Filed bug 68185 re: moving the calls to another layer.
Comment 33 Jon Lee 2011-09-15 14:03:17 PDT
Committed r95226: <http://trac.webkit.org/changeset/95226>