801703 - marionette-listeners should get all iframe elements instead of using window.frames

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

Attachment: Failing test case

When switching frames, we should look at the iframes that can be found under the document, instead of attached to window.frames. 

Marionette-actors.js is already doing the right thing, but marionette-listener.js has to be updated to look at document.getElementsbyTagName('iframe') instead of just window.frames, since there are times when the iframe can be found on the document, but not in window.frames (like mozbrowser from within the Browser in B2G).

I've attached a test that Zac wrote to reproduce this on B2G. Line 38 will fail

Comment 1

[Zac C (:zac)]

Thanks for very quick debug :mdas.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

This is blocking Zac from writing about 4 Gaia smoketests right now, so appreciate traction on this -- thanks!

Comment 3

[David Burns :automatedtester]

deleted severity by accident, adding it back

Comment 4

[David Burns :automatedtester]

Attachment: Using getElementsByTagName

Comment 5

[David Burns :automatedtester]

This hasn't been tested on B2G but works on desktop.

Comment 6

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

Comment on attachment 671965 [details] [diff] [review]
Using getElementsByTagName

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

Sorry for the delay, I wanted to verify that it worked in b2g. Thanks for addressing this!

Comment 7

[David Burns :automatedtester]

landed in m-i https://hg.mozilla.org/integration/mozilla-inbound/rev/77e5b3ac81e9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77e5b3ac81e9

Comment 9

[Zac C (:zac)]

For the original test case I am still getting a failure.

Here is the stack trace:




test_browser_basic (test_browser_2.TestBrowser) ... True
{7bd2c041-06ff-4e59-958f-8bfaec75c914}
{7bd2c041-06ff-4e59-958f-8bfaec75c914}
ERROR

======================================================================
ERROR: test_browser_basic (test_browser_2.TestBrowser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/zacdev/Mozilla/gaia-ui-tests/gaiatest/tests/test_browser_2.py", line 38, in test_browser_basic
    self.marionette.switch_to_frame(browser_frame)
  File "/home/zacdev/.virtualenvs/gaia-ui-tests/local/lib/python2.7/site-packages/marionette_client-0.5.1-py2.7.egg/marionette/marionette.py", line 312, in switch_to_frame
    response = self._send_message('switchToFrame', 'ok', element=frame.id)
  File "/home/zacdev/.virtualenvs/gaia-ui-tests/local/lib/python2.7/site-packages/marionette_client-0.5.1-py2.7.egg/marionette/marionette.py", line 183, in _send_message
    self._handle_error(response)
  File "/home/zacdev/.virtualenvs/gaia-ui-tests/local/lib/python2.7/site-packages/marionette_client-0.5.1-py2.7.egg/marionette/marionette.py", line 206, in _handle_error
    raise NoSuchFrameException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL : NoSuchFrameException: Unable to locate frame: undefined
START LOG:
INFO TEST-START: /home/zacdev/Mozilla/gaia-ui-tests/gaiatest/tests/test_browser_2.py:test_browser_basic Tue Oct 23 2012 15:50:45 GMT+0000 (GMT)
INFO TEST-END: /home/zacdev/Mozilla/gaia-ui-tests/gaiatest/tests/test_browser_2.py:test_browser_basic Tue Oct 23 2012 15:51:22 GMT+0000 (GMT)
END LOG:
----------------------------------------------------------------------
Ran 1 test in 37.701s

Comment 10

[Zac C (:zac)]

Apologies I think this is due to using an old B2G build.

Comment 11

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

Comment on attachment 671965 [details] [diff] [review]
Using getElementsByTagName

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

We need to get this in aurora so we can unblock WebQA

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: We won't be able to run some WebQA/marketplace tests for B2G
Testing completed (on m-c, etc.): passed on m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: N/A

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 671965 [details] [diff] [review]
Using getElementsByTagName

We don't need to request approval-mozilla-aurora for test-only patches any longer.  Just land on ash, and if it doesn't break any automation there, land directly on aurora with a=NPOTB.

Comment 13

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

pushed to ash:

https://hg.mozilla.org/projects/ash/rev/4c43f7492cf4

Comment 14

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

Scratch that, this is for another bug.

Comment 15

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

landed in Ash (aurora-try):
https://tbpl.mozilla.org/?tree=Ash&rev=8922e65c4cb3

Comment 16

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

Passed in Ash, landing in aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/432029759616

Comment 17

[Zac C (:zac)]

I can now switch to the frame but I can't perform any marionette commands on the frame.

Using the test case in the original bug posting, uncomment the print page_source on line 40 and run the test again.

After switching to the frame (which appears to be successful, at least it's not failing as it used to) the marionette command will time out.

Is marionette attached to the new frame?

Comment 18

[David Burns :automatedtester]

assigning to :mdas since she is currently looking into this

Comment 19

[Jonathan Griffin (:jgriffin)]

I tested Malini's patch, and the patch does in fact fix a bug, but it exposes another problem, so doesn't solve this in general.

The problem is that the browser's OOP iframe is inside another in-process iframe inside the main document.  So, it's a nested iframe.  Marionette's logic for dealing with OOP iframes assumes that the OOP iframes will be direct children of the main document, which is unfortunately not the case here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#1609

Normally when switching to an OOP frame, marionette-listeners sends a message to marionette-actors (which has to handle the switch) with the frame index of the frame you want to switch into.  It is expected that this index is a member of window.frames[].  However, in the specific case of the B2G browser, it isn't, because in this case the index points to a member of the frames[] list inside the browser iframe, not the top-level window.frames[].

Somehow we need to let the :switchToFrame message convey a parent window that can be used in marionette-actors when trying to locate the target frame.

Comment 20

[Zac C (:zac)]

A second test case for which this patch could be tested against is the test_marketplace.py ( https://github.com/zacc/gaia-ui-tests/blob/master/gaiatest/tests/test_marketplace.py#L43 ) with the commented out steps removed, of course!

I'd be happy to wait a little bit longer for the original use case to be solved if the marketplace/persona use case was solved by this patch in the meantime.

Comment 21

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

Attachment: pass reference to parent window when switching OOP frames. patch v1.0

So this patch now passes zac's test and passes all our unit tests in both emulator and browser!

Comment 22

[Jonathan Griffin (:jgriffin)]

Comment on attachment 678547 [details] [diff] [review]
pass reference to parent window when switching OOP frames. patch v1.0

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

Sweet, thanks!

Comment 23

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

testing on try (to monitor marionette tests):

http://hg.mozilla.org/try/pushloghtml?changeset=6397717c39bb

testing on ash:

https://hg.mozilla.org/projects/ash/rev/b96282aae3b9

Comment 24

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

I fail at pushing to ash. Had to remerge:

https://hg.mozilla.org/projects/ash/rev/fe559e054e46

It's passing on try, so I'll push to m-c as soon as I see green in ash

Comment 25

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

landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee1e54815

landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/084811a4a947

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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