Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 6 years ago

I noticed this misbehavior while working on bug 1473553.

Basically when the Element Send Key command finds an open user prompt, and closes it if set so via the `unhandledPromptBehavior` capability, the synthesizing of key events fail and the text is not getting added.

I already tried to set the focus manually or via the frame script to the `contentBrowser` but nothing worked. Maybe this is even a deeper problem in Firefox. More investigation clearly has to be done.

Attached is a Marionette testcase which demonstrates this failure.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 1 • 6 years ago

As it can be seen in this focus log, when the user prompt is opened, the window is lowered and as such the input element is blurred. Once the prompt closed, the window is raises, made active, and the input element gets the focus back.

So I doubt that the problem here is related to the focus. It may be something else.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 6 years ago

Also note that it works when the user prompt is closed via the alert accept/dismiss command before calling send keys again:

> elem.send_keys("foo ")
>
> self.marionette.execute_script("window.prompt('what')")
> alert = Alert(self.marionette)
> alert.dismiss()
>
> elem.send_keys("bar")

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 6 years ago

The reason for this misbehavior is actually easy. Simply have a look at this code:

https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/testing/marionette/driver.js#3176-3177

> GeckoDriver.prototype.acceptDialog = function() {
[..]
>   let {button0} = this.dialog.ui;
>   button0.click();
>   this.dialog = null;
> }

There is absolutely no event handling in place! Right now we assume that clicking the button immediately closes the window, but no all this is happening asynchronously. As such we send the text while the prompt is still open, and as result it never reaches the input.

So both the `acceptDialog()` and `dismissDialog()` methods need a registered observer to handle the removal of the tab modal prompt.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 6 years ago

Both "WebDriver:AcceptAlert" and "WebDriver:DismissAlert" have to
wait until the tab modal dialog has been closed.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 6 years ago

Both "WebDriver:AcceptAlert" and "WebDriver:DismissAlert" have to
wait until the tab modal dialog has been closed.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 6 years ago

The last try build had a lot of errors all caused by a missing `catch()` for the await.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6435ac0b94943fd362cd965a8864ac6e48732b9

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 6 years ago

Comment on attachment 9003359 [details] [diff] [review]
[marionette] Wait for tab modal dialog to disappear

Review of attachment 9003359 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/driver.js
@@ +3135,4 @@
>  GeckoDriver.prototype.fullscreenWindow = async function() {
>    assert.firefox();
>    const win = assert.open(this.getCurrentWindow());
> +  await this._handleUserPrompts().catch(err => { throw err; });

AIUI errors are thrown implicitly.

@@ +3185,5 @@
> +    }), {once: true});
> +
> +    let {button0} = this.dialog.ui;
> +    button0.click();
> +  });

I’m skeptical about the use of whenIdle here.  Is it absolutely
necessary?  It it works without, I would prefer not to delegate
until the main thread is idle.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 6 years ago

Comment on attachment 9003359 [details] [diff] [review]
[marionette] Wait for tab modal dialog to disappear

Review of attachment 9003359 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/driver.js
@@ +3135,4 @@
>  GeckoDriver.prototype.fullscreenWindow = async function() {
>    assert.firefox();
>    const win = assert.open(this.getCurrentWindow());
> +  await this._handleUserPrompts().catch(err => { throw err; });

Well, we have to catch the failure here before proceeding with the rest of the method.

If you don't do it this way, the call to `_handleUserPrompts()` would have to be wrapped with a try/catch block, so that we can still return immediately with the failure. If it's not added the exception is propagated to the callee, but then we would have run the actual command.

See the examples here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await

@@ +3185,5 @@
> +    }), {once: true});
> +
> +    let {button0} = this.dialog.ui;
> +    button0.click();
> +  });

This is necessary here. Only with a sleep(0) I can get it working. Otherwise we would still fail.

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 6 years ago

It’s a bit hard to understand what you’re replying to when you left
out the comments.

(In reply to Henrik Skupin (:whimboo) from comment #8)

> Well, we have to catch the failure here before proceeding with the
> rest of the method.

The rest of the function _won’t_ proceed/be evaluated if "await
_handleUserPrompts()" throws an error.

> If you don't do it this way, the call to `_handleUserPrompts()`
> would have to be wrapped with a try/catch block, so that we can
> still return immediately with the failure. If it's not added the
> exception is propagated to the callee, but then we would have run
> the actual command.

I don’t think I understand what you’re trying to tell me here.

Why does _handleUserPrompts() have to be wrapped in try…catch?  Let
me contrive an example, because it’s easier to talk about this in
isolation:

> function foo() {
>   throw new Error();
> }
> 
> function bar() {
>   foo();
>   return true;
> }
> 
> bar();

Is equivalent to:

> function foo() {
>   throw new Error();
> }
> 
> function bar() {
>   try {
>     foo();
>   } catch (e) {
>     throw e;
>   }
>   return true;
> }
> 
> bar();

Neither of the bar()s will return true.

> See the examples here:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await

The example here catches the error when it wants to _do_ something
with it, because otherwise it would lead to the runtime breaking
due to the error, instead of it being written to console.  In our
case, the caller of, say, GeckoDriver#get catches whatever error
the command function produces:
https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/testing/marionette/server.js#271-272

> @@ +3185,5 @@
> > +    }), {once: true});
> > +
> > +    let {button0} = this.dialog.ui;
> > +    button0.click();
> > +  });
> 
> This is necessary here. Only with a sleep(0) I can get it
> working. Otherwise we would still fail.

My guess is that the vent you’re listening for fires too soon in
that case.  That is surprising, since these dialogues live in chrome
space.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 6 years ago

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #9)
> It’s a bit hard to understand what you’re replying to when you left
> out the comments.

I replied via splinter, that was the problem here. So follow-up comments should just be better done via just comments on the bug. Will do that from now on.

> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await
> 
> The example here catches the error when it wants to _do_ something
> with it, because otherwise it would lead to the runtime breaking
> due to the error, instead of it being written to console.  In our

You are right here. And that indeed works. I will revert all those lines.

> > This is necessary here. Only with a sleep(0) I can get it
> > working. Otherwise we would still fail.
> 
> My guess is that the vent you’re listening for fires too soon in
> that case.  That is surprising, since these dialogues live in chrome
> space.

I have seen a couple of mochitests which also make use of this method. Especially when a test opens a tab, and immediately closes it. Here the tests wait for the next tick.

So which specific concerns do you have when we are also doing that? What could happen?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 11 • 6 years ago

Both "WebDriver:AcceptAlert" and "WebDriver:DismissAlert" have to
wait until the tab modal dialog has been closed.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 6 years ago

(In reply to Henrik Skupin (:whimboo) from comment #10)

> > > This is necessary here. Only with a sleep(0) I can get it
> > > working. Otherwise we would still fail.
> > 
> > My guess is that the vent you’re listening for fires too soon in
> > that case.  That is surprising, since these dialogues live in
> > chrome space.
> 
> I have seen a couple of mochitests which also make use of this
> method.  Especially when a test opens a tab, and immediately
> closes it. Here the tests wait for the next tick.
> 
> So which specific concerns do you have when we are also doing
> that? What could happen?

We use Services.tm.idleDispatchToMainThread wrapping
ChromeWindow.requestAnimationFrame as a hack around the fact that
the events related to window resizing- and repositioning can’t be
trusted and that DOM property propagation of outerHeight/outerWidth
is buggy.

Depending on the calling context, outerWidth/outerHeight sometimes
return the old values and interchangably the updated values, before
they again return the old values.  I haven’t been able to track
this down, but this is essentially the source of all the window
manipulation related woes we’re seeing in tests.

We use the two techniques implemented in whenIdle to circumvent
this problem by inducing extra latency.  This happens to introduce
enough lag for the tests to pass, but it is severely unreliable.

If DOMModalDialogClosed fires before the dialogues close, it means
it fires too early.  If using whenIdle is the only way to make these
tests pass reliably we obviously have no choice, but I just wanted
to point out the brittleness of the approach.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 6 years ago

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #12)
> If DOMModalDialogClosed fires before the dialogues close, it means
> it fires too early.  If using whenIdle is the only way to make these

No, it is clearly fired when the dialog has already been closed:

https://developer.mozilla.org/en-US/docs/Web/Events/DOMModalDialogClosed

> tests pass reliably we obviously have no choice, but I just wanted
> to point out the brittleness of the approach.

What I could imagine is that it is maybe really a focus problem. When the dialog has been closed, and we receive the event Marionette starts immediately to send the text. But if the window hasn't gotten any cycles yet to set the focus back to the window, and formerly focused element, the synthesized key events would end-up nowhere. Waiting for the next tick, might make that happen.

But I will keep this in mind, and if failures occur we should further investigate.

	Pulsebot
	Comment 14 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0509419ea085
[marionette] Wait for tab modal dialog to disappear.

	Andrei Ciure[:aciure]
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0509419ea085