775116 - Switching frames by id or name fails in content

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Switching to frames by id or name, at least in the B2G environment. I get:

E/GeckoConsole(  116): [JavaScript Error: "frameElement is null" {file: "chrome://marionette/content/marionette-listener.js" line: 708}]

In the logcat. It seems like there is no frameElement attached to the frame, so looking at the name this way won't work.

to reproduce in marionette session:
m.switch_to_frame(0) # this works
m.switch_to_frame() #go back to parent
m.switch_to_frame('keyboard-frame') # this fails and throws error.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

After running MC/testing/marionette/client/marionette/tests/unit/test_switch_frame.py on a nightly debug build, I can confirm that this is a B2G specific bug.

Comment 2

[Alfredos-Panagiotis Damkalis [:fredy]]

not sure if this comment fits here...

I am also able to reproduce the steps above.

However running the same above steps but in chrome context it switches to the keyboard-frame.

STR:
m.client.set_context("chrome")
m.client.switch_to_frame(0) # goes to gaia system frame
m.client.switch_to_frame("keyboard-frame") # works fine

But if i try to select any other frame by id or name then fails in python environment with 

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "marionette/marionette.py", line 278, in switch_to_frame
    response = self._send_message('switchToFrame', 'ok', value=frame)
  File "marionette/marionette.py", line 153, in _send_message
    self._handle_error(response)
  File "marionette/marionette.py", line 211, in _handle_error
    raise MarionetteException(message=response, status=500)
marionette.errors.MarionetteException: {u'message': u"error occurred while processing 'switchToFrame' request: TypeError: frameElement is null", u'from': u'conn0.marionette1', u'error': u'unknownError'}


and at logcat shows:

E/GeckoConsole(  208): [JavaScript Error: "frameElement is null" {file: "chrome://marionette/content/marionette-actors.js" line: 924}]

STR:
m.client.set_context("chrome")
m.client.switch_to_frame(0) # goes to gaia system frame
m.client.switch_to_frame("appframe0") # gives the above errors

or

m.client.set_context("chrome")
m.client.switch_to_frame(0) # goes to gaia system frame
m.client.switch_to_frame("background") # gives the above errors

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Alfredos-Panagiotis Damkalis from comment #2)
> not sure if this comment fits here...
> 
> I am also able to reproduce the steps above.
> 
> However running the same above steps but in chrome context it switches to
> the keyboard-frame.
> 
> STR:
> m.client.set_context("chrome")
> m.client.switch_to_frame(0) # goes to gaia system frame
> m.client.switch_to_frame("keyboard-frame") # works fine
> 
> But if i try to select any other frame by id or name then fails in python
> environment with 
> 
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File "marionette/marionette.py", line 278, in switch_to_frame
>     response = self._send_message('switchToFrame', 'ok', value=frame)
>   File "marionette/marionette.py", line 153, in _send_message
>     self._handle_error(response)
>   File "marionette/marionette.py", line 211, in _handle_error
>     raise MarionetteException(message=response, status=500)
> marionette.errors.MarionetteException: {u'message': u"error occurred while
> processing 'switchToFrame' request: TypeError: frameElement is null",
> u'from': u'conn0.marionette1', u'error': u'unknownError'}
> 
> 
> and at logcat shows:
> 
> E/GeckoConsole(  208): [JavaScript Error: "frameElement is null" {file:
> "chrome://marionette/content/marionette-actors.js" line: 924}]
> 
> STR:
> m.client.set_context("chrome")
> m.client.switch_to_frame(0) # goes to gaia system frame
> m.client.switch_to_frame("appframe0") # gives the above errors
> 
> or
> 
> m.client.set_context("chrome")
> m.client.switch_to_frame(0) # goes to gaia system frame
> m.client.switch_to_frame("background") # gives the above errors

Interesting, thanks for the comment!

Comment 4

[Jonathan Griffin (:jgriffin)]

For Gaia, app frames (currently) look like the following (this is for the contacts app):

<iframe mozallowfullscreen="true" class="appWindow active" data-frame-origin="app://communications.gaiamobile.org/contacts/index.html" src="app://communications.gaiamobile.org/contacts/index.html" mozbrowser="true" mozapp="app://communications.gaiamobile.org/manifest.webapp" remote="true" id="appframe1" data-frame-type="window" style="width: 480px; height: 780px;"></iframe>

It would be nice if Marionette allowed us to switch to an iframe based on an arbitrary attribute/value pair, so we could do something like:

m.switch_to_frame({'attribute': 'src', 'value': 'app://communications.gaiamobile.org/contacts/index.html'})

Comment 5

[David Burns :automatedtester]

(In reply to Jonathan Griffin (:jgriffin) from comment #4)
> It would be nice if Marionette allowed us to switch to an iframe based on an
> arbitrary attribute/value pair, so we could do something like:
> 
> m.switch_to_frame({'attribute': 'src', 'value':
> 'app://communications.gaiamobile.org/contacts/index.html'})

I think that if we have the ability to pass in HTMLElement objects that we can have this work in a much nicer approach. This will allow for easier tests.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to David Burns :automatedtester from comment #5)
> (In reply to Jonathan Griffin (:jgriffin) from comment #4)
> > It would be nice if Marionette allowed us to switch to an iframe based on an
> > arbitrary attribute/value pair, so we could do something like:
> > 
> > m.switch_to_frame({'attribute': 'src', 'value':
> > 'app://communications.gaiamobile.org/contacts/index.html'})
> 
> I think that if we have the ability to pass in HTMLElement objects that we
> can have this work in a much nicer approach. This will allow for easier
> tests.

Can you give an example of what you mean? I don't think I quite understand.

Comment 7

[David Burns :automatedtester]

element = m.find_element('xpath', '//*[@src="app://communications.gaiamobile.org/contacts/index.html")
m.switch_to_frame(element)

This approach is what we use in Selenium to make things simpler for developers/testers.

example test is available at https://code.google.com/p/selenium/source/browse/trunk/py/test/selenium/webdriver/common/frame_switching_tests.py#74

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: fix v0.1

The problem was that window.frames would occasionally hold frames without frameElement attributes (since these were top level windows). I realized we don't really need to do it this way, and we can just access the relevant attributes on the return objects of window.frames themselves.

I've updated the test_switch_frame.py tests so they now test against frame switching based on id, name and index. I also added the www/ test pages to the jar, so we can access them via b2g. As such, I modified all the tests to use the chrome:// urls instead of using file paths. So now, we can run the switch_frame tests on b2g. I'd enable all the relevant marionette tests, but I've run into unrelated errors and have created bug 790787 to handle that change.

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

oh, I've tested this in Firefox and B2G and they pass

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 660601 [details] [diff] [review]
fix v0.1

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

Cool, thanks for doing this!  I don't know that we really want to load all the test files as chrome:// url's though.  That gives all the files chrome privileges, and is what is responsible for those failures in test_execute_permission you're seeing in bug 790787.  I don't think we need to do this; the files load fine in an emulator without moving them to chrome:// url's, and they load fine in a device too, as long as the device and the host machine are on the same network, which is the same requirement we have for e.g., mochitests.  I think we should probably leave all the tests to load http://, unless I'm missing something.

I also think adding to this the ability to specify the frame using an HTMLElement (like AutomatedTester suggests in comment #7) makes a lot of sense.  Otherwise, we won't be able to switch to Gaia app frames without devs changing their implementation.

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: fix v0.2

Hah! I made a very human mistake when I switched to chrome:// url. I thought we were still using file:// urls instead of using the test server... anyway, apologies, I reverted those changes.

As for switching via an HTMLElement, we already have support for it, but no tests. I just added a test and it passes on both b2g and firefox.

Comment 12

[Alfredos-Panagiotis Damkalis [:fredy]]

Some comments on the v0.1 - sorry for the tl;dr, maybe some of the comments should become separate bugs.
I have changed only the marionette-listener.js file to build and test.

Running the following command:
for x in range(6):
    m.execute_script("a = window.document.getElementsByTagName(\"iframe\")["+str(x)+"].getAttribute(\"src\");
                      b = window.document.getElementsByTagName(\"iframe\")["+str(x)+"].getAttribute(\"id\");
                      return a + \" --- \" + b;
                    ")

gives:

u'app://costcontrol.gaiamobile.org/widget.html --- null'
u'app://homescreen.gaiamobile.org/index.html --- appframe0'
u'app://keyboard.gaiamobile.org/index.html --- keyboard-frame'
u'app://communications.gaiamobile.org/dialer/background.html --- null'
u'app://sms.gaiamobile.org/background.html#0 --- null'
u'null --- null'

so then I tried to switch to the frames with ids, but the frames, that were selected, were wrong:

>>> m.switch_to_frame("appframe0")
True

>>> m.get_url()
u'app://costcontrol.gaiamobile.org/widget.html'

>>> m.execute_script("return window.document.body.innerHTML")
u'\n  <section id="cost-control">\n    <div id="cost-control-icon"></div>\n    <div>\n      <p><span id="cost-control-currency"></span><span id="cost-control-credit">--</span></p>\n      <p><time id="cost-control-time">Never</time></p>\n    </div>\n  </section>\n\n\n\n'

>>> m.switch_to_frame()
True

>>> m.switch_to_frame("keyboard-frame")
True

>>> m.get_url()
u'app://communications.gaiamobile.org/dialer/background.html'

>>> m.execute_script("return window.document.body.innerHTML")
u'\n\n\n'

================================================================================

Changing into chrome context except the "keyboard-frame" id that works correctly switching to the right frame (checked with get_url()) the rest doesn't work as it is mentioned at my comment above (comment 2). Maybe a similar to marionette-listener.js change should be done at http://dxr.mozilla.org/mozilla-central/testing/marionette/marionette-actors.js.html#l934

================================================================================

Finally there is no way (neither by index nor by id) to switch to apps that are inside an iframe with the attribute "remote=true" which, if I don't make a mistake, means that they run out of process (OOP) as homescreen, calculator etc

Comment 13

[Jonathan Griffin (:jgriffin)]

Thanks for the comments fredy, we're looking into the OOP problem now.

Comment 14

[Jonathan Griffin (:jgriffin)]

Attachment: fix v0.3

This patch allows switch_to_frame to work with OOP frames.  It doesn't break any existing tests on Firefox.  I still need to add tests for this, but I'll add them in a separate patch on this bug in another day or two.

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 663618 [details] [diff] [review]
fix v0.3

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

lgtm, thanks for doing this!

::: testing/marionette/marionette-actors.js
@@ +113,5 @@
> +  this.windowId = windowId;
> +  this.frameId = frameId;
> +  this.targetFrameId = null;
> +  this.messageManager = null;
> +}

Is a semi-colon needed here?

@@ +207,5 @@
> +    messageManager.addMessageListener("Marionette:log", this);
> +    messageManager.addMessageListener("Marionette:shareData", this);
> +    messageManager.addMessageListener("Marionette:register", this);
> +    messageManager.addMessageListener("Marionette:runEmulatorCmd", this);
> +    messageManager.addMessageListener("Marionette:switchToFrame", this);

As long as we don't maintain context-specific state in MarionetteDriverActors and none of these functions modify those members, then the use of 'this' should be fine. We should probably note in the code about that.

::: testing/marionette/marionette-listener.js
@@ +775,5 @@
>    }
>    if (foundFrame == null) {
>      sendError("Unable to locate frame: " + msg.json.value, 8, null);
>      return;
>    }

We should still set the focus to the curWindow (http://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/frame)

@@ +783,5 @@
> +  if (curWindow.contentWindow == null) {
> +    // The frame we want to switch to is a remote frame; notify our parent to handle
> +    // the switch.
> +    curWindow = content;
> +    sendToServer('Marionette:switchToFrame', {win: winUtil.outerWindowID, frame: foundFrame});

Where is sendToServer defined?

Comment 16

[Alfredos-Panagiotis Damkalis [:fredy]]

Thanks for the patch. I applied it at my b2g builds and worked fine with both OOP and non-OOP apps.

Comment 17

[Jonathan Griffin (:jgriffin)]

> ::: testing/marionette/marionette-listener.js
> @@ +775,5 @@
> >    }
> >    if (foundFrame == null) {
> >      sendError("Unable to locate frame: " + msg.json.value, 8, null);
> >      return;
> >    }
> 
> We should still set the focus to the curWindow (http://code.google.com/p/selenium
> /wiki/JsonWireProtocol#/session/:sessionId/frame)

In my interpretation, the Selenium statement "Change focus to another frame on the page" means that the Marionette/Selenium focus will change; i.e., all future commands will be directed at the new frame, rather than requiring that the frame have user focus.

Personally, I find it quite annoying that Firefox keeps bringing itself to the foreground while running Marionette tests.

David, what's your opinion on the meaning of this statement in the JSON Wire Protocol docs?

Comment 18

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #15)
> Comment on attachment 663618 [details] [diff] [review]
> fix v0.3
> 
> Review of attachment 663618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, thanks for doing this!
> 
> ::: testing/marionette/marionette-actors.js
> @@ +113,5 @@
> > +  this.windowId = windowId;
> > +  this.frameId = frameId;
> > +  this.targetFrameId = null;
> > +  this.messageManager = null;
> > +}
> 
> Is a semi-colon needed here?

No, semi-colons are not needed after a function definition, unless it is being assigned to a variable.

> 
> @@ +207,5 @@
> > +    messageManager.addMessageListener("Marionette:log", this);
> > +    messageManager.addMessageListener("Marionette:shareData", this);
> > +    messageManager.addMessageListener("Marionette:register", this);
> > +    messageManager.addMessageListener("Marionette:runEmulatorCmd", this);
> > +    messageManager.addMessageListener("Marionette:switchToFrame", this);
> 
> As long as we don't maintain context-specific state in
> MarionetteDriverActors and none of these functions modify those members,
> then the use of 'this' should be fine. We should probably note in the code
> about that.
>

Yes, I agree.  In fact, I'd like to make a follow-up patch (though probably not this week) that cleans up some of the context management.  I'll file a separate bug for this.

> @@ +783,5 @@
> > +  if (curWindow.contentWindow == null) {
> > +    // The frame we want to switch to is a remote frame; notify our parent to handle
> > +    // the switch.
> > +    curWindow = content;
> > +    sendToServer('Marionette:switchToFrame', {win: winUtil.outerWindowID, frame: foundFrame});
> 
> Where is sendToServer defined?

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#193

Comment 19

[David Burns :automatedtester]

> > We should still set the focus to the curWindow (http://code.google.com/p/selenium
> > /wiki/JsonWireProtocol#/session/:sessionId/frame)
> 
> In my interpretation, the Selenium statement "Change focus to another frame
> on the page" means that the Marionette/Selenium focus will change; i.e., all
> future commands will be directed at the new frame, rather than requiring
> that the frame have user focus.
> 
> Personally, I find it quite annoying that Firefox keeps bringing itself to
> the foreground while running Marionette tests.

This problem with it needing to be in the foreground is mainly because of bug 566671

> 
> David, what's your opinion on the meaning of this statement in the JSON Wire
> Protocol docs?

My interpretation is the same as yours. I would expect this would switch to the default frame[1] and then that would handle the new requests in there.

http://selenium.googlecode.com/svn/trunk/docs/api/py/webdriver_remote/selenium.webdriver.remote.webdriver.html#selenium.webdriver.remote.webdriver.WebDriver.switch_to_default_content

Comment 20

[Jonathan Griffin (:jgriffin)]

I've updated the patch to restore the focus() calls and pushed to try:  https://tbpl.mozilla.org/?tree=Try&rev=b88b4a9e6407&noignore=1

Comment 21

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #20)
> I've updated the patch to restore the focus() calls and pushed to try: 
> https://tbpl.mozilla.org/?tree=Try&rev=b88b4a9e6407&noignore=1

This run was green except for a timeout on Windows 7, which occurs with or without this patch.

Comment 22

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/mozilla-central/rev/dcc6a2d0bd16