Bug 28633 - Submitting a form with target=_blank works only once
: Submitting a form with target=_blank works only once
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
:
: HasReduction, InRadar, Regression, Re...
: 68033 68309
: 37236 39021
  Show dependency treegraph
 
Reported: 2009-08-21 15:15 PST by
Modified: 2011-09-17 01:01 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-08-21 15:15:15 PST
Created an attachment (id=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 From 2009-09-23 12:16:01 PST -------
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 From 2009-09-23 12:22:59 PST -------
Created an attachment (id=40011) [details]
test cases
------- Comment #3 From 2009-09-23 12:24:20 PST -------
(From update of attachment 40011 [details])
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 From 2009-11-02 12:13:10 PST -------
<rdar://problem/7357787>
------- Comment #5 From 2009-11-20 13:10:55 PST -------
Does this affect any real website?
------- Comment #6 From 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 From 2009-12-11 17:38:13 PST -------
I can't reproduce this in Safari 4.0.4 - can anyone else?
------- Comment #8 From 2010-01-07 16:11:28 PST -------
I can't reproduce the problem either with the latest nightly.
------- Comment #9 From 2010-04-03 19:49:47 PST -------
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 From 2010-04-10 15:08:33 PST -------
I could reproduce with r57388 on Windows using the original test case (opening from Bugzilla directly).
------- Comment #11 From 2010-04-15 05:05:22 PST -------
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 From 2010-04-17 21:25:55 PST -------
See also: bug 37756.
------- Comment #13 From 2010-05-16 23:03:21 PST -------
 * 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 From 2010-05-17 09:04:11 PST -------
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 From 2010-05-17 09:25:36 PST -------
(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 From 2010-05-26 07:40:06 PST -------
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 From 2011-09-13 15:59:14 PST -------
Created an attachment (id=107248) [details]
Patch
------- Comment #18 From 2011-09-13 16:30:39 PST -------
(From update of attachment 107248 [details])
This moves the code from Mac-specific to platform-independent code. What effect does this have on other platforms?
------- Comment #19 From 2011-09-13 16:43:24 PST -------
(From update of attachment 107248 [details])
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 From 2011-09-13 17:55:21 PST -------
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 From 2011-09-14 14:06:04 PST -------
Created an attachment (id=107389) [details]
Patch
------- Comment #22 From 2011-09-14 14:06:43 PST -------
I reduced the timeout to 100ms and removed the call in the chromium port.
------- Comment #23 From 2011-09-14 14:08:21 PST -------
BTW, the new test fails if I set the timeout to 0.
------- Comment #24 From 2011-09-14 21:19:03 PST -------
(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.

> 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 From 2011-09-15 00:42:28 PST -------
(From update of attachment 107389 [details])
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 From 2011-09-15 11:30:04 PST -------
(In reply to comment #24)
> (From update of attachment 107389 [details] [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 From 2011-09-15 11:36:55 PST -------
Created an attachment (id=107519) [details]
Patch
------- Comment #28 From 2011-09-15 11:38:03 PST -------
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 From 2011-09-15 12:58:01 PST -------
(From update of attachment 107519 [details])
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 From 2011-09-15 13:06:43 PST -------
(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 From 2011-09-15 13:08:45 PST -------
(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 From 2011-09-15 13:25:02 PST -------
Filed bug 68185 re: moving the calls to another layer.
------- Comment #33 From 2011-09-15 14:03:17 PST -------
Committed r95226: <http://trac.webkit.org/changeset/95226>