814768 - Marionette-actors should be aware of when a frame closes so tests can resume

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Aaron Train [:aaronmt]]

Currently in Gaia-UI tests when one is context-switched to a frame, e.g, an <iframe> (such as as this [1]) and closes it, the marionette testing environment connection becomes severed. A message needs to be passed back so that a test can resume.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/ussd.js#L42 & https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/attention_screen.js#L177

Comment 1

[Zac C (:zac)]

This is true for Persona/login too. It self closes the window when you click 'Sign in'.

Comment 2

[Jonathan Griffin (:jgriffin)]

Can someone attach a test case that exposes this problem?

Comment 3

[Aaron Train [:aaronmt]]

Attachment: example Gaia-UI test-case

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> Can someone attach a test case that exposes this problem?

This is where I encountered this, https://gist.github.com/4150011

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

Aaron/Zac: is this going to block Marketplace/Identity automation?

Comment 6

[Zac C (:zac)]

You can work around it but it might be a bit fragile.

This is not the main blocker to Marketplace/Identity automation.

Comment 7

[Mozilla RelEng Bot]

Try run for 87b4fc0bde35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=87b4fc0bde35
Results (out of 9 total builds):
    success: 5
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-87b4fc0bde35

Comment 8

[Mozilla RelEng Bot]

Try run for 7020f3ce97ba is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7020f3ce97ba
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7020f3ce97ba

Comment 9

[Mozilla RelEng Bot]

Try run for 87b4fc0bde35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=87b4fc0bde35
Results (out of 10 total builds):
    exception: 1
    success: 5
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-87b4fc0bde35

Comment 10

[Mozilla RelEng Bot]

Try run for 5a226b4dc208 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5a226b4dc208
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5a226b4dc208

Comment 11

[Mozilla RelEng Bot]

Try run for 787dff9219a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=787dff9219a5
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-787dff9219a5

Comment 12

[Mozilla RelEng Bot]

Try run for 02323d3760b3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02323d3760b3
Results (out of 9 total builds):
    success: 8
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-02323d3760b3

Comment 13

[David Burns :automatedtester]

Attachment: Force marionette to move to window.top if we try access something the compartment is dead

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 697179 [details] [diff] [review]
Force marionette to move to window.top if we try access something the compartment is dead

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

I think this is a good check, but sadly it doesn't fix the original problem reported by Aaron.  In that case, what happens is that the Marionette:ok response to the click event gets initiated from marioentte-listeners, but is never received by marionette-actors, presumably because the process containing the frame is destroyed before the message is actually sent.

Additionally, there is another failure mode where attempting to send a message to a dead frame causes a NS_ERROR_FAILURE exception to be thrown here, which we do not catch:  http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#202

I think the right solution is to keep the check you've implemented in this patch, plus put all instances of sendAsync in marionette-actors.js in try/catch blocks (so we can return an exception on failure), plus do something additional to fix the first problem I mentioned.  That "something additional" might take some experimentation to get right; one approach that seems likely to succeed is to add a DOMNodeRemoved listener in marionette-actors, and when we've sent an async message to an OOP iframe (so this.currentRemoteFrame !== null) and that iframe is the evt.target of a DOMNodeRemoved event, return an error (instead of waiting forever for a response that will never come).

Comment 15

[Mozilla RelEng Bot]

Try run for 02323d3760b3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=02323d3760b3
Results (out of 10 total builds):
    success: 8
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-02323d3760b3

Comment 16

[David Burns :automatedtester]

Attachment: v2 - added in support for OOP frames disappearing on us

I have added in support for when OOP frames disappear from under us. the test is happier in that it doesnt hang the client marionette code however When we do a click.

The fix is in marionette-actors.js and the problems I can envisage and can't think of good fixes are

1) It attaches an event handler that never gets removed unless the OOP never dies under us
2) It's only on the clickElement method, there might be other ways to kill a frame which means we dont support it.
3) I send back an "ok" message because we need to assume the test is keeping state of things not us. 

I was hoping to see if you can think of a better more efficient approach to fixing this. Also I should wrap this in a conditional so it only happens on B2G and we try keep other platforms cleaner

Comment 17

[Jonathan Griffin (:jgriffin)]

Comment on attachment 700747 [details] [diff] [review]
v2 - added in support for OOP frames disappearing on us

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

Cool, this works for this particular case!

It's definitely a hack, and I think in the longer term we can improve this, but in the short term, I'd say you could land this with the following modifications:

assign mozBrowserClose to this.mozBrowserClose (which should be set to null by default), and in receiveMessage (which handles messages from the listeners), remove the event listener when this.mozBrowserClose != null.  This will at least prevent us from leaking and piling up these event listeners with every click.

I also think we should probably return an error (probably a new error code that would correspond to something like FrameClosedError in the Python client), rather than ok, since we can't really tell if this is expected or not, and tests can catch the exception if they expect this to occur.

Please also change deletingFrame.htm to have an .html extension, and replace formPage.html with some minimal html page that's needed for the test.

Longer-term, I think we want to set the mozBrowserClose handler on any request from the client when a frame-specific message manager is active, then tear it down on any response from the listener.  But, I think we may want to do that after bug 797529, since it would be easier after some refactoring.

Comment 18

[David Burns :automatedtester]

Try for Desktop

https://tbpl.mozilla.org/?tree=Try&rev=06c6dbdaeb8d

Try for mobile

https://tbpl.mozilla.org/?tree=Try&rev=bcb08563cdf8

Comment 19

[David Burns :automatedtester]

Attachment: v3 - Handling OOP frames from being pulled from us and better support if we get dead compartments

Will set review when try comes in

Comment 20

[Mozilla RelEng Bot]

Try run for bcb08563cdf8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bcb08563cdf8
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bcb08563cdf8

Comment 21

[Mozilla RelEng Bot]

Try run for 06c6dbdaeb8d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=06c6dbdaeb8d
Results (out of 9 total builds):
    success: 8
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-06c6dbdaeb8d

Comment 22

[Jonathan Griffin (:jgriffin)]

Comment on attachment 701339 [details] [diff] [review]
v3 - Handling OOP frames from being pulled from us and better support if we get dead compartments

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

Thanks!  One minor change, but r+ with that fixed.

::: testing/marionette/marionette-actors.js
@@ +1304,5 @@
> +      // call should be aware something isnt right and handle accordingly
> +      let curWindow = this.getCurrentWindow();
> +      let self = this;
> +      this.mozBrowserClose = function() { 
> +        curWindow.removeEventListener('mozbrowserclose', this.mozBrowserClose, true);

This should be 'self.mozBrowserClose', rather than using 'this'.

Comment 23

[David Burns :automatedtester]

landed in 

https://hg.mozilla.org/integration/mozilla-inbound/rev/eafdefd97161

Comment 24

[David Burns :automatedtester]

Attachment: Force marionette to move to window.top if we try access something the compartment is dead

Carrying r+ forward and updated patch attached

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eafdefd97161

Comment 26

[Jonathan Griffin (:jgriffin)]

Uplifted for bug 833764 - https://hg.mozilla.org/releases/mozilla-b2g18/rev/033959896859

Comment 27

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3368006a3e53