Closed
Bug 814768
Opened 12 years ago
Closed 12 years ago
Marionette-actors should be aware of when a frame closes so tests can resume
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
mozilla21
People
(Reporter: aaronmt, Assigned: automatedtester)
References
Details
Attachments
(2 files, 3 obsolete files)
1.84 KB,
text/plain
|
Details | |
9.19 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
This is true for Persona/login too. It self closes the window when you click 'Sign in'.
Comment 2•12 years ago
|
||
Can someone attach a test case that exposes this problem?
Reporter | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
Attachment #685260 -
Attachment mime type: text/x-python-script → text/plain
Comment 5•12 years ago
|
||
Aaron/Zac: is this going to block Marketplace/Identity automation?
Comment 6•12 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•12 years ago
|
Assignee: nobody → dburns
Comment 7•12 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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
Attachment #697179 -
Flags: review?(jgriffin)
Comment 14•12 years ago
|
||
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•12 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•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
Try for Desktop
https://tbpl.mozilla.org/?tree=Try&rev=06c6dbdaeb8d
Try for mobile
https://tbpl.mozilla.org/?tree=Try&rev=bcb08563cdf8
Assignee | ||
Comment 19•12 years ago
|
||
Will set review when try comes in
Attachment #700747 -
Attachment is obsolete: true
Comment 20•12 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•12 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•12 years ago
|
Attachment #701339 -
Flags: review?(jgriffin)
Comment 22•12 years ago
|
||
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 23•12 years ago
|
||
Assignee | ||
Comment 24•12 years ago
|
||
Carrying r+ forward and updated patch attached
Attachment #701339 -
Attachment is obsolete: true
Attachment #702808 -
Flags: review+
Comment 25•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 26•12 years ago
|
||
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox21:
--- → fixed
Comment 27•12 years ago
|
||
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•