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

RESOLVED FIXED in mozilla19


5 years ago
5 years ago


(Reporter: mdas, Assigned: mdas)


Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)

Created attachment 671467 [details] [diff] [review]
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

5 years ago
Thanks for very quick debug :mdas.
Attachment #671467 - Attachment is patch: true
Attachment #671467 - Attachment mime type: text/x-python-script → text/plain
This is blocking Zac from writing about 4 Gaia smoketests right now, so appreciate traction on this -- thanks!
Severity: normal → major
Assignee: nobody → dburns
Severity: major → normal
deleted severity by accident, adding it back
Severity: normal → major
Created attachment 671965 [details] [diff] [review]
Using getElementsByTagName
Attachment #671965 - Flags: review?(mdas)
This hasn't been tested on B2G but works on desktop.
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!
Attachment #671965 - Flags: review?(mdas) → review+
landed in m-i
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Comment 9

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

Here is the stack trace:

test_browser_basic (test_browser_2.TestBrowser) ... True

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

Comment 10

5 years ago
Apologies I think this is due to using an old B2G build.
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
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
Attachment #671965 - Flags: approval-mozilla-aurora?
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.
Attachment #671965 - Flags: approval-mozilla-aurora?
Whiteboard: [automation-needed-in-aurora]
pushed to ash:
Scratch that, this is for another bug.
landed in Ash (aurora-try):
Passed in Ash, landing in aurora:
Whiteboard: [automation-needed-in-aurora]

Comment 17

5 years ago
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?
Resolution: FIXED → ---
assigning to :mdas since she is currently looking into this
Assignee: dburns → mdas
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:

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

5 years ago
A second test case for which this patch could be tested against is the ( ) 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.
Created attachment 678547 [details] [diff] [review]
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!
Attachment #671965 - Attachment is obsolete: true
Attachment #678547 - Flags: review?(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!
Attachment #678547 - Flags: review?(jgriffin) → review+
testing on try (to monitor marionette tests):

testing on ash:
I fail at pushing to ash. Had to remerge:

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

landed on mozilla-aurora:
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.