Closed Bug 1487705 Opened 6 years ago Closed 5 years ago

Selenium's submit() method causes an extra alert to be opened due to an additionally send custom "submit" event

Categories

(Remote Protocol :: Marionette, defect, P3)

Version 3
defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1370630

People

(Reporter: whimboo, Unassigned)

References

(Depends on 1 open bug, )

Details

(Keywords: regression)

Attachments

(1 file)

Attached file marionette test
Today I got a report from Aleksei that one of the Selenium tests for form submission are failing. Here the alert as raised by a Selenium script used for the submission is somewhat special. Calling `accept` for it will close it, but `modal.findModalDialogs()` still finds the user prompt.

Attached you can find the Marionette testcase.

Note that removing this special Event dispatching code makes it work.
So please note that the extra dispatched event causes two alerts to be opened! As such we correctly find a second alert, once the first one has been closed. It was formerly working because we internally assumed there is no dialog anymore, and as such no NoSuchAlertException was thrown.

As such the Selenium test has to be updated by removing the extra dispatching of the `submit` event.

This renders the bug invalid.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(barancev)
Resolution: --- → INVALID
David, I have heard that the Python bindings are doing the same thing. So you also want to fix it.
Flags: needinfo?(dburns)
All Selenium bindings are doing the same. And they used to be doing this for about three years. If you decline to fix this issue it can hurt users of all 3.x versions of all Selenium bindings.
Flags: needinfo?(barancev)
Here the reduced test case which only opens two alerts in Firefox. For each other browser only a single alert is opened:

<script>
function run() {
  var form = document.getElementById("theForm");
  var e = form.ownerDocument.createEvent('Event');
  e.initEvent('submit', true, true);
  if (form.dispatchEvent(e)) {
    HTMLFormElement.prototype.submit.call(form);
  }
}
</script>
<form id='theForm' action="javascript:alert('Tasty cheese');">
  <input id='unused' type='button' value='Submit' onclick="run()">
</form>

Olli, do you have an explanation for that? Is that correct, or do we have a bug in Firefox here?
Status: RESOLVED → REOPENED
Flags: needinfo?(dburns) → needinfo?(bugs)
Resolution: INVALID → ---
Btw. the event dispatching code in Selenium was added because of: https://github.com/SeleniumHQ/selenium/issues/3398

As it explains:

> If a form control (such as a submit button) has a name or id of submit it will mask the form's submit method.

and

> When invoking this method directly, no submit event is raised (in particular, the form's onsubmit event handler is not run)

So that makes my above mentioned code partly invalid. Here the updated code:

```
<script>
function run() {
  var form = document.getElementById("theForm");
  var e = form.ownerDocument.createEvent('Event');
  // form.submit();
  e.initEvent('submit', true, true);
  if (form.dispatchEvent(e)) {
    HTMLFormElement.prototype.submit.call(form);
  }
}
</script>
<form id='theForm' action="javascript:alert('Tasty cheese');">
  <input id='submit' type='button' value='Submit' onclick="run()">
</form>
```

So now I wonder why when I remove the dispatching of `submit`, I still see the alert being opened by the onsubmit handler, given that no such event should be sent when calling `submit`, which is done here via `HTMLFormElement.prototype.submit.call(form)`.
Keywords: regression
Please note that until we solved this problem I asked for a backout of my patch on bug 1487358.
I queried Bugzilla a bit and actually found bug 1477286 which sounds exactly what the underlying problem is here. Synthesizing the `submit` event shouldn't cause the alert to be opened, right?
Flags: needinfo?(bugs)
er, that didn't work. I was trying to forward ni? to edgar since he is looking at bug 1477286
Flags: needinfo?(echen)
(In reply to Henrik Skupin (:whimboo) from comment #7)
> I queried Bugzilla a bit and actually found bug 1477286 which sounds exactly
> what the underlying problem is here. Synthesizing the `submit` event
> shouldn't cause the alert to be opened, right?

Yes, Synthesizing the `submit` event shouldn't submit the form per spec, but some website depends on this behavior, unfortunately (see bug 1399783).

(In reply to Henrik Skupin (:whimboo) from comment #5)
> So now I wonder why when I remove the dispatching of `submit`, I still see
> the alert being opened by the onsubmit handler, given that no such event
> should be sent when calling `submit`, which is done here via
> `HTMLFormElement.prototype.submit.call(form)`.

I don't understand where is the problem here. Calling HTMLFormElement.prototype.submit.call(form) will submit the form and the alter will be opened which is expected behavior, IMO.
Flags: needinfo?(echen) → needinfo?(hskupin)
(In reply to Edgar Chen [:edgar] from comment #9)
> I don't understand where is the problem here. Calling
> HTMLFormElement.prototype.submit.call(form) will submit the form and the
> alter will be opened which is expected behavior, IMO.

Oh, that is interesting! Thanks for the info. I tried it myself with an updated pen:

https://codepen.io/anon/pen/MqmrxY?editors=1010#0

And it looks like that it works just fine across browsers. At least for Firefox, Chrome, and Safari (I cannot test IE or Edge).

Alexei, could you please check if updating the Selenium code by removing the custom event works for you? It would be great to get this workaround removed.

Thanks.
Flags: needinfo?(hskupin) → needinfo?(barancev)
Summary: Event dispatching for form submission via "WebDriver:ExecuteScript" causes "modal.findModalDialogs()" to still find a prompt even after it was accepted → Selenium's submit() method causes an extra alert to be opened due to an additionally send custom "submit" event
Of course we can change Selenium client bindings. But we can't change already released versions. If you say that it's OK to break compatibility with all the versions shipped during two years -- who am I to stop you? Just be informed that users of all versions from 3.0 to 3.14 in all programming languages can be hit by this issue.
Flags: needinfo?(barancev)
Please note that we have two phases here. At first we can change Selenium to get rid of this quirk, none of the browsers need but which causes an unexpected behavior in Firefox due to a bug, which cannot easily be fixed due to external dependencies like Jenkins. This change shouldn't hurt anyone, and as far as I got was even only added due to Firefox about a year ago, right?

Then at some point we will have a Selenium release. Let it be 3.15 or whatever. Once this is out we can still wait a bit with landing my patch, or just do it. Only from the perspective of Firefox something should change for Selenium users, and we will just bump our minimum version of Selenium to lets say 3.15 for one of the Firefox releases. Everyone using `.submit()` can simply update it's installation.

Would you mind to help us with the binding changes? I would appreciate.
Flags: needinfo?(barancev)
Yes, raise a new issue on Selenium, please, and mark all bindings as subject to change.

P.S. I wonder how Jenkins works in older Firefox versions and other browsers if it depends on this behaviour? And why can't they fix it if the bug has been recently introduced? Why should you change the browser instead and break the whole world?
Flags: needinfo?(barancev)
Jenkins is using some custom code for Firefox only, so no other browser is affected. And that becauses it uses non-trusted submit events, which actually blocks our fix for the problem. For more details see bug 1399783 (as mentioned above). 

Having Selenium firing this non-trusted submit event (which is actually wrong), blocks us in WebDriver compatibility due to the extra user prompt being raised.

I hope this clears it up why it has to be removed.
Anyway, I suppose it's much easier to fix the Jenkins site (a single site!) than doing harm to users of all existing Selenium releases.
I filed https://github.com/SeleniumHQ/selenium/issues/6361 for getting the untrusted event code removed from Selenium.
(In reply to Alexei Barantsev from comment #15)
> Anyway, I suppose it's much easier to fix the Jenkins site (a single site!)
> than doing harm to users of all existing Selenium releases.

No, Selenium should not use this code! See the details of my newly filed issue.
Oh, I'm sorry, it's not "Jenkins website" that I thought the site of the organization developing Jenkins, but "Jenkins itself" is broken.

Sure we'll change Selenium, I'm just worrying about backward compatibility.
(In reply to Alexei Barantsev from comment #18)
> Sure we'll change Selenium, I'm just worrying about backward compatibility.

As I said above it works all fine down to ESR52 and maybe even further, but earlier Firefox releases we don't support anyway.
I'm talking about backward compatibility of new Firefox version with old (<= 3.14) Selenium versions, it's going to be broken.
(In reply to Alexei Barantsev from comment #20)
> I'm talking about backward compatibility of new Firefox version with old (<=
> 3.14) Selenium versions, it's going to be broken.

No, because the extra event only triggered an additional form submission. Removing it a single one will remain, which is what we would expect.

But one question through for Edgar. I assume the Selenium project wanted to make sure that an `submit` event is dispatched on the window given that `HTMLFormElement.prototype.submit.call(form)` doesn't send one. So I assume that the following line only has to be changed to fix the problem:

> -  if (form.dispatchEvent(e)) {
> +  if (window.dispatchEvent(e)) {

Here the full code: https://codepen.io/anon/pen/BOZjWJ?editors=1010
Flags: needinfo?(echen)
(In reply to Henrik Skupin (:whimboo) from comment #21)
> But one question through for Edgar. I assume the Selenium project wanted to
> make sure that an `submit` event is dispatched on the window given that
> `HTMLFormElement.prototype.submit.call(form)` doesn't send one. So I assume
> that the following line only has to be changed to fix the problem:

Yes, `HTMLFormElement.prototype.submit.call(form)` doesn't generate a `submit` event per spec.
And only dispatching `submit` event to HTMTFormElement will trigger a form submission, so dispatching it to window should be fine.
Flags: needinfo?(echen)
Great. I will work on those changes for Selenium then, and will mark this bug as fixed once it has been done.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
(In reply to Edgar Chen [:edgar] from comment #22)
> Yes, `HTMLFormElement.prototype.submit.call(form)` doesn't generate a
> `submit` event per spec.
> And only dispatching `submit` event to HTMTFormElement will trigger a form
> submission, so dispatching it to window should be fine.

As it looks like it is not fine because if there is a registered `onsubmit` handler on the form, it won't be called that way. Please see this example: https://codepen.io/anon/pen/BOZjWJ?editors=1000#0

I feel that I should revert the changes which I already made for Selenium, and that we have to wait until Jenkins, and Firefox have been fixed.
Flags: needinfo?(echen)
As we decided for the Selenium project we are not going to make this change. As such this bug is dependent on bug 1477286 now. Lets hope that it won't take that long to get the Jenkins issue sorted.
Assignee: hskupin → nobody
Blocks: 1487358
Status: ASSIGNED → NEW
Depends on: 1477286
No longer depends on: 1487358
Flags: needinfo?(echen)
Depends on: 1399783
Depends on: 1370630
No longer depends on: 1477286

Note that Jenkins finally shipped a fix for the non-trusted submit event (bug 1399783), which unblocked us from landing bug 1370630. This means all dependencies have been fixed now, and we could get this landed again. But before doing so I will wait a couple of days.

Alexei, do you know which version of Jenkins the Selenium project is using? Is it already v2.164.3 LTS? If not, are there plans in upgrading it? Thanks

Flags: needinfo?(barancev)

We use Travis now

Flags: needinfo?(barancev)

Note, that this behavior was actually not caused by Marionette. It got fixed by bug 1370630 in the recent nightly build!

Status: NEW → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → DUPLICATE
Blocks: 1560010
No longer blocks: 1560010
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: