Closed Bug 843793 Opened 7 years ago Closed 7 years ago

Throw an error when frame is closed/crashes when using switch_to_frame

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

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

RESOLVED FIXED
mozilla23
Tracking Status
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(1 file, 4 obsolete files)

We see this error a lot in some test runs:

marionette.errors.MarionetteException: {u'message': u'error occurred while processing \'switchToFrame\' request: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://marionette/content/marionette-actors.js :: MDA_sendAsync :: line 205"  data: no]', u'from': u'conn0.marionette1', u'error': u'unknownError'}

This can occur after an app has crashed, but it can also occur when an app closes itself, leaving Marionette attached to an invalid frame.  We should handle this better.
This patch allows switch_to_frame() to work even when the current frame is invalid, which is what I think we want to do here.  Other commands on an invalid frame will still fail, but at least subsequent tests should not be affected, since switch_to_frame() will be called during the next test's setUp.
Attachment #716859 - Flags: review?(mdas)
Comment on attachment 716859 [details] [diff] [review]
Allow switch_to_frame() to work, even when the current frame is invalid,

Is it possible to check if it throws NS_ERROR_NOT_INITIALIZED, and return with an appropriate error to the client, so the client can catch it and deal with it accordingly? It looks like this can catch any kind of exception, and it seems reasonable that there will be cases where we want an error to be thrown if the frame is not yet initialized, or if any other error happens.
(In reply to Malini Das [:mdas] from comment #2)
> Comment on attachment 716859 [details] [diff] [review]
> Allow switch_to_frame() to work, even when the current frame is invalid,
> 
> Is it possible to check if it throws NS_ERROR_NOT_INITIALIZED, and return
> with an appropriate error to the client, so the client can catch it and deal
> with it accordingly? It looks like this can catch any kind of exception, and
> it seems reasonable that there will be cases where we want an error to be
> thrown if the frame is not yet initialized, or if any other error happens.

Yes, I think we need to do something like that.  I intended the patch in this bug to only fix a very specific problem, that is, that switch_to_frame() fails (so you're completely stuck) after the active frame dies (either crashes or deliberately closes itself).  All it does is allow switchToGlobalMessageManager() to succeed even if it can't send "sleepSession" to the current frame.

I totally agree that a larger fix should differentiate between NS_ERROR_NOT_INITIALIZED vs NS_ERROR_FAILURE vs something else, and not just during switch_to_frame().

This patch should really belong in a more focused bug, so we can leave this bug open until we have a more general fix (which will be a bit more complicated); I'll file a new bug and move this patch over there.
Comment on attachment 716859 [details] [diff] [review]
Allow switch_to_frame() to work, even when the current frame is invalid,

Moved this patch over to bug 844942.
Attachment #716859 - Attachment is obsolete: true
Attachment #716859 - Flags: review?(mdas)
Summary: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage] when sending messages to closed frame → Throw an error when frame is closed/crashes when using switch_to_frame
Regarding the title update: 

If we are switching to a frame that is closed or has crashed, we should throw an appropriate error (other than the case in  bug 844942, where we are in a child frame whose parent is dead)
Blocks: b2g-testing
Here's a WIP.  I'll run it through try.
Comment on attachment 732134 [details] [diff] [review]
Throw specific errors when a frame has frozen/crashed

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

Try run was green.  I don't have any way to make a test for this, since we don't have a way of arbitrarily crashing or hanging child processes right now.

Since I had to touch all the calls to sendAsync(), I reformatted them at the same time, using the 'gjslist' rules that the Gaia devs use, which I think makes generally more readable code.
Attachment #732134 - Flags: review?(mdas)
Comment on attachment 732134 [details] [diff] [review]
Throw specific errors when a frame has frozen/crashed

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

::: testing/marionette/marionette-actors.js
@@ +235,5 @@
> +          success = false;
> +          let error = e;
> +          switch(e.result) {
> +            case Components.results.NS_ERROR_FAILURE:
> +              error = new FrameSendNotInitializedError(this.currentRemoteFrame);

shouldn't this send FrameSendFailureError?

@@ +238,5 @@
> +            case Components.results.NS_ERROR_FAILURE:
> +              error = new FrameSendNotInitializedError(this.currentRemoteFrame);
> +              break;
> +            case Components.results.NS_ERROR_NOT_INITIALIZED:
> +              error = FrameSendFailureError(this.currentRemoteFrame);

swap with 239?
Also, is there any reason why we want to pass in a value for ignoreFailure for each call to sendAsync? seems like we can just have it be the last parameter in the function signature, and if it's null, we can assume it's false. Rarely will we want it to be true in any case.
Good catch!  That was a strange sort of dyslexia.
Attachment #734095 - Flags: review?(mdas)
Attachment #732134 - Attachment is obsolete: true
Attachment #732134 - Flags: review?(mdas)
(In reply to Malini Das [:mdas] from comment #10)
> Also, is there any reason why we want to pass in a value for ignoreFailure
> for each call to sendAsync? seems like we can just have it be the last
> parameter in the function signature, and if it's null, we can assume it's
> false. Rarely will we want it to be true in any case.

Oops I missed this comment.  Yes, you're right, let me fix that and upload a new patch.
Removed the extra 'false' parameters from all the sendAsync calls.
Attachment #734110 - Flags: review?(mdas)
Attachment #734095 - Attachment is obsolete: true
Attachment #734095 - Flags: review?(mdas)
pushed to try again, just in case: https://tbpl.mozilla.org/?tree=Try&rev=ca4eb4c2f245
Attachment #734110 - Flags: review?(mdas) → review+
Attachment #734110 - Attachment is obsolete: true
Comment on attachment 736006 [details] [diff] [review]
Throw specific errors when a frame has frozen/crashed,

Carry r+ forward.
Attachment #736006 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5da461e63f72
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.