Submitting a form with target=_blank works only once

• Component: Forms
• Status: RESOLVED FIXED

[jasneet]

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

[Derek Krzanowski]

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.

[Derek Krzanowski]

Created attachment 40011 [details]
more detailed bug information

[Derek Krzanowski]

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.

[Mark Rowe (bdash)]

<rdar://problem/7357787>

[Simon Fraser (smfr)]

Does this affect any real website?

[Derek Krzanowski]

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.

[Adele Peterson]

I can't reproduce this in Safari 4.0.4 - can anyone else?

[Maciej Stachowiak]

I can't reproduce the problem either with the latest nightly.

[Rob]

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.

[Alexey Proskuryakov]

I could reproduce with r57388 on Windows using the original test case (opening from Bugzilla directly).

[Sachin Sharma]

I too just reproduced this Issue on Safari 4.0.4 (Windows).
It could not be reproduced in Safari 4 on Mac though.

[Alexey Proskuryakov]

See also: bug 37756.

[Tom Clift]

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

[Dimitri Glazkov (Google)]

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.

[Adam Roben (:aroben)]

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

[Derk-Jan Hartman]

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

[Jon Lee]

Created attachment 107248 [details]
Patch

[Darin Adler]

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?

[Andy Estes]

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?

[Jon Lee]

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.

[Jon Lee]

Created attachment 107389 [details]
Patch

[Jon Lee]

I reduced the timeout to 100ms and removed the call in the chromium port.

[Jon Lee]

BTW, the new test fails if I set the timeout to 0.

[Ryosuke Niwa]

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.

[Andy Estes]

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.

[Jon Lee]

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

[Jon Lee]

Created attachment 107519 [details]
Patch

[Jon Lee]

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.

[Andy Estes]

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.

[Jon Lee]

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

[Andy Estes]

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

[Jon Lee]

Filed bug 68185 re: moving the calls to another layer.

[Jon Lee]

Committed r95226: <http://trac.webkit.org/changeset/95226>