Closed Bug 984508 Opened 8 years ago Closed 4 years ago

marionette should monitor marionette-listeners to make sure they are still alive

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID
mozilla36

People

(Reporter: mdas, Unassigned)

References

Details

(Keywords: pi-marionette-server)

Attachments

(1 file, 5 obsolete files)

From https://bugzilla.mozilla.org/show_bug.cgi?id=980541#c2, we currently hang on OOP marionette-listeners if they disappear/become unresponsive, so we'll need to add some kind of monitoring on them so we can recover and respond to the client.
Assignee: nobody → rwood
Attached patch apply this first (obsolete) — Splinter Review
To reproduce this, you can start a marionette session, open an app and switch into it, and then execute: m.execute_async_script("setTimeout(function() {marionetteScriptFinished();}, 7000);window.close();")

Which will close the app, and the session will hang.

Regarding the questions brought up from our talk:
* check if executeScript will hang: what happens is the server will send pings but get no response. there should be a periodic function that runs in the server that checks the (currentTime - lastReceivedMessage) time, and if it exceeds the timeout, then it will return a response.

it would also be super cool to add a function to set the timeout of this value.

* check if frame-manager needs to be updated: Yeah, the server should tell the frame manager to remove the currentRemoteFrame from the remoteFrames list.

* define how we recover: let's return the user to the top-most root process. Let's notify them in the error message we send back. Something like "frame with url:<url> is unresponsive or crashed. Returning to the root frame"

When you start working on this patch, please apply this patch first. It fixes a bug in our frame status detection code.

Also, fyi, when you add a new listener to marionette-server ("Marionette:pong" in this case), you need to update the marionette-frame-manager to add and remove these listeners https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-frame-manager.js#167. We do this because we want to send/listen to messages from the current content frame, and we need to register using its specific message manager.
(In reply to Malini Das [:mdas] from comment #1)
> 
> it would also be super cool to add a function to set the timeout of this
> value.
> 

An API the client can use like "setFrameTimeout" or something
Malini, this sounds the same as https://bugzilla.mozilla.org/show_bug.cgi?id=1065933

If so can we bump this up to high/urgent priority as it is causing us to lose test coverage.
Flags: needinfo?(mdas)
Seems different. If the underlying process crashed, we'd see an NS_ERROR_NOT_INITIALIZED in the logs when the marionette-server was trying to talk to that frame.

Even if a failure is due to an underlying frame crash, then the patch for this bug won't change much. It would make marionette return an error immediately instead of timing out, which would help you with recovery, but won't really tell you why the frame crashed. You'd still need to look at the logs/crash logs to figure out what caused the app to crash.
Flags: needinfo?(mdas)
Attached patch bug984508wip.patch (obsolete) — Splinter Review
Attaching WIP, with some debugging output so you can see it work in logcat.

- heartbeat starts when switch to OOP frame

- if the OOP window is closed unexpectedly, frame will timeout; switches back to root frame; and frame is removed from the remote frames list

- frame timeout is 5000 ms by default, but can set via new client method setFrameTimeout

- heartbeat stops if switch out of OOP frame

A couple of things I need direction with:

- I'm not sure how to catch an actual sendAsync error when sending the ping, aside from the return value

- I'm not sure how to send an error back to the client, when the frame times out (you mentioned we can't use sendError in this case?)

- I want to store the current remote frame's URL so when it times out the URL name can be passed to the client in the error message; not sure how to get that/couldn't figure out how to getCurrentUrl and get the response from within another method

Thanks in advance :mdas for your feedback!
Attachment #8500516 - Flags: feedback?(mdas)
Comment on attachment 8500516 [details] [diff] [review]
bug984508wip.patch

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

Looks good so far!

nit: Weird whitespace issues.

::: testing/marionette/marionette-server.js
@@ +2531,5 @@
> +   * Start the OOP frame heartbeat
> +   */
> +  startHeartbeat: function MDA_startHeartbeat() {
> +    logger.info("Starting OOP frame heartbeat");
> +    this.frameHeartbeatLastUrl = "(frame URL)"; // TODO: How to get the Url?

getting it from here is difficult, what we can do is have marionette-frame-manager.js return it to us when we call switchToFrame on it and we'll store it then. Instead of the url, it can get the mozapp which is better since the url may change. You can do that by returning the oopFrame.getAttribute("mozapp") value

@@ +2533,5 @@
> +  startHeartbeat: function MDA_startHeartbeat() {
> +    logger.info("Starting OOP frame heartbeat");
> +    this.frameHeartbeatLastUrl = "(frame URL)"; // TODO: How to get the Url?
> +    this.frameHeartbeatLastPong = new Date().getTime();
> +    this.sendAsync("ping", {}, this.command_id);

I'd remove this and just rely on the pulse() function in the event.

@@ +2556,5 @@
> +          logger.info("Target OOP frame not responding")
> +        }
> +	    this.stopHeartbeat();
> +        this.curBrowser.frameManager.removeRemoteFrame(this.curBrowser.frameManager.currentRemoteFrame.frameId);
> +        this.switchToFrame({parameters:{id:null, focus:true}});

calling this.switchToFrame will send a message to the client (https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#1476), which is not what we want to do. What you should do here is just call this.switchToGlobalMessageManager()
Attachment #8500516 - Flags: feedback?(mdas) → feedback+
(In reply to Robert Wood [:rwood] from comment #5)
> 
> A couple of things I need direction with:
> 
> - I'm not sure how to catch an actual sendAsync error when sending the ping,
> aside from the return value
> 
> - I'm not sure how to send an error back to the client, when the frame times
> out (you mentioned we can't use sendError in this case?)
> 

For both these concerns, what you can do is modify sendAsync to take in a parameter, say throwError (default to false). You can call it with throwError set to true, and in that case, sendAsync will throw an error instead of sending a message back to the client. You can modify https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#231 to check the value of throwError here. Then, after handling the switch back to main frame, you can send an error message about the frame crash to the client using this.sendError.

> - I want to store the current remote frame's URL so when it times out the
> URL name can be passed to the client in the error message; not sure how to
> get that/couldn't figure out how to getCurrentUrl and get the response from
> within another method
> 
I added a suggestion in the patch review. We should use mozapp.
> Thanks in advance :mdas for your feedback!

Also, remember to fold  the "apply this first" patch into your patch. It's needed for the frame exception handling to work, otherwise you'll get W/GeckoConsole(  292): [JavaScript Error: "TypeError: "toString" is read-only" {file: "chrome://marionette/content/marionette-server.js" line: 88}] errors
Attached patch bug984508.patch (obsolete) — Splinter Review
Thanks Malini.
Attachment #8491073 - Attachment is obsolete: true
Attachment #8500516 - Attachment is obsolete: true
Attachment #8505080 - Flags: review?(mdas)
Comment on attachment 8505080 [details] [diff] [review]
bug984508.patch

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

Nice patch so far, but there is a bug:

Open an app on your b2g device, start a marionette session, switch to its frame, and then manually kill the app (I did this by holding down home and pressed the X in cardview). When the client makes another request, say execute_script("return 1"), they get the frame closed exception. Good... but let's say the client then asks for get_url(). They get 1 instead! The commands are out of sync. You can fix this (or at least get started fixing it) by not assigning new command ids as described in the giant paragraph I wrote :)

::: testing/marionette/client/marionette/tests/unit/test_set_frame_timeout.py
@@ +8,5 @@
> +class TestSetFrameTimeout(MarionetteTestCase):
> +
> +    def tearDown(self):
> +        self.marionette.set_frame_timeout(5000)
> +        super(MarionetteTestCase, self).tearDown()

you don't need this since a new marionette session is created with each test. The frame timeout should be reset

::: testing/marionette/marionette-frame-manager.js
@@ +107,5 @@
> +    try {
> +      appName = oopFrame.getAttribute("mozapp");
> +    }
> +    catch(e) {
> +      appName = "(unavailable)";

can this be "mozapp name unavailable"? It will be clearer to the user when they receive any error messages if this frame dies.

@@ +131,5 @@
>        if (frameMessageManager == mm) {
>          this.currentRemoteFrame = frame;
>          this.addMessageManagerListeners(mm);
>          mm.sendAsyncMessage("Marionette:restart", {});
> +        return appName;

This has changed recently, where we return oopFrame id. Instead, here and on line 148, we shoudl return:

return [oopFrame.id, appName];

::: testing/marionette/marionette-server.js
@@ +202,5 @@
>     *        Suffix of the targetted message listener (Marionette:<suffix>)
>     * @param object values
>     *        Object to send to the listener
>     */
> +  sendAsync: function MDA_sendAsync(name, values, commandId, ignoreFailure, throwError=false) {

default parameters are only part of the draft of ECMAScript 6 (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters#Browser_compatibility)... Even though it should be fine since we implement this anyway, I'd be more comfortable using them when they become spec standard. Can this be changed to assigning default values in the function?

@@ +2510,5 @@
>    /**
> +   * Sets the OOP frame timeout value (ms)
> +   */
> +  setFrameTimeout: function MDA_setFrameTimeout(aRequest) {
> +    logger.info("setFrameTimeout");

remove this log line

@@ +2536,5 @@
>    /**
> +   * Start the OOP frame heartbeat
> +   */
> +  startHeartbeat: function MDA_startHeartbeat() {
> +    logger.info("Starting OOP frame heartbeat");

remove log line

@@ +2541,5 @@
> +    this.frameHeartbeatLastPong = new Date().getTime();
> +    function pulse() {
> +      let noResponse = false;
> +      let now = new Date().getTime();
> +      let elapsed = now - this.frameHeartbeatLastPong;

Can you move this line to below the getCommandId()? That will be a more accurate representation of the time elapsed

@@ +2542,5 @@
> +    function pulse() {
> +      let noResponse = false;
> +      let now = new Date().getTime();
> +      let elapsed = now - this.frameHeartbeatLastPong;
> +      this.command_id = this.getCommandId();

We want to use the currently active command_id. Otherwise, we will be sending messages out of sync...

Here's a rundown of how command_ids work and their purpose:

A client with a session requests the server to do something. The server assigns that request a command_id. There can only ever be one command_id active at one time, since the client can only request one thing at a time. So if the client requests the current url (get_url()), then the server assigns this request some command_id, then messages the listener this command_id along with the "getCurrentUrl" message. It then waits to get a message back from teh client, and it verifies that the command_id is the one that's currently active. If so, it will respond back to the client. If it receives a command_id from the client that's not the currently active Id, it just ignores it and doesn't send it back to the client.

This was initially introduced due to executeAsyncScript calls that would take too long. In that case, if the script took too long, you send the client a timeout and if the script returns, you disregard anything you were supposed to return. Instead, if the script ended up finishing after the timeout, you'd get the listener messaging the server after the timeout saying 'hey! i finally finished and here's what you should send back to the client!' but the client already got a response. In this case, when the client makes subsequent request, it would receive the response of its timed-out execute script, and it was all very confusing.

Anyway, we don't want to create a new command id. What we want to do is send back an exception if a) there is an active command_id (ie: if there is an active request) or failing that, then b) on the next request. This means if my frame dies while the client is waiting for a response (for example, if the client is execute a long script), then the client should receive the exception immediately. If the frame dies while the client has no active requests, then we should wait for the next time the client requests something of us, and instead send this exception.

@@ +2545,5 @@
> +      let elapsed = now - this.frameHeartbeatLastPong;
> +      this.command_id = this.getCommandId();
> +      try {
> +        if (elapsed > this.frameTimeout) {
> +          throw {message:null, code:500, stack:null};

How about instead of throwing 500 here and below, we throw a custom error code? Consumers seem to like those.

You can add a new error code here:
https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/errors.py, we can use 56 for its code, and maybe FRAME_NOT_RESPONDING for its name

@@ +2557,5 @@
> +      catch (e) {
> +        let lastApp = "(undefined)";
> +        if (this.frameHeartbeatLastApp !== null) {
> +          lastApp = this.frameHeartbeatLastApp;
> +        }

JS has a cool trick where you can replace these assignments, using a one-line conditional assignment like this:

let lastApp = this.frameHeartbeatLastApp ? this.frameHeartbeatLastApp : "undefined";

And I'm guessing we want to remove the parentheses around "undefined" because they get added in errorTxt

@@ +2565,5 @@
> +        let errorTxt = "Frame not responding (" + lastApp + "), switching to root frame";
> +        this.sendError(errorTxt, e.code, e.stack, this.command_id);
> +        return;
> +      }
> +      return;

return statement is not needed here; it is implicit

@@ +2576,5 @@
> +   * Stop the OOP frame heartbeat
> +   */
> +  stopHeartbeat: function MDA_stopHeartbeat() {
> +    if (this.frameHeartbeatTimer !== null) {
> +      logger.info("Stopping OOP frame heartbeat");

remove log line

@@ +2618,5 @@
>        case "Marionette:runEmulatorShell":
>          this.sendToClient(message.json, -1);
>          break;
>        case "Marionette:switchToFrame":
> +        this.frameHeartbeatLastApp = this.curBrowser.frameManager.switchToFrame(message);

The code has changed here recently, since we now return oopFrame.id from the frame manager. You can update switchToFrame to return both oopFrame.Id and the appname. Then you can do: 

[this.oopFrameId, this.frameHeartbeatLastApp] = this.curBrowser.frameManager.switchToFrame(message);

here
Attachment #8505080 - Flags: review?(mdas) → review-
(In reply to Malini Das [:mdas] from comment #9)
> Comment on attachment 8505080 [details] [diff] [review]
> bug984508.patch
> 
> Review of attachment 8505080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice patch so far, but there is a bug:
> 
> Open an app on your b2g device, start a marionette session, switch to its
> frame, and then manually kill the app (I did this by holding down home and
> pressed the X in cardview). When the client makes another request, say
> execute_script("return 1"), they get the frame closed exception. Good... but
> let's say the client then asks for get_url(). They get 1 instead! The
> commands are out of sync. You can fix this (or at least get started fixing
> it) by not assigning new command ids as described in the giant paragraph I
> wrote :)

Thanks Mdas! Not creating a new command_id makes sense, and I'll make that change. However, isn't the above expected behavior? i.e. Scenario one, the frame unexpectedly closes while no request is in progress, then when the next request happens, respond with the exception. Or, scenario two, the frame unexpectedly closes during a request, like a long execute_script, then respond immediately with the exception. Either way, at that point we have switched back to the root/system frame to recover, so isn't it ok that the next request (i.e. the get_url above) returns a valid response?

> 
> ::: testing/marionette/client/marionette/tests/unit/test_set_frame_timeout.py
> @@ +8,5 @@
> > +class TestSetFrameTimeout(MarionetteTestCase):
> > +
> > +    def tearDown(self):
> > +        self.marionette.set_frame_timeout(5000)
> > +        super(MarionetteTestCase, self).tearDown()
> 
> you don't need this since a new marionette session is created with each
> test. The frame timeout should be reset
> 
> ::: testing/marionette/marionette-frame-manager.js
> @@ +107,5 @@
> > +    try {
> > +      appName = oopFrame.getAttribute("mozapp");
> > +    }
> > +    catch(e) {
> > +      appName = "(unavailable)";
> 
> can this be "mozapp name unavailable"? It will be clearer to the user when
> they receive any error messages if this frame dies.
> 
> @@ +131,5 @@
> >        if (frameMessageManager == mm) {
> >          this.currentRemoteFrame = frame;
> >          this.addMessageManagerListeners(mm);
> >          mm.sendAsyncMessage("Marionette:restart", {});
> > +        return appName;
> 
> This has changed recently, where we return oopFrame id. Instead, here and on
> line 148, we shoudl return:
> 
> return [oopFrame.id, appName];
> 
> ::: testing/marionette/marionette-server.js
> @@ +202,5 @@
> >     *        Suffix of the targetted message listener (Marionette:<suffix>)
> >     * @param object values
> >     *        Object to send to the listener
> >     */
> > +  sendAsync: function MDA_sendAsync(name, values, commandId, ignoreFailure, throwError=false) {
> 
> default parameters are only part of the draft of ECMAScript 6
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/
> Default_parameters#Browser_compatibility)... Even though it should be fine
> since we implement this anyway, I'd be more comfortable using them when they
> become spec standard. Can this be changed to assigning default values in the
> function?
> 
> @@ +2510,5 @@
> >    /**
> > +   * Sets the OOP frame timeout value (ms)
> > +   */
> > +  setFrameTimeout: function MDA_setFrameTimeout(aRequest) {
> > +    logger.info("setFrameTimeout");
> 
> remove this log line
> 
> @@ +2536,5 @@
> >    /**
> > +   * Start the OOP frame heartbeat
> > +   */
> > +  startHeartbeat: function MDA_startHeartbeat() {
> > +    logger.info("Starting OOP frame heartbeat");
> 
> remove log line
> 
> @@ +2541,5 @@
> > +    this.frameHeartbeatLastPong = new Date().getTime();
> > +    function pulse() {
> > +      let noResponse = false;
> > +      let now = new Date().getTime();
> > +      let elapsed = now - this.frameHeartbeatLastPong;
> 
> Can you move this line to below the getCommandId()? That will be a more
> accurate representation of the time elapsed
> 
> @@ +2542,5 @@
> > +    function pulse() {
> > +      let noResponse = false;
> > +      let now = new Date().getTime();
> > +      let elapsed = now - this.frameHeartbeatLastPong;
> > +      this.command_id = this.getCommandId();
> 
> We want to use the currently active command_id. Otherwise, we will be
> sending messages out of sync...
> 
> Here's a rundown of how command_ids work and their purpose:
> 
> A client with a session requests the server to do something. The server
> assigns that request a command_id. There can only ever be one command_id
> active at one time, since the client can only request one thing at a time.
> So if the client requests the current url (get_url()), then the server
> assigns this request some command_id, then messages the listener this
> command_id along with the "getCurrentUrl" message. It then waits to get a
> message back from teh client, and it verifies that the command_id is the one
> that's currently active. If so, it will respond back to the client. If it
> receives a command_id from the client that's not the currently active Id, it
> just ignores it and doesn't send it back to the client.
> 
> This was initially introduced due to executeAsyncScript calls that would
> take too long. In that case, if the script took too long, you send the
> client a timeout and if the script returns, you disregard anything you were
> supposed to return. Instead, if the script ended up finishing after the
> timeout, you'd get the listener messaging the server after the timeout
> saying 'hey! i finally finished and here's what you should send back to the
> client!' but the client already got a response. In this case, when the
> client makes subsequent request, it would receive the response of its
> timed-out execute script, and it was all very confusing.
> 
> Anyway, we don't want to create a new command id. What we want to do is send
> back an exception if a) there is an active command_id (ie: if there is an
> active request) or failing that, then b) on the next request. This means if
> my frame dies while the client is waiting for a response (for example, if
> the client is execute a long script), then the client should receive the
> exception immediately. If the frame dies while the client has no active
> requests, then we should wait for the next time the client requests
> something of us, and instead send this exception.
> 
> @@ +2545,5 @@
> > +      let elapsed = now - this.frameHeartbeatLastPong;
> > +      this.command_id = this.getCommandId();
> > +      try {
> > +        if (elapsed > this.frameTimeout) {
> > +          throw {message:null, code:500, stack:null};
> 
> How about instead of throwing 500 here and below, we throw a custom error
> code? Consumers seem to like those.
> 
> You can add a new error code here:
> https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette/errors.py, we can use 56 for its code, and maybe
> FRAME_NOT_RESPONDING for its name
> 
> @@ +2557,5 @@
> > +      catch (e) {
> > +        let lastApp = "(undefined)";
> > +        if (this.frameHeartbeatLastApp !== null) {
> > +          lastApp = this.frameHeartbeatLastApp;
> > +        }
> 
> JS has a cool trick where you can replace these assignments, using a
> one-line conditional assignment like this:
> 
> let lastApp = this.frameHeartbeatLastApp ? this.frameHeartbeatLastApp :
> "undefined";
> 
> And I'm guessing we want to remove the parentheses around "undefined"
> because they get added in errorTxt
> 
> @@ +2565,5 @@
> > +        let errorTxt = "Frame not responding (" + lastApp + "), switching to root frame";
> > +        this.sendError(errorTxt, e.code, e.stack, this.command_id);
> > +        return;
> > +      }
> > +      return;
> 
> return statement is not needed here; it is implicit
> 
> @@ +2576,5 @@
> > +   * Stop the OOP frame heartbeat
> > +   */
> > +  stopHeartbeat: function MDA_stopHeartbeat() {
> > +    if (this.frameHeartbeatTimer !== null) {
> > +      logger.info("Stopping OOP frame heartbeat");
> 
> remove log line
> 
> @@ +2618,5 @@
> >        case "Marionette:runEmulatorShell":
> >          this.sendToClient(message.json, -1);
> >          break;
> >        case "Marionette:switchToFrame":
> > +        this.frameHeartbeatLastApp = this.curBrowser.frameManager.switchToFrame(message);
> 
> The code has changed here recently, since we now return oopFrame.id from the
> frame manager. You can update switchToFrame to return both oopFrame.Id and
> the appname. Then you can do: 
> 
> [this.oopFrameId, this.frameHeartbeatLastApp] =
> this.curBrowser.frameManager.switchToFrame(message);
> 
> here
Flags: needinfo?(mdas)
Attached patch bug984508_take2.patch (obsolete) — Splinter Review
Attachment #8514995 - Flags: review?(mdas)
Comment on attachment 8514995 [details] [diff] [review]
bug984508_take2.patch

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

awesome! Works perfectly
Attachment #8514995 - Flags: review?(mdas) → review+
Flags: needinfo?(mdas)
For some reason I cannot reproduce the try failures locally on device or on emulator (will keep trying).

Regardless, from looking at the try logs and code, it looks like this is a problem when switching into an iframe; more specifically when a test opens a local webpage/url via marionette.navigate(local_html). From what I can tell in the code, in this case it is not a typical switch to frame; instead function get() is called in the listener, and that changes the curFrame.location to the local URL. I'm guessing marionette commands are then forwarded to that frame, but the listener for the heartbeat ping isn't in that frame, so the ping isn't received (eventually causing the OOP frame exception). Mdas, does this sound like a plausible theory?
Flags: needinfo?(mdas)
I was running the tests with the wrong manifest, and so they weren't running in the test container, hence why I couldn't reproduce the failure (thanks Mdas!). The failure can be reproduced but not at the same place in the tests, looking into it more...
Flags: needinfo?(mdas)
Ok so one issue here, the OOP heartbeat (pinging the test container) was timing out in between marionette unit test runs - when the marionette session was deleted, the heartbeat ping / timer was not stopped and attempted to continue anyway (and would timeout) so I fixed that. Still having the heartbeat timeout when loading a local URL, investigating...
Ahh, the OOP frame heartbeat is timing out because loading the local URL test file is taking longer than 500ms / the default heartbeat frame timeout. When loading a URL / calling get, I'll stop the OOP frame heartbeat and then start it again after the URL is loaded.
Got it... I was setting this.frameHeartbeatLastPong to null in function pulse(); so if the current pulse timer expired before the pong was received and frameHeartbeatLastPong updated, the elapsed time was not being calculated correctly (elapsed was the same as the current time) causing the frametimeout exception. New patch coming soon!
Attached patch bug984508_take3.patch (obsolete) — Splinter Review
Add fixes.
Attachment #8505080 - Attachment is obsolete: true
Attachment #8514995 - Attachment is obsolete: true
Attachment #8524139 - Flags: review?(mdas)
Comment on attachment 8524139 [details] [diff] [review]
bug984508_take3.patch

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

this looks good so far, but as discussed on irc, we need to handle the case where the listener gets interrupted with a modal dialog. punting my r? till then.

::: testing/marionette/marionette-server.js
@@ +726,5 @@
>        this.sendOk(this.command_id);
> +      // Stop the OOP frame heartbeat if switched into chrome
> +      if (context == "chrome") {
> +        this.stopHeartbeat();
> +      }      

nit: extra whitespace after }
Attachment #8524139 - Flags: review?(mdas)
(In reply to Malini Das [:mdas] from comment #23)
> Comment on attachment 8524139 [details] [diff] [review]
> bug984508_take3.patch
> 
> Review of attachment 8524139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks good so far, but as discussed on irc, we need to handle the case
> where the listener gets interrupted with a modal dialog. punting my r? till
> then.
> 
> ::: testing/marionette/marionette-server.js
> @@ +726,5 @@
> >        this.sendOk(this.command_id);
> > +      // Stop the OOP frame heartbeat if switched into chrome
> > +      if (context == "chrome") {
> > +        this.stopHeartbeat();
> > +      }      
> 
> nit: extra whitespace after }

Hmmm it seems to work ok. When I start an OOP frame/app, switch into that frame (so the heartbeat starts), and then do this:

marionette.execute_script("""alert('hi');""")

The modal alert dialog appears, but even if I leave the dialog active the heartbeat continues fine. Same if I do window.confirm instead of alert. Also if I start the settings app, switch to that frame, then change a setting to cause a modal confirmation dialog to appear (i.e. settings, cellular & data, turn data connection on) the heartbeat keeps going.
(In reply to Robert Wood [:rwood] from comment #24)
> (In reply to Malini Das [:mdas] from comment #23)
> > Comment on attachment 8524139 [details] [diff] [review]
> > bug984508_take3.patch
> > 
> > Review of attachment 8524139 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > this looks good so far, but as discussed on irc, we need to handle the case
> > where the listener gets interrupted with a modal dialog. punting my r? till
> > then.
> > 
> > ::: testing/marionette/marionette-server.js
> > @@ +726,5 @@
> > >        this.sendOk(this.command_id);
> > > +      // Stop the OOP frame heartbeat if switched into chrome
> > > +      if (context == "chrome") {
> > > +        this.stopHeartbeat();
> > > +      }      
> > 
> > nit: extra whitespace after }
> 
> Hmmm it seems to work ok. When I start an OOP frame/app, switch into that
> frame (so the heartbeat starts), and then do this:
> 
> marionette.execute_script("""alert('hi');""")
> 
> The modal alert dialog appears, but even if I leave the dialog active the
> heartbeat continues fine. Same if I do window.confirm instead of alert. Also
> if I start the settings app, switch to that frame, then change a setting to
> cause a modal confirmation dialog to appear (i.e. settings, cellular & data,
> turn data connection on) the heartbeat keeps going.

Was that the scenario you had in mind, MDas?
Flags: needinfo?(mdas)
Comment on attachment 8524139 [details] [diff] [review]
bug984508_take3.patch

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

Interesting! I've been playing around with it and I think the behaviour of modals changed... we won't need to worry about it! Ship this, but clear that whitespace nit. \o/
Attachment #8524139 - Flags: review+
Thanks MDas! New patch with whitespace removed, just carrying fw the R+
Attachment #8524139 - Attachment is obsolete: true
Flags: needinfo?(mdas)
Comment on attachment 8526218 [details] [diff] [review]
bug984508_take4.patch

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

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +131,5 @@
>  [test_profile_management.py]
>  b2g = false
>  [test_set_window_size.py]
>  b2g = false
> +[test_set_frame_timeout.py]

This needs to be one line down so it doesn't re-enable test_set_window_size on linux.
(That's bug 1104301.)
(In reply to Chris Manchester [:chmanchester] from comment #30)
> Comment on attachment 8526218 [details] [diff] [review]
> bug984508_take4.patch
> 
> Review of attachment 8526218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
> @@ +131,5 @@
> >  [test_profile_management.py]
> >  b2g = false
> >  [test_set_window_size.py]
> >  b2g = false
> > +[test_set_frame_timeout.py]
> 
> This needs to be one line down so it doesn't re-enable test_set_window_size
> on linux.

Fixed in bug 1104301 as https://hg.mozilla.org/integration/mozilla-inbound/rev/b93c92150721
https://hg.mozilla.org/mozilla-central/rev/5671737e4f24
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1104301
Depends on: 1104624
No longer depends on: 1104301
Backed out both changesets because of intermittent failures (Bug 1104624):

https://hg.mozilla.org/integration/mozilla-inbound/rev/96b859f97454
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ec914bf55f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: rwood → nobody
This isn’t necessary.  The content frame script only disappears when
a remoteness change occurs, and because we use the aDelayedLoad flag
for mm.loadFrameScript, it gets reloaded when the new frame spawns.
Status: REOPENED → RESOLVED
Closed: 7 years ago4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.