Closed Bug 779284 Opened 12 years ago Closed 11 years ago

Implement B2G Modal dialog handling to Marionette

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(firefox24 wontfix, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g18 affected, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd affected)

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g18 --- affected
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- affected

People

(Reporter: automatedtester, Assigned: mdas)

References

Details

(Whiteboard: [u=marionette-user c=frames p=5][xfail])

Attachments

(2 files, 7 obsolete files)

rolled up patches for m-c
58.28 KB, patch
mdas
: review+
Details | Diff | Splinter Review
rolled up patches for mozilla-b2g18
55.15 KB, patch
mdas
: review+
Details | Diff | Splinter Review 
We need to be able to handle alerts and prompts that pages can created
OS: Mac OS X → All
Hardware: x86 → All
The way how we handle those instances of modal dialogs (except tab modal dialogs) with Mozmill you can find here:

http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/modal-dialog.js
This seems like a pretty major blocker for Marionette. Are there any plans to work on this?

Or has a workaround been found?
This is not currently needed in B2G so hasnt been a priority. Do you have a need for it in B2G?
No, we were going to try to use Marionette to test are add-on.

Are you saying that Marionette is only suitable for B2G?
Marionette bugs and features that are needed for B2G automation currently take highest priority, since we don't have the resources to fix all that needs to be done at this point.

Unfortunately, since this feature isn't needed in B2G at the moment, it's on the backburner.
If we supplied a patch, would it be reviewed and accepted?
Sure. Please make sure to add relevant test cases to the patch so we can see that it works and to make sure we dont have regressions.
This is now showing up in B2G. In the Contacts app, if you wish to remove a contact, you are prompted with a dialog:
<form data-type="confirm" role="dialog" class="" id="confirmation-message"> 

And we can't yet interact with it, so we can't automate removal of contacts.
Changed importance to 'major', as this is blocking the development of a gaia ui stress test for deleting contacts (as mdas noted above) and possibly other similar tests also. Thanks!
Severity: normal → major
fyi, through the magic of HTML5 (data-* and role attributes), it makes a modal dialog through: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/modal_dialog.js#L195
looking at this bug and bug 849183 I dont think they are related now. The B2G dialog, as far as I can see is nor more than dialog that disables all items that can be disabled. While we can handle this in this bug it will not be a true modal (as in window.alert/prompt/confirm) and we can already handle this scenario with Marionette.

I think the real reason for :rwood's issue, and in bug 849183, is that we arent firing events in the manner that it expects them.
(In reply to David Burns :automatedtester from comment #12)
> I think the real reason for :rwood's issue, and in bug 849183, is that we
> arent firing events in the manner that it expects them.

Are you setting explicitOriginalTarget?  We depend on that in ConfirmDialog inmail-common.js.
Modal dialogs now pause processing of the frames that triggered them. As a result, if you're running some marionette commands that trigger a modal dialog, like in Bug 877397, then you won't get a response back from marionette until the dialog is closed manually. We'll need some way to detect when a dialog has started and then switch into the system app frame and get a reference to the dialog.

From looking at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/modal_dialog.js, we might be able to listen for 'mozbrowsershowmodalprompt' from chrome and from there switch to the right frame.
I still have to see if we can catch the mozbrowsershowmodalprompt event, but if we can, then we're stuck with a bit of a problem, which is that if you triggered the modal dialog due to a 'touchend' event, then right after the dialog is closed and the app frame resumes, marionette will dispatch the mousemove/mousedown/mouseup events and then send a response back to the client. Stopping the response from the client can be prevented by checking some state. The event dispatching problem can be resolved by Bug 853622, but may be temporarily be worked around by checking the same state and dispatching the mouse events if a dialog did not appear.
As an update, we can catch 'mozbrowsershowmodalprompt' from the system app, and we're going to have to add some potentially ugly code to handle the switching of frames to the system app, then back to the previous app when we close the dialog (we also need to figure out how to reliably be signaled/detect the closing of the dialog) since this code is specific to B2G, and switching to the system app frame and back requires switching between OOP frames at times. 

This code may have to change when Bug 779284 lands
mdas, AutomatedTester and I had an IRC chat about these today.

The proposal:  require tests to explicitly switch frames to handle the modal dialog (which is implemented in the system app).  A test would look something like this:

element.tap() # this triggers modal dialog
marionette.switch_to_frame() # switch to top-level frame
marionette.switch_to_frame(system_app_frame)
# add code here to deal with modal dialog
marionette.switch_to_frame()
marionette.switch_to_frame(previous_app_frame)

To implement this solution, we need the following:

1) We need to detect when the current frame freezes so that if a marionette command is in-progress (as with tap) when the frame freezes, we send an "ok" response from marionette-server, rather than waiting for the response to come from marionette-listener, which won't happen because the listener is running in the frozen frame.

2) We make it so that marionette.switch_to_frame() (top-level) can work even when the current frame is frozen.

2) is "easy", 1) is a little harder.  We'd probably have to load a separate frame script into every frame that watches for mozbrowsershowmodalprompt and notifies marionette-server when that happens.

We could also opt for a more Mozmill-like approach, which would be much cleaner to implement, but a little less webdriver-like to work with.  In this approach, we'd define a JS callback to be executed when the alert appeared:

marionette.switch_to_frame() # top-level frame
marionette.switch_to_frame(system_app_frame)
marionette.handle_modal_dialog("""
   // javascript which will interact with the dialog when it appears
""")
marionette.switch_to_frame()
marionette.switch_to_frame(some_app_frame)
element = marionette.findXXX()
element.tap() # triggers modal dialog

Here, there is no frame switching needed at the point where the modal dialog appears, as the JS handler is already loaded in the correct frame and gets called automatically when the modal dialog pops up.
(In reply to Jonathan Griffin (:jgriffin) from comment #18)
> mdas, AutomatedTester and I had an IRC chat about these today.
> 
> The proposal:  require tests to explicitly switch frames to handle the modal
> dialog (which is implemented in the system app).  A test would look
> something like this:
> 
> element.tap() # this triggers modal dialog
> marionette.switch_to_frame() # switch to top-level frame
> marionette.switch_to_frame(system_app_frame)
> # add code here to deal with modal dialog
> marionette.switch_to_frame()
> marionette.switch_to_frame(previous_app_frame)
> 
> To implement this solution, we need the following:
> 
> 1) We need to detect when the current frame freezes so that if a marionette
> command is in-progress (as with tap) when the frame freezes, we send an "ok"
> response from marionette-server, rather than waiting for the response to
> come from marionette-listener, which won't happen because the listener is
> running in the frozen frame.
> 
> 2) We make it so that marionette.switch_to_frame() (top-level) can work even
> when the current frame is frozen.


Either of these sound good to me.


> 
> 2) is "easy", 1) is a little harder.  We'd probably have to load a separate
> frame script into every frame that watches for mozbrowsershowmodalprompt and
> notifies marionette-server when that happens.
> 
> We could also opt for a more Mozmill-like approach, which would be much
> cleaner to implement, but a little less webdriver-like to work with.  In
> this approach, we'd define a JS callback to be executed when the alert
> appeared:

If we do this I would want this to happen entirely remote. The WebDriver approach is that we take on the hard work so that the people using it dont have to. Maybe it's just me but asking users to do

marionette.handle_modal_dialog("""
   // javascript which will interact with the dialog when it appears
""")

is a recipe for disaster, unless of course the user never needs to call handle_modal_dialog and is hidden in an alert/modal dialog abstraction. In WebDriver we have a nice abstraction for Modals called Alerts (http://selenium.googlecode.com/git/py/selenium/webdriver/common/alert.py). 

If we can move all the heavy lifting to the server/listener( Yes I appreciate that means a lot of work for us) then we easily keep interop.

> 
> marionette.switch_to_frame() # top-level frame
> marionette.switch_to_frame(system_app_frame)
> marionette.handle_modal_dialog("""
>    // javascript which will interact with the dialog when it appears
> """)
> marionette.switch_to_frame()
> marionette.switch_to_frame(some_app_frame)
> element = marionette.findXXX()
> element.tap() # triggers modal dialog
> 

I think if we do the handle_modal_dialog JS on listener(not sure if technically feasible) I think that would be a good option


Hope that makes sense
(In reply to David Burns :automatedtester from comment #19)
> (In reply to Jonathan Griffin (:jgriffin) from comment #18)
> > mdas, AutomatedTester and I had an IRC chat about these today.
> > 
> > The proposal:  require tests to explicitly switch frames to handle the modal
> > dialog (which is implemented in the system app).  A test would look
> > something like this:
> > 
> > element.tap() # this triggers modal dialog
> > marionette.switch_to_frame() # switch to top-level frame
> > marionette.switch_to_frame(system_app_frame)
> > # add code here to deal with modal dialog
> > marionette.switch_to_frame()
> > marionette.switch_to_frame(previous_app_frame)
> > 
> > To implement this solution, we need the following:
> > 
> > 1) We need to detect when the current frame freezes so that if a marionette
> > command is in-progress (as with tap) when the frame freezes, we send an "ok"
> > response from marionette-server, rather than waiting for the response to
> > come from marionette-listener, which won't happen because the listener is
> > running in the frozen frame.
> > 
> > 2) We make it so that marionette.switch_to_frame() (top-level) can work even
> > when the current frame is frozen.
> 
> 
> Either of these sound good to me.
> 
> 
> > 
> > 2) is "easy", 1) is a little harder.  We'd probably have to load a separate
> > frame script into every frame that watches for mozbrowsershowmodalprompt and
> > notifies marionette-server when that happens.

We can use addEventListener instead of loading a new frame script. We can do the frame script loading but I don't think it buys us much

> > 
> > We could also opt for a more Mozmill-like approach, which would be much
> > cleaner to implement, but a little less webdriver-like to work with.  In
> > this approach, we'd define a JS callback to be executed when the alert
> > appeared:
> 
> If we do this I would want this to happen entirely remote. The WebDriver
> approach is that we take on the hard work so that the people using it dont
> have to. Maybe it's just me but asking users to do
> 
> marionette.handle_modal_dialog("""
>    // javascript which will interact with the dialog when it appears
> """)
> 
> is a recipe for disaster, unless of course the user never needs to call
> handle_modal_dialog and is hidden in an alert/modal dialog abstraction. In
> WebDriver we have a nice abstraction for Modals called Alerts
> (http://selenium.googlecode.com/git/py/selenium/webdriver/common/alert.py). 
> 
> If we can move all the heavy lifting to the server/listener( Yes I
> appreciate that means a lot of work for us) then we easily keep interop.
> 
> > 
> > marionette.switch_to_frame() # top-level frame
> > marionette.switch_to_frame(system_app_frame)
> > marionette.handle_modal_dialog("""
> >    // javascript which will interact with the dialog when it appears
> > """)
> > marionette.switch_to_frame()
> > marionette.switch_to_frame(some_app_frame)
> > element = marionette.findXXX()
> > element.tap() # triggers modal dialog
> > 
> 
> I think if we do the handle_modal_dialog JS on listener(not sure if
> technically feasible) I think that would be a good option
> 
> 
> Hope that makes sense

I agree with you that we shouldn't have to make the user switch to frames and add JS based modal dialog handling. 

I think we can abstract away the all the 'switch to frame & add modal dialog handling' work needed into a module similar to alerts.py too. This is a little nicer because then we can have alerts.py (or similar) abstract away the differences in modal handling between b2g and desktop, so the user of alerts.py doesn't really need to know what needs to be done in order to handle a modal. But the question now is what should we provide to the user? 

The current alerts.py abilities are very limited, especially when considering that in b2g, we may want to do things other than just get the text of the modal,but of specific elements and say, execute an action against them as well.

From this, I suggest that we just have a 'switch_to_modal' call:


element.tap() # generates alert
m.switch_to_modal() # handles the switching into the modal dialog
# add code against the modal()
m.switch_to_frame(app_frame)

Alternatively, to avoid the switching to frame work completely, we might have something like alerts.py, but it can just be a wrapper around the marionette instance alongside normal alerts.py functions. It allow us to do both of the following:

element.tap() # generates alert
alert = m.get_alert() # this switches into the frame of the alert
element = alert.find_element("id", "modal-dialog-confirm-ok")
element.tap() # this would dismiss the alert
alert.finish() # this will let us go back to the original frame

And we can do this as well:
element.tap() # generates alert
alert = m.get_alert() # this switches into the frame of the alert
alert.accept() # This will find the modal-dialog-confirm-ok or similar element and tap it, then switch out of the frame

Thoughts?
(In reply to Malini Das [:mdas] from comment #20)
> From this, I suggest that we just have a 'switch_to_modal' call:
> 
> 
> element.tap() # generates alert
> m.switch_to_modal() # handles the switching into the modal dialog
> # add code against the modal()
> m.switch_to_frame(app_frame)

Actually, I don't like this suggestion anymore since it still involves the switching code, and alerts.py idea is much nicer. I strongly prefer the suggestion following it.
> And we can do this as well:
> element.tap() # generates alert
> alert = m.get_alert() # this switches into the frame of the alert
> alert.accept() # This will find the modal-dialog-confirm-ok or similar
> element and tap it, then switch out of the frame
> 
> Thoughts?

Will pre-defined simple actions like 'accept' be sufficient, or will we need full access to the Marionette API?

Either way, this seems like the simplest API for test developers.

We won't be able to implement using the JS approach, as that requires that a specific handler be added before the alert appears.

In addition to requiring 1) and 2) above, this probably will also depend on bug 855327.
(In reply to Jonathan Griffin (:jgriffin) from comment #22)
> > And we can do this as well:
> > element.tap() # generates alert
> > alert = m.get_alert() # this switches into the frame of the alert
> > alert.accept() # This will find the modal-dialog-confirm-ok or similar
> > element and tap it, then switch out of the frame
> > 
> > Thoughts?
> 
> Will pre-defined simple actions like 'accept' be sufficient, or will we need
> full access to the Marionette API?

If we want to allow people to write once and run anywhere (like Java but way better!) then it should be. This is why the webdriver project keeps the client code as thin as possible.
also instead of 

> alert = m.get_alert()

I would prefer 

> alert = m.switch_to_alert()

The switch_to_ metaphor implies that we are doing a specific action and maps nicely to webdriver for future contributions
(In reply to Jonathan Griffin (:jgriffin) from comment #22)
> > And we can do this as well:
> > element.tap() # generates alert
> > alert = m.get_alert() # this switches into the frame of the alert
> > alert.accept() # This will find the modal-dialog-confirm-ok or similar
> > element and tap it, then switch out of the frame
> > 
> > Thoughts?
> 
> Will pre-defined simple actions like 'accept' be sufficient, or will we need
> full access to the Marionette API?
> 
> Either way, this seems like the simplest API for test developers.
> 
> We won't be able to implement using the JS approach, as that requires that a
> specific handler be added before the alert appears.
> 
> In addition to requiring 1) and 2) above, this probably will also depend on
> bug 855327.

I spoke with wlach about this and we'll integrate the code from bug 855327 into the frame manager code for this bug since bug 855327 is most likely to land first
Attached patch first pass (obsolete) — — Splinter Review 
This is a first pass of handling modal dialog interrupts in b2g. When the system frame catches the modal dialog event, it calls 'handleModal' which forces a switch to the system frame (the frame that triggered the dialog will be halted somewhere in generateEvents function). Once the dialog is dismissed, then the frame that triggered the dialog will resume. I added a check at the end of generateEvents, and if the frame was interrupted, then it will force a switch back into this frame.

I'm not sure if it is possible for a call to generateEvents to complete before triggering the modal. While I don't think there's anything stopping this from happening, I have never seen this, since event dispatching is quick. We could add event listeners for each event we dispatch, forcing us to ensure that we have successfully sent our event listeners before completing generateEvents, but that seems a bit overkill for an unlikely scenario.

This patch works against a phone. I'll be testing it in desktop firefox and b2g shortly.
Attachment #781838 - Flags: feedback?(jgriffin)
This is causing some problems in desktop b2g, working to resolve them now.
Comment on attachment 781838 [details] [diff] [review]
first pass

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

Cool, I think this is going in the right direction.  I made some drive-by comments, but will wait for a final patch before doing a more careful review.

I also think this patch is not likely to work in non-remote cases, but I think that will come out in testing.  Let me know if you need help tracking anything down.

::: testing/marionette/marionette-listener.js
@@ +77,5 @@
> +    }
> +  }
> +  else {
> +    sendSyncMessage("Marionette:handleModal", {win: winUtil.outerWindowID, frame: null});
> +  }

Neither of the 'handleModal' implementations use any message parameters.

@@ +181,4 @@
>   */
>  function sleepSession(msg) {
>    deleteSession();
> +  curWindow.removeEventListener("mozbrowsershowmodalprompt", modalHandler, false);

We'll want to remove the event listener during deleteSession as well, I think.

::: testing/marionette/marionette-server.js
@@ +253,5 @@
>      messageManager.addMessageListener("Marionette:register", this);
> +    messageManager.addMessageListener("Marionette:switchToModalOrigin", this);
> +    messageManager.addMessageListener("Marionette:switchToFrame", this);
> +    messageManager.addMessageListener("Marionette:handleModal", this);
> +    messageManager.addMessageListener("Marionette:checkIfInterrupted", frameManager);

This essentially duplicates similar code in the new frame-manager.js; can we consolidate, and have only one implementation of add/removeMessageManagerListeners?

@@ +2101,5 @@
>        case "Marionette:switchToFrame":
> +        frameManager.switchToFrame(message);
> +        this.messageManager = frameManager.currentRemoteFrame.messageManager;
> +        break;
> +      case "Marionette:handleModal":

We seem to have competing implementations of 'handleModal' here and in frame-manager.js.  Can we have just one, instead?
Attachment #781838 - Flags: feedback?(jgriffin) → feedback+
(In reply to Jonathan Griffin (:jgriffin) from comment #28)
> Comment on attachment 781838 [details] [diff] [review]
> first pass
> 
> Review of attachment 781838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, I think this is going in the right direction.  I made some drive-by
> comments, but will wait for a final patch before doing a more careful review.
> 
> I also think this patch is not likely to work in non-remote cases, but I
> think that will come out in testing.  Let me know if you need help tracking
> anything down.

You're right, and it's because we don't check if we're in OOP or not, and we don't deal with modals differently if we do. Seeing this also brought to light that this patch should probably use/modify the BrowserObjs we keep in the server (curBrowser), since that's the object that currently keeps track of the frames, and is more closely linked to non-oop frame-switching. I'll have to update my patch so that we only deal with frames in one, consistent manner, instead of having various structures strewn about.

> 
> ::: testing/marionette/marionette-listener.js
> @@ +77,5 @@
> > +    }
> > +  }
> > +  else {
> > +    sendSyncMessage("Marionette:handleModal", {win: winUtil.outerWindowID, frame: null});
> > +  }
> 
> Neither of the 'handleModal' implementations use any message parameters.

Thanks, that's a remnant from an older attempt.
> 
> @@ +181,4 @@
> >   */
> >  function sleepSession(msg) {
> >    deleteSession();
> > +  curWindow.removeEventListener("mozbrowsershowmodalprompt", modalHandler, false);
> 
> We'll want to remove the event listener during deleteSession as well, I
> think.
> 
> ::: testing/marionette/marionette-server.js
> @@ +253,5 @@
> >      messageManager.addMessageListener("Marionette:register", this);
> > +    messageManager.addMessageListener("Marionette:switchToModalOrigin", this);
> > +    messageManager.addMessageListener("Marionette:switchToFrame", this);
> > +    messageManager.addMessageListener("Marionette:handleModal", this);
> > +    messageManager.addMessageListener("Marionette:checkIfInterrupted", frameManager);
> 
> This essentially duplicates similar code in the new frame-manager.js; can we
> consolidate, and have only one implementation of
> add/removeMessageManagerListeners?

Yes, thanks for pointing that out.
> 
> @@ +2101,5 @@
> >        case "Marionette:switchToFrame":
> > +        frameManager.switchToFrame(message);
> > +        this.messageManager = frameManager.currentRemoteFrame.messageManager;
> > +        break;
> > +      case "Marionette:handleModal":
> 
> We seem to have competing implementations of 'handleModal' here and in
> frame-manager.js.  Can we have just one, instead?

Yeah, I set up the "Marionette:handleModal" message listener in marionette-server.js so the server can update its message manager and would send back the Ok message, but I can handle that in the frame manager.
Forgot to mention that I'll take Bug 855327 into account for the next patch too.
This patch "works" for single and out of process environments... to a point. The previous patch and this one both suffer from hard crashes right after closing a modal dialog window in the Settings app. I had been testing against contacts and gallery, which work fine with both patches, but the Settings app just dies with very little obvious log output except a typical memory freeing message, then the crash. I'll have to look into this
Attachment #781838 - Attachment is obsolete: true
Attachment #788217 - Flags: feedback?(jgriffin)
The crash was due to PEBKAC, I was tapping the wrong button, telling the phone to reset! I'll fix up this patch and r?
Did you guys patch something here since the 9th?

One of our tests has regressed but trying to work out if it is the Gaia Call screen or Marionette which has moved.
(In reply to Zac C (:zac) from comment #34)
> Did you guys patch something here since the 9th?
> 
> One of our tests has regressed but trying to work out if it is the Gaia Call
> screen or Marionette which has moved.

nothing landed in testing/marionette since August 8th, which were just merges (https://hg.mozilla.org/mozilla-central/rev/f057fca09627). You can check by doing 'hg log -l1 testing/marionette/' from the tip of your m-c to see the last commit under testing/marionette
Whiteboard: [u=marionette-user c=frames p=1]
OK np Malini. We worked around it with a short sleep anyway.
Whiteboard: [u=marionette-user c=frames p=1] → [u=marionette-user c=frames p=5]
Assignee: nobody → mdas
As an update, I'm trying to work in the currentFrameElement changes, and I realized when testing manually that there's a bug in the frame-manager code when revisiting an OOP frame. Fixing this bug, and will add a 'revisit OOP frame' test.
FYI, I'm running a fixed patch through https://tbpl.mozilla.org/?tree=Try&rev=4e9154fe6ae8

There's apparently some other patch on my patch queue (though I don't see it in my .hg/patches at all! It isn't applied and there is no patch remotely like that), but that one shouldn't affect this run. If you know how that extra, magic patch got submitted, I'll give you one free internet.
Comment on attachment 788217 [details] [diff] [review]
"Working" for single process and OOP

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

Great, this looks almost ready.  There are lots of MDAS logging statements that either need to be generalized a bit or removed.  This is a mozilla-b2g18 version of the patch; we'll need an m-c version to land.

::: testing/marionette/marionette-frame-manager.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +this.EXPORTED_SYMBOLS = [

nit:  should have a newline before this line

@@ +37,5 @@
> +  this.previousRemoteFrame = null; //frame we'll need to restore once interrupt is gone
> +  this.handledModal = false; //set to true when we have been interrupted by a modal
> +  this.remoteFrames = []; //list of OOP frames that has the frame script loaded
> +  this.server = server; // a reference to the marionette server
> +  this.messageManager.addMessageListener("MarionetteFrame:checkIfInterrupted", this);

Do we need to register this message handler both here and in addMessageManagerListeners below?

@@ +46,5 @@
> +   * Receives all messages from content messageManager
> +   */
> +  receiveMessage: function FM_receiveMessage(message) {
> +    switch (message.name) {
> +      case "MarionetteFrame:checkIfInterrupted":

I think "checkIfInterrupted" and "clearInterrupted" might be more clearly named "getInterruptedState" and "clearInterruptedState"; what do you think?

@@ +52,5 @@
> +        if (this.previousRemoteFrame) {
> +          /*can't use Services.wm.getOuterWindowWithId in b2g18
> +          let thisFrame = Services.wm.getOuterWindowWithId(this.previousRemoteFrame.windowId);//get the frame window of the interrupted frame
> +          */
> +          let getSomeWindow = this.server.getCurrentWindow();

This seems like an awkward variable name; maybe currentWindow?

@@ +55,5 @@
> +          */
> +          let getSomeWindow = this.server.getCurrentWindow();
> +          let thisFrame = getSomeWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIDOMWindowUtils)
> +                          .getOuterWindowWithId(this.previousRemoteFrame.windowId);

'thisFrame' sounds like this frame is active right now; is it?  Perhaps 'aFrame' would be a better name?

@@ +85,5 @@
> +          this.removeMessageManagerListeners(this.currentRemoteFrame.messageManager);
> +          //store the previous frame so we can switch back to it when the modal is dismissed
> +          this.previousRemoteFrame = this.currentRemoteFrame;
> +          //by setting currentRemoteFrame to null, it signifies we're in the main process
> +          this.currentRemoteFrame = null; 

nit: extra white space at end of line

@@ +132,5 @@
> +    let aFrame = new MarionetteRemoteFrame(message.json.win, message.json.frame);
> +    aFrame.messageManager = mm;
> +    this.remoteFrames.push(aFrame);
> +    this.currentRemoteFrame = aFrame;
> +    return mm;

We don't need to return mm here, nothing uses this value (I think).

@@ +170,5 @@
> +    messageManager.addMessageListener("Marionette:switchToFrame", this.server);
> +    messageManager.addMessageListener("Marionette:switchedToFrame", this.server);
> +    messageManager.addMessageListener("MarionetteFrame:handleModal", this);
> +    messageManager.addMessageListener("MarionetteFrame:checkIfInterrupted", this);
> +    messageManager.addMessageListener("MarionetteFrame:clearInterrupted", this);

Is not clearing checkIfInterrupted and clearInterrupted in removeMessageManagerListeners deliberate?

::: testing/marionette/marionette-listener.js
@@ +67,4 @@
>  let isTap = false;
>  // whether to send mouse event
>  let mouseEventsOnly = false;
> +let modalHandler = function() { 

nit: extra whitespace at end of line

@@ +226,4 @@
>    removeMessageListenerId("Marionette:getAllCookies", getAllCookies);
>    removeMessageListenerId("Marionette:deleteAllCookies", deleteAllCookies);
>    removeMessageListenerId("Marionette:deleteCookie", deleteCookie);
> +  curFrame.removeEventListener("mozbrowsershowmodalprompt", modalHandler, false);

I think it would be safer to use 'content' here instead of curFrame (and same in the calls to addEventListener); I don't think there will ever be a case where we actually want to use any other value of curFrame here.

@@ +294,5 @@
> +  if (previousFrame) {
> +    dump("MDAS: calling wasInterrupted in : " + content.document.activeElement.ownerDocument.location.href + "\n");
> +    let element = content.document.elementFromPoint((content.innerWidth/2), (content.innerHeight/2));
> +    dump("MDAS: id of topmost element: " + element.id + "\n");
> +    if (element.id.indexOf("modal-dialog") == -1) {

Is this a robust enough method of determining if a modal dialog is active?  Isn't it possible that an element within the modal dialog would be returned here instead of the dialog itself?
Attachment #788217 - Flags: feedback?(jgriffin) → feedback-
Comment on attachment 788217 [details] [diff] [review]
"Working" for single process and OOP

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

er, I just realized this was f? and not r?, so f+!
Attachment #788217 - Flags: feedback- → feedback+
(In reply to Jonathan Griffin (:jgriffin) from comment #39)

Thanks I'm updating my patch and testing it. It doesn't work well in b2g18 anymore, so I have to figure out what's wrong.

> Comment on attachment 788217 [details] [diff] [review]
> "Working" for single process and OOP
> 
> Review of attachment 788217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +170,5 @@
> > +    messageManager.addMessageListener("Marionette:switchToFrame", this.server);
> > +    messageManager.addMessageListener("Marionette:switchedToFrame", this.server);
> > +    messageManager.addMessageListener("MarionetteFrame:handleModal", this);
> > +    messageManager.addMessageListener("MarionetteFrame:checkIfInterrupted", this);
> > +    messageManager.addMessageListener("MarionetteFrame:clearInterrupted", this);
> 
> Is not clearing checkIfInterrupted and clearInterrupted in
> removeMessageManagerListeners deliberate?

Yes, it is so that frames other than the one that frame-manager thinks is the current frame (ie: the interrupted frame) can check the status of the frame-manager's interrupted state. checkIfInterrupted will be called by a non-current frame when the modal dialog is dismissed, and the interrupted frame's process is resumed. The interrupted frame will check if it has been interrupted, and then will ask the frame-manager to set itself as the current frame (using switchToModalOrigin).

clearInterrupted should be removed though.

> @@ +294,5 @@
> > +  if (previousFrame) {
> > +    dump("MDAS: calling wasInterrupted in : " + content.document.activeElement.ownerDocument.location.href + "\n");
> > +    let element = content.document.elementFromPoint((content.innerWidth/2), (content.innerHeight/2));
> > +    dump("MDAS: id of topmost element: " + element.id + "\n");
> > +    if (element.id.indexOf("modal-dialog") == -1) {
> 
> Is this a robust enough method of determining if a modal dialog is active? 
> Isn't it possible that an element within the modal dialog would be returned
> here instead of the dialog itself?

It is possible, but not the way any of the modal dialog templates are setup. You have some text in the center of the screen, but the modal-dialog-* element will be returned. 

This isn't nearly as robust as I'd like it to be, but no other solution was working. Checking to see which window had focus didn't help, since the interrupted frame will still have focus if a modal is on top of it. getActiveElement wouldn't retrieve a useful element in all cases either. 

The only other solution I could think of was to try and get the correct modal-dialog-* element when the "mozbrowsershowmodalprompt" event is fired and attach a MutationObserver to it, but I couldn't differentiate between modals (the visible modals will have a new class added in the classList according to the code, but I wasn't able to see this in any of the modal-dialog-* objects I retrieved), and even if I could retrieve a visible modal-dialog element, we don't know if it's the dialog for the current interrupted window, or some other modal dialog window for some other application, since multiple modal dialogs can be active at the same time.

I've likely explored other options, but now I can't remember them...I will gladly accept any other suggestions to try, but this ugly hack is the only hack that has been successful thus far :(
Attached patch modal (obsolete) — — Splinter Review 
I've updated the patch according to feedback and added a test for switching back into a previously visited OOP frame. I tested this patch in m-c FF desktop, m-c desktop B2G and on the device (using mozilla-b2g18 and m-c), and it passes all tests. I've manually tested the modal dialog switching handling in all these environments as well (I don't think we should write a unit test in gaia-ui-tests for marionette unit testing since it's out of place).

Note: marionette-frame-manager.js's Services.wm.getOuterWindowWithId calls will have to be changed to use nsIDOMWindowUtils's methods when merging into mozilla-b2g18
Attachment #788217 - Attachment is obsolete: true
Attachment #790812 - Flags: review?(jgriffin)
Comment on attachment 790812 [details] [diff] [review]
modal

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

Cool, looks great!  This is a complicated bit code; in order to prevent breaking it, is there any way to add a test for it?  I wouldn't block landing this for a test, but it would be great to add one post-facto if possible; I imagine one for the local case is easier than for the OOP case, but both would be valuable.

::: testing/marionette/marionette-frame-manager.js
@@ +66,5 @@
> +         * handleModal is called when we need to switch frames to the main process due to a modal dialog interrupt.
> +         */
> +        // If previousRemoteFrame was set, that means we switched into a remote frame.
> +        // If this is the case, then we want to switch back into the system frame.
> +        // If it isn't the case, then we're in a non-OOP environment, so we don't need to handle remote frames

Could probably combine all of the above comments into one block.

@@ +85,5 @@
> +    }
> +  },
> +
> +  //This is just 'switch to OOP frame'. We're handling this here so we can maintain a list of remoteFrames. 
> +  switchToFrame: function FM_switchToFrame(message) {

To avoid confusion with the other switchToFrame, we should probably name this switchToRemoteFrame, or switchToOOPFrame.

::: testing/marionette/marionette-listener.js
@@ +71,4 @@
>  Cu.import("resource://gre/modules/services-common/log4moz.js");
>  let logger = Log4Moz.repository.getLogger("Marionette");
>  logger.info("loaded marionette-listener.js");
> +let modalHandler = function() {

Add a comment here that this handles the 'mozbrowsershowmodalprompt' event, and what this is doing.

::: testing/marionette/marionette-server.js
@@ +2102,5 @@
>        case "Marionette:register":
>          // This code processes the content listener's registration information
>          // and either accepts the listener, or ignores it
>          let nullPrevious = (this.curBrowser.curFrameId == null);
> +        let listenerWindow = 

nit: extra space at end of line
Attachment #790812 - Flags: review?(jgriffin) → review+
(In reply to Jonathan Griffin (:jgriffin) from comment #43)
> Comment on attachment 790812 [details] [diff] [review]
> modal
> 
> Review of attachment 790812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, looks great!  This is a complicated bit code; in order to prevent
> breaking it, is there any way to add a test for it?  I wouldn't block
> landing this for a test, but it would be great to add one post-facto if
> possible; I imagine one for the local case is easier than for the OOP case,
> but both would be valuable.

Discussed on irc: we can add one to gaia-ui-tests and it will be run in tbpl against desktop b2g, so OOP case will not be tested. FYI, it needs to be a gaia-ui-test since the modal dialog implementation is gaia specific.

The bug number for this test is Bug 906175.

I will push a fixed and tested patch to m-i soon.
https://hg.mozilla.org/mozilla-central/rev/705d668fb81b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Summary: Implement Modal dialog handling to Marionette → Implement B2G Modal dialog handling to Marionette
Attached patch modal patch for mozilla-b2g18 (obsolete) — — Splinter Review 
This is a slight modification to the original patch, so that it can land cleanly in mozilla-b2g18. I have tested this locally against our b2g18 tests and it passes.
Attachment #792960 - Flags: review+
please commit the "modal patch for mozilla-b2g18" patch to mozilla-b2g18. Thanks!
Whiteboard: [u=marionette-user c=frames p=5] → [u=marionette-user c=frames p=5][checkin-needed-b2g18]
https://hg.mozilla.org/releases/mozilla-b2g18/rev/43d3b34de0f2
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox24: --- → wontfix
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
Whiteboard: [u=marionette-user c=frames p=5][checkin-needed-b2g18] → [u=marionette-user c=frames p=5]
Backed out of mozilla-b2g18 for apparently causing a perf regression at https://datazilla.mozilla.org/b2g/?branch=v1-train&device=inari&range=30&test=cold_load_time&app_list=calendar,camera,email%20FTU,gallery,music&app=calendar&gaia_rev=913bcdc9d8595161&gecko_rev=a0f186ae3cc41c1e.

A local build reproduced the regression; backing out this commit resolved it.  We'll see if datazilla agrees on Monday.
(In reply to Jonathan Griffin (:jgriffin) from comment #52)
> backout changeset:
> http://hg.mozilla.org/releases/mozilla-b2g18/rev/c6f7c8ffc535

This did resolve the perf regression.  Also backing out from 101hd:

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/4a049b23167e
status-b2g18: fixed → ---
status-b2g-v1.1hd: fixed → ---
I think we're leaking frame scripts.  Prior to this patch, remoteFrames[] was a global variable for marionette-server, but we now create a new remoteFrames[] list for each frameManager instance, which happens at least once per session.  This probably causes us to load multiple copies of the frame script into each frame over time.

Backing out from m-c as well:  https://hg.mozilla.org/mozilla-central/rev/60c382ba1773
Status: RESOLVED → REOPENED
status-firefox26: fixed → ---
Resolution: FIXED → ---
We're also leaking imported scripts; see bug 909129.  But, I don't think these two things are related.
(In reply to Jonathan Griffin (:jgriffin) from comment #53)
> This did resolve the perf regression.  Also backing out from 101hd:
> 
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/4a049b23167e

Non-hd+ landings on v1.1hd are typically handled via b2g18 branch merges. To avoid potential merge conflicts, please don't land directly there unless the patch isn't landing on b2g18 otherwise.
status-b2g18: --- → affected
status-b2g-v1.1hd: --- → affected
status-firefox26: --- → affected
Target Milestone: mozilla26 → ---
This patch applies on top of yours, and is enough to fix the perf regressions that this caused.  It's still potentially leaking frame scripts, but we can handle that in a separate patch.
Attachment #796310 - Flags: review?(mdas)
Assignee: mdas → jgriffin
Comment on attachment 796310 [details] [diff] [review]
Make sure cleanup happens properly,

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

haha, I have a similar patch lying around as an improvement, but didn't realize this was the cause of the performance failure! Thanks for this!
Attachment #796310 - Flags: review?(mdas) → review+
This patch changes how we store messageManagers so that we use weakReferences, and ensures that we don't inject our frame script multiple times. It ensures this by persisting the remoteFrames array across sessions.

The problem we're currently having is that this approach doesn't seem to detect when an remote frame closes. Theoretically, when we retrieve the real messageManager object from the weakref, it should be null if the appframe closed, but right now, it's returning a seemingly valid object. This makes it seem like we're caching messageManager objects somewhere, but I'm not yet sure where.
This is a kludgy approach, but it works...we try to send a dummy message to the frame, and if we get NS_ERROR_NOT_INITIALIZED, we delete the reference.  I'm not sure if there's a better way.
Attachment #796845 - Attachment is obsolete: true
The backed out version of this patch (with or without the 2 follow-ups) is causing some out-of-sync problems on master when running b2gperf with the "FM Radio" app:

Traceback (most recent call last):                                                      
  File "/home/jgriffin/b2gperf/venv/local/lib/python2.7/site-packages/b2gperf/b2gperf.py", line 198, in test_startup
    result = self.marionette.execute_async_script('launch_app("%s")' % app_name)
  File "/home/jgriffin/b2gperf/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 624, in execute_async_script
    scriptTimeout=script_timeout)
  File "/home/jgriffin/b2gperf/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 350, in _send_message
    self._handle_error(response)
  File "/home/jgriffin/b2gperf/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 412, in _handle_error
    raise MarionetteException(message=response, status=500)
MarionetteException: {u'ok': True, u'from': u'0'}
Actually this happens on the first invocation of the *next* app, Usage.
Also in the logcat:

I/Gecko   ( 1021): 1377817112886	Marionette	TRACE	Got: {"to": "0", "session": "6-b2g", "type": "switchToFrame", "focus": true, "value": null}
I/Gecko   ( 1021): 1377817112895	Marionette	DEBUG	Got request: switchToFrame, data: {"to":"0","session":"6-b2g","type":"switchToFrame","focus":true,"value":null}, id: {39d64c81-42af-4630-9a17-7f733efdf0d4}
I/Gecko   ( 1021): 1377817112902	Marionette	INFO	Switched to frame: {"frameValue":null}
I/Gecko   ( 1021): 1377817113099	Marionette	INFO	sendToClient: {"from":"0","value":{"frame":{"ELEMENT":"{4b25b3be-a058-417f-aad1-9550aaedf892}"},"src":"app://costcontrol.gaiamobile.org/index.html","name":"Usage","origin":"app://costcontrol.gaiamobile.org","cold_load_time":1546}}, {a11da367-172d-41e2-8a0f-29f6aca2c8e1}, {39d64c81-42af-4630-9a17-7f733efdf0d4}
I/Gecko   ( 1021): 1377817113102	Marionette	WARN	ignoring out-of-sync response
I/Gecko   ( 1021): 1377817113153	Marionette	INFO	sendToClient: {"from":"0","ok":true}, {39d64c81-42af-4630-9a17-7f733efdf0d4}, {39d64c81-42af-4630-9a17-7f733efdf0d4}
handing this back to mdas
Assignee: jgriffin → mdas
I tested these patches against master and mozilla-b2g18, on inari and unagi, and I can no longer reproduce this error, even after running additional tests. I will reland and watch b2gperf to see if this is still happening.

This new patch updates and rolls up the 3 former patches into one. Rolling former r+s forward
Attachment #790812 - Attachment is obsolete: true
Attachment #796310 - Attachment is obsolete: true
Attachment #797008 - Attachment is obsolete: true
Attachment #807265 - Flags: review+
rolling up all changes into one patch for mozilla-b2g18 and rolling forward r+
Attachment #792960 - Attachment is obsolete: true
Attachment #807298 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/aa93203babed
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 923674
Reopening this because it is reproducing again

Gecko  http://hg.mozilla.org/releases/mozilla-aurora/rev/8b4152d366c3
Gaia  0579e4ec4903344d1e92f6a02037ad133c6d974d
BuildID 20131006004004
Version 26.0a2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
with this patch this test:  https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/gallery/test_gallery_delete_image.py
should be passing. 
can you investigate this marionette change using our test as a use case? 
Thanks!
What problem are you seeing exactly?

I have some success at running it with today's build, but it takes a few tries for it to fail. When it does, it kind of hangs waiting to press the Delete button. I think the tap is being dispatched before the modal dialog is fully loaded, but I'll have to keep investigating
Malini, we get a socket.timeout.

http://qa-selenium.mv.mozilla.com:8080/job/b2g.hamachi.mozilla-central.ui.xfail/43/testReport/junit/%28root%29/TestGalleryDelete/test_gallery_delete_image/

The stack trace did not change after this patch.

I've been running it locally and it's a 100% replication on today's m-c Hamachi nightly.
Blocks: 923465
Whiteboard: [u=marionette-user c=frames p=5] → [u=marionette-user c=frames p=5][xfail]
Malini were you running this on device or desktopb2g?
Blocks: 926881
(In reply to Zac C (:zac) from comment #73)
> Malini were you running this on device or desktopb2g?

I can't remember now, but definitely on device. I can try with desktop b2g, shortly after I land Bug 909129
No it's ok I was hoping you'd say device.

Our test is definitely failing. I can't see how it's working for you. Can you send me your test case?
(In reply to Zac C (:zac) from comment #75)
> No it's ok I was hoping you'd say device.
> 
> Our test is definitely failing. I can't see how it's working for you. Can
> you send me your test case?

I was running https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/gallery/test_gallery_delete_image.py where it would intermittently fail.
I'm re-marking this as resolved/fixed since the original bug was to add modal dialog handling which has landed.

The modal dialog bug brought up in Comment #69 will be handled in Bug 927592
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/7ca5d1e81d37 to find out whether it was you or bug 807282 which turned b2g mochitest-7 permaorange without it bothering to say why it was orange, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=29217047&tree=Mozilla-Aurora. Come on back in if it turns out not to be you.
status-firefox26: fixed → affected
No longer blocks: 923674
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: