Closed Bug 1414329 Opened 7 years ago Closed 7 years ago

interaction.flushEventLoop should not use beforeunload and request animation frame

Categories

(Remote Protocol :: Marionette, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(1 file)

For the reasons stated in https://bugzil.la/1411393,
interaction.flushEventLoop should not rely on the beforeunload DOM
event.

The purpose of flushEventLoop should be self-explanatory, but I
also think it is implemented the wrong way.  Instead of attaching a
beforeunload and requesting an animation frame, it should append a
custom event to the queue and wait for that to be processed.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: 1411393
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:335
(Diff revision 1)
> +              });
> +            </script>
> +        """))
> +        button = self.marionette.find_element(By.TAG_NAME, "button")
> +        button.click()
> +        self.assertTrue(self.marionette.execute_script("return window.clicked", sandbox=None))

You only check that `window.clicked` is set to true. But isn't that always true?

Right now it doesn't matter in how you prevent bubbling or propagation. The assertion as is would not be affected.

Actually the button should cause a navigation so that the `unload` event as used in `flushEventLoop` is utilized.

Maybe put those tests then under the navigation class.

::: testing/marionette/interaction.js:183
(Diff revision 1)
>      interaction.selectOption(el);
>    } else {
> +    // step 9
> +    let clicked = interaction.flushEventLoop(containerEl);
>      event.synthesizeMouseAtPoint(clickPoint.x, clickPoint.y, {}, win);
> +    await clicked;

I kinda like that change because it is exactly what I wanted to see when you originally implemented this feature. But at that time it didn't seem that important.

::: testing/marionette/interaction.js:311
(Diff revision 1)
> +    unloadEv = resolve;
> +    clickEv = () => {
> -    if (win.closed) {
> +      if (win.closed) {
> -      resolve();
> +        resolve();
> -      return;
> +      } else {
> +        win.setTimeout(resolve, 0);

Can you explain why we have to delay resolving the promise, even when the click event has already been happened?

::: testing/marionette/interaction.js:315
(Diff revision 1)
> -      return;
> +      } else {
> +        win.setTimeout(resolve, 0);
> -    }
> +      }
> +    };
>  
> -    win.addEventListener("beforeunload", handleEvent);
> +    win.addEventListener("unload", unloadEv, {mozSystemGroup: true});

I assume using `mozSystemGroup` here doesn't get in conflict with the other page load event listeners because we now return very late from `clickElement`.

::: testing/marionette/interaction.js:319
(Diff revision 1)
>  
> -    win.addEventListener("beforeunload", handleEvent);
> -    win.requestAnimationFrame(handleEvent);
> +    win.addEventListener("unload", unloadEv, {mozSystemGroup: true});
> +    el.addEventListener("click", clickEv, {mozSystemGroup: true});
> +  }).then(() => {
> +    win.removeEventListener("unload", unloadEv);
> +    el.removeEventListener("click", clickEv);

Shouldn't we make use of the `once` option instead? Or is that safer in terms of no `unload` event (no page navigation) is fired by the click, and we clearly remove the registered listener.
Attachment #8925131 - Flags: review?(hskupin) → review-
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201892

My main concern is what happens if the page overwrites some of the functions like addEventListener or setTimeout. Are we sure we are always getting the original function in that case?
Attachment #8925131 - Flags: review?(james) → review+
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> You only check that `window.clicked` is set to true. But isn't that always true?
> 
> Right now it doesn't matter in how you prevent bubbling or propagation. The assertion as is would not be affected.
> 
> Actually the button should cause a navigation so that the `unload` event as used in `flushEventLoop` is utilized.
> 
> Maybe put those tests then under the navigation class.

window.clicked is not always true if
event.{preventDefault,stopPropagation,stopImmediatePropagation}
prevents the event from bubbling. mozSystemGroup ensures that it
does.  I agree that in this case they would time out, but having an
assertion seems better than not having one.

> Can you explain why we have to delay resolving the promise, even when the click event has already been happened?

setTimeout spins the event loop one more time.

> I assume using `mozSystemGroup` here doesn't get in conflict with the other page load event listeners because we now return very late from `clickElement`.

The purpose of mozSystemGroup is to let
chrome event handlers not get interrupted by
preventDefault/stopPropagation/stopImmediatePropagation.

> Shouldn't we make use of the `once` option instead? Or is that safer in terms of no `unload` event (no page navigation) is fired by the click, and we clearly remove the registered listener.

We need to remove the event handlers explicitly because not both of
them fire, in which case {once: true} doesn’t matter because we
need to remove them both explicitly.  If we were dealing with one
event handler, I would agree with you but alas we are not.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> window.clicked is not always true if
> event.{preventDefault,stopPropagation,stopImmediatePropagation}
> prevents the event from bubbling. mozSystemGroup ensures that it
> does.  I agree that in this case they would time out, but having an
> assertion seems better than not having one.

I think this is the right explaination, but it buries the lede a little.

The tests are really just checking that we return from the promise at all in the case that event propogation is prevented. The clicked thing is just an additional test that the original handler actually runs; it isn't the central point of the test.

> setTimeout spins the event loop one more time.

So the original thinking was that this was needed to ensure that any handlers after the marionette one are activated. Thinking a bout it some more the fact that a promise has to resolve provides the same guarantee, but spinning the event loop does mean that we allow any other promises to resolve. I don't think it's strictly necessary per spec, but I think it's quite reasonable to delay returning until we have had a chance to spin the event loop.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> I think this is the right explaination, but it buries the lede a little.
> 
> The tests are really just checking that we return from the promise at all in the case that event propogation is prevented. The clicked thing is just an additional test that the original handler actually runs; it isn't the central point of the test.

Ok, James explanation makes sense. Given that we test the escape of the promise, that part is fine. But `window.clicked` should always be set, given that this callback is always called, and `preventDefault` (and the other methods) stops bubbling the event to the original listeners of the node. So it is confusing why it is present.

Lets make sure to add a doc string to the tests which explains what is really getting tested here.

> So the original thinking was that this was needed to ensure that any handlers after the marionette one are activated. Thinking a bout it some more the fact that a promise has to resolve provides the same guarantee, but spinning the event loop does mean that we allow any other promises to resolve. I don't think it's strictly necessary per spec, but I think it's quite reasonable to delay returning until we have had a chance to spin the event loop.

If it's for safety and stability I'm in favor of keeping it. I was just wondering why we need it at all.

> The purpose of mozSystemGroup is to let
> chrome event handlers not get interrupted by
> preventDefault/stopPropagation/stopImmediatePropagation.

That's not what I mean. The page navigation listeners operate on the capture phase, and also rely on `beforeunload`. I haven't checked the async code in detail yet, but what happens with the page load callback now given that `flushEventLoop` returns after this event has been fired? Do we still correctly setup the page load listeners?

Btw. I see various hangs for Wd spec tests related to click, mouse, and actions in general. I retriggered some of the jobs, so we can see if those are just random oranges, or bugs as introduced by this changes.

> We need to remove the event handlers explicitly because not both of
> them fire, in which case {once: true} doesn’t matter because we
> need to remove them both explicitly.  If we were dealing with one
> event handler, I would agree with you but alas we are not.

Please add this as comment to make it clear.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> That's not what I mean. The page navigation listeners operate on the capture phase, and also rely on `beforeunload`. I haven't checked the async code in detail yet, but what happens with the page load callback now given that `flushEventLoop` returns after this event has been fired? Do we still correctly setup the page load listeners?
> 
> Btw. I see various hangs for Wd spec tests related to click, mouse, and actions in general. I retriggered some of the jobs, so we can see if those are just random oranges, or bugs as introduced by this changes.

I think my concerns here were correct. As it can be seen now, all Wd spec tests are permanently failing.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> Ok, James explanation makes sense. Given that we test the escape of the promise, that part is fine. But `window.clicked` should always be set, given that this callback is always called, and `preventDefault` (and the other methods) stops bubbling the event to the original listeners of the node. So it is confusing why it is present.
> 
> Lets make sure to add a doc string to the tests which explains what is really getting tested here.

I agree the assertion is misleading.  I’ve taken some steps to
simplify the tests and make it clear exactly what the pass condition is.

> If it's for safety and stability I'm in favor of keeping it. I was just wondering why we need it at all.

The purpose of this function is to delay return of control to the
user until DOM event handlers triggered by the click have been
given a chance to fire. setTimeout in the click event handler
causes the event loop to spin, allowing event handlers we don’t
know about time to finish.  The central idea is to not return from
Element Click before the _effects_ the click may have on the DOM
are complete.  Of course we cannot guarantee that, but it is an
approximation.

> I think my concerns here were correct. As it can be seen now, all Wd spec tests are permanently failing.

The tests were failing because I had forgotten that action.js used
interaction.flushEventLoop after performing an action chain, and I
had forgotten to update the call after I changed flushEventLoop to
take the target element as an argument.

Because it now takes the target element as an argument, I’m
not sure we can continue to use flushEventLoop after performing
an action chain because we don’t know what the target of a
pointer action is.  I think this is fine because it is in line with
the thinking that the actions API provides low-level primitives
for direct input control, without any security checks such as
interactability.

I’m still not sure I understand your point about the capture phase
and beforeunload.  Because this is something you seem to know more
about than I do, can you please suggest what change I need to make?
Or how the approach taken here is conceptually wrong?
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201892

I think this is a valid concern, but it would affect much, much more
than just this code change.  I think we need to do some testing in
this area to harden Marionette.  If you would like to file against
https://bugzil.la/marionette-security, I would welcome that.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review201880

> The tests were failing because I had forgotten that action.js used
> interaction.flushEventLoop after performing an action chain, and I
> had forgotten to update the call after I changed flushEventLoop to
> take the target element as an argument.
> 
> Because it now takes the target element as an argument, I’m
> not sure we can continue to use flushEventLoop after performing
> an action chain because we don’t know what the target of a
> pointer action is.  I think this is fine because it is in line with
> the thinking that the actions API provides low-level primitives
> for direct input control, without any security checks such as
> interactability.
> 
> I’m still not sure I understand your point about the capture phase
> and beforeunload.  Because this is something you seem to know more
> about than I do, can you please suggest what change I need to make?
> Or how the approach taken here is conceptually wrong?

Ok, I digged through the listener code again, and it actually should not cause harm. All page load related commands go through `navigate`. That method sets up the page listeners before we actually call the `clickElement` method. The unload timer which I was concerned about is set afterward. So if we delay the return of `clickElement` with your changes, it should  all be fine and our page load listener will catch all the events.

Good to know what was the actual problem.
Comment on attachment 8925131 [details]
Bug 1414329 - Make WebDriver:ClickElement wait for click events

https://reviewboard.mozilla.org/r/196382/#review204556

::: testing/marionette/interaction.js:306
(Diff revision 3)
> -interaction.flushEventLoop = async function(win) {
> +interaction.flushEventLoop = async function(el) {
> +  const win = el.ownerGlobal;
> +  let unloadEv, clickEv;
> +
>    return new Promise(resolve => {
> -    let handleEvent = () => {
> +    dump("flushEventLoop\n");

I assume that is a left-over from debugging? If not please make it a debug log event, maybe even including the type of event which caused the resolve.
Attachment #8925131 - Flags: review?(hskupin) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f67d247cf652
Make WebDriver:ClickElement wait for click events r=jgraham,whimboo
https://hg.mozilla.org/mozilla-central/rev/f67d247cf652
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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: