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

RESOLVED FIXED in Firefox 21

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: automatedtester)

Tracking

unspecified
mozilla21
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
This is true for Persona/login too. It self closes the window when you click 'Sign in'.
Can someone attach a test case that exposes this problem?
(Reporter)

Comment 3

6 years ago
Created attachment 685260 [details]
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
(Reporter)

Updated

6 years ago
Attachment #685260 - Attachment mime type: text/x-python-script → text/plain

Updated

6 years ago
Duplicate of this bug: 816830
Aaron/Zac: is this going to block Marketplace/Identity automation?

Comment 6

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

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

Updated

6 years ago
Assignee: nobody → dburns
(Assignee)

Updated

6 years ago
Depends on: 821303
(Assignee)

Updated

6 years ago
No longer depends on: 821303

Comment 7

6 years ago
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/dburns@mozilla.com-87b4fc0bde35

Comment 8

6 years ago
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/dburns@mozilla.com-7020f3ce97ba

Comment 9

6 years ago
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/dburns@mozilla.com-87b4fc0bde35

Comment 10

6 years ago
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/dburns@mozilla.com-5a226b4dc208

Comment 11

6 years ago
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/dburns@mozilla.com-787dff9219a5

Comment 12

6 years ago
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/dburns@mozilla.com-02323d3760b3
(Assignee)

Comment 13

6 years ago
Created attachment 697179 [details] [diff] [review]
Force marionette to move to window.top if we try access something the compartment is dead
Attachment #697179 - Flags: review?(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).
Attachment #697179 - Flags: review?(jgriffin) → review-

Comment 15

6 years ago
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/dburns@mozilla.com-02323d3760b3
(Assignee)

Comment 16

6 years ago
Created attachment 700747 [details] [diff] [review]
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
Attachment #697179 - Attachment is obsolete: true
Attachment #700747 - Flags: feedback?(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.
Attachment #700747 - Flags: feedback?(jgriffin) → feedback+
(Assignee)

Comment 19

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

Will set review when try comes in
Attachment #700747 - Attachment is obsolete: true

Comment 20

6 years ago
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/dburns@mozilla.com-bcb08563cdf8

Comment 21

6 years ago
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/dburns@mozilla.com-06c6dbdaeb8d
(Assignee)

Updated

6 years ago
Attachment #701339 - Flags: review?(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'.
Attachment #701339 - Flags: review?(jgriffin) → review+
(Assignee)

Comment 24

6 years ago
Created attachment 702808 [details] [diff] [review]
Force marionette to move to window.top if we try access something the compartment is dead

Carrying r+ forward and updated patch attached
Attachment #701339 - Attachment is obsolete: true
Attachment #702808 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/eafdefd97161
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Uplifted for bug 833764 - https://hg.mozilla.org/releases/mozilla-b2g18/rev/033959896859
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → fixed
Blocks: 833764
You need to log in before you can comment on or make changes to this bug.