1287904 - Unable to locate elements within remote pages included as iframes in about: pages

Status: RESOLVED INVALID

(Remote Protocol :: Marionette, defect, P3)

Description

[Justin Potts [:justinpotts]]

Steps to reproduce:
1. Ensure selenium is installed and geckodriver (wires) is in your path
2. Run code (I used iPython to do it line by line) https://justinpotts.pastebin.mozilla.org/8885271

Expected result: Element should be found

Actual result: No element with ID "intro" is found, and switching context into the browser element causes program to hang

We need to be able to do this to test the new discovery pane inside about:addons.

Is there a solution for finding elements inside this embedded browser element?

Comment 1

[David Burns :automatedtester]

Moving pastebin to bug in case we lose the pastebin

from selenium.webdriver import Firefox
from selenium.webdriver.common.keys import Keys


f = Firefox(capabilities={'marionette':True})
html = f.find_element_by_tag_name('html')
html.send_keys(Keys.COMMAND + Keys.SHIFT + 'a') # Opens the about:addons page
browser = html.find_element_by_id('discover-browser') # Proves browser element exists
elem = browser.find_element_by_id('intro')

Comment 2

[David Burns :automatedtester]

Justin,

Do you navigate at any point or do something specific to load a page?

Comment 3

[Justin Potts [:justinpotts]]

The only navigation we're doing at the moment is using the hotkeys to open about:addons.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Justin Potts [:justinpotts] from comment #3)
> The only navigation we're doing at the moment is using the hotkeys to open
> about:addons.

I believe the same issue can be replicated by navigating to 'about:addons'.

Comment 5

[David Burns :automatedtester]

About pages do have their own quirks and a number of Chrome features (Bug 1288336). As such this is not a priority for us as we shore other bits and pieces up for us to support Selenium users.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Keep in mind that the discovery pane is a remote HTML page and as such lives as a frame inside the addons manager. So you will have to switch to the remote frame before you can access any elements.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I did some investigation and the problem here is in lister.js. We actually successfully allow the <browser> element to be switched to when calling `switch_to_frame()` but we fail in the following line due to undefined returned by `curContainer.frame.wrappedJSObject`:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#1432

>  let frameValue = element.toJson(
>      curContainer.frame.wrappedJSObject, seenEls)[element.Key];

I'm not sure why we are using wrappedJSObject here for DOM nodes, given that its normal usage is for XPCOM components implemented in JS. For DOM objects the Xray vision (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision) should be used.

When I make this small change in the above line I can successfully switch to the discovery pane and can find any of its elements.

Mike, would you mind checking the above link and tell me if I'm correct with my assumption?

We aren't using wrappedJSObject a lot in marionette, so we might have to review all its usage:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Fmarionette+wrappedJSObject&redirect=false

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/listener.
> js#1432
> 
> >  let frameValue = element.toJson(
> >      curContainer.frame.wrappedJSObject, seenEls)[element.Key];
> 
> I'm not sure why we are using wrappedJSObject here for DOM nodes, given that
> its normal usage is for XPCOM components implemented in JS. For DOM objects
> the Xray vision
> (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision) should
> be used.

It's not 100% clear to me exactly what this code is attempting to look up here (I'm not familiar with element.js, element.Key, etc), however, I know that wrappedJSObject is (unfortunately) overloaded.

When dealing with JS-implemented XPCOM components, wrappedJSObject is usually the mechanism that we use to make it possible to access the underlying JS objects.

That's not what's happening here.

When dealing with something like a content window (which is what curContainer.frame refers to), wrappedJSObject is how we're able to look at that window object with Xrays waived. This is an alternative to using Cu.waiveXrays. See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision#Waiving_Xray_vision

So it's not clear to me why Cu.waiveXrays(curContainer.frame) should be any different than curContainer.frame.wrappedJSObject. At least according to my understanding, they should be equivalent.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8)
> So it's not clear to me why Cu.waiveXrays(curContainer.frame) should be any
> different than curContainer.frame.wrappedJSObject. At least according to my
> understanding, they should be equivalent.

So I had a chat with Mike today about this topic and as it turned out why wrappedJSObject is not working here, is that the browser element is actually XUL and part of the in-content (chrome-)page of about:addons. That means using wrappedJSObject on an element from chrome will not work, while `waiveXrays()` seems to work transparently and simply returns the same object. 

But the underlying problem we have seen here is the different meaning of chrome and content, especially for those in-content chrome pages. While in Marionette we have a strict separation (chrome is Firefox ui, and content are tabs) we have a mixture in Firefox itself. 

The question now is if using `waiveXrays()` is safe enough when we use it in listener.js, or if we should better add a complete separate code path for such in-content chrome pages. I will chat with Andreas tomorrow to get a better understanding of the security model. If more questions will come up I will have to seek out for answers from e.g. Blake.

Also as a side-note `element.toJson()` seems to hang when called with undefined as first argument. This might warrant a separate bug to be filed.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I talked with Andreas already Wednesday last week. Sorry for not updating this bug earlier.

We both think that using waveXrays() instead of wrappedJSObject should not be a problem given that both are basically doing the same thing, whereby the former can transparently handle non-xray DOM nodes which are those XUL nodes embedded in an in-content chrome page (about:addons).

The file where this change has to be made (listener.js) is running as a privileged frame script. So the code from Marionette is executed with chrome privileges in Firefox. The question which raises now is how safe is it to operate with waveXrays() on XUL <browser> nodes which include arbitrary websites (eg. the discovery pane in about:addons). We don't know if those websites can actually set expandos which could be used for malicious code injection.

Blake, maybe you could give us some feedback for the above case? Any other details you miss? Please let me know. Thanks!

I for myself will try to find some test cases which actually hit the frame switching code in listener.js. Right now none of our unit tests do that. Once done I will check the other wrappedJSObject usages and see if those could be moved to waiveXrays too.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> [...] We don't know if those
> websites can actually set expandos which could be used for malicious code
> injection.

You don't have to worry at all about untrusted web pages creating evil expandos on their enclosing browser elements. If they could, that would be a major security problem that we'd have to fix. We should only need to create or use Xray wrappers on content objects.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> You don't have to worry at all about untrusted web pages creating evil
> expandos on their enclosing browser elements. If they could, that would be a
> major security problem that we'd have to fix. We should only need to create
> or use Xray wrappers on content objects.

That's great to hear! Is there an easy way to determine if we have a content object or a chrome one? Just in case I should not call waveXrays() on the browser element. But if that doesn't matter at all, I would simply replace wrappedJSObject with it.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Henrik Skupin (:whimboo) from comment #12)
> That's great to hear! Is there an easy way to determine if we have a content
> object or a chrome one? Just in case I should not call waveXrays() on the
> browser element. But if that doesn't matter at all, I would simply replace
> wrappedJSObject with it.

You can just call waiveXrays unconditionally and it'll be fine.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried to change some code to not use wrappedJSObject but waiveXrays(). In case of our frame scripts like listener.js it will work because they run with chrome privileges but actual tests operating in content context will fail. This is because waiveXRays() is only accessible from scripts running with chrome privileges. But if a test does so, the eg. referenced window object of a `execute_script()` call is no longer the content (tab) but the chrome window.

Given the above I would say that I change only the wrappedJSObject usages for our marionette frame scripts, and leave all the other instances alone.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I don't feel well to make those changes right now. The reason is that NO single unit test we currently have make use of the switchToFrame code in listener.js! How's that? It's really suspicious because I see code and test additions/changes for those changesets:

https://hg.mozilla.org/mozilla-central/rev/69d426d27d31 (Dave Hunt)
https://hg.mozilla.org/mozilla-central/rev/20a18be004c7 (David Burns)
https://hg.mozilla.org/mozilla-central/rev/dfe1fe631991 (David Burns)
https://hg.mozilla.org/mozilla-central/rev/cee1e54815f2 (Malini Das)
https://hg.mozilla.org/mozilla-central/rev/77e5b3ac81e9 (David Burns)
https://hg.mozilla.org/mozilla-central/rev/54bdba1494af (Malini Das)

I'm not able to get further than bug 712643 because I don't know where Marionette development happened before.

David, do you have any idea for what all this code in switchToFrame is used for? Maybe close to all of that for B2G?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Or maybe the switchToFrame() version in driver.js is handling all of the different code paths nowadays so that we never reach the content scope path (second link)?

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1381
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#1514

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Or maybe the switchToFrame() version in driver.js is handling all of the
> different code paths nowadays so that we never reach the content scope path
> (second link)?

As it looks like this is the case! Even for frames in content we handle everything in the driver.js's version of switchToFrame(). This applies to all test_switch_frame* tests.

Before I want to do any change here, I would like to get feedback from Andreas in how best to proceed. If all the code on listener.js is mood those days, we should get it removed and the specific case for this bug added to the driver.js version.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I have to add that we indeed reach the content condition but when listener.switchToFrame() is called none of the current tests actually reach this method! It's only my new case which enters this method in listener.js:

from a test in test_switch_frames.py:
1470842779455	Marionette	INFO	************** content
1470842779456	Marionette	INFO	*************** before listener switchToFrame *****
1470842779457	Marionette	INFO	(...args) => obj.send(prop, args)

from the discovery pane test:
1470842777394	Marionette	INFO	************** content
1470842777394	Marionette	INFO	*************** before listener switchToFrame *****
1470842777394	Marionette	INFO	(...args) => obj.send(prop, args)
1470842777397	Marionette	INFO	*************** switchtoFrame (listener.js) **********
1470842777398	Marionette	INFO	******************** key: element-6066-11e4-a52e-4f735466cecf
1470842777398	Marionette	INFO	******************** wrappedJSObject: undefined
1470842777398	Marionette	INFO	******************** waiveXrays: [object XULElement]
1470842777398	Marionette	INFO	******************** json: 64679511-97d4-4e10-8205-6d39bc717e5b

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: testcase

This is a testcase for switching between iframes in content, and the discovery pane. Just run it via `mach marionette-test`.

Comment 20

[Karl Thiessen [:kthiessen, he/him]]

We have a bug in FxA end-to-end testing which looks a lot like this: 
   https://github.com/mozilla/fxa/issues/104

I have two questions:

1) Is it likely that the cause of this bug is the same as the cause of the issue above?

2) Do we have a timeline for when this bug is likely to be resolved?

Please feel free to reply either in this bug or in the above issue.  Thanks very much.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Karl Thiessen [:kthiessen] from comment #20)
> 1) Is it likely that the cause of this bug is the same as the cause of the
> issue above?

This is something I have already mentioned to you all in a reply to the email you sent out a while ago. So yes, I highly doubt so due to the remote frame inside about:accounts.

> 2) Do we have a timeline for when this bug is likely to be resolved?

No, given that I still wait for an answer from Andreas who knows this module way better than me.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Andreas, why did you remove the ni? I'm still waiting for feedback from you.

Comment 23

[Andreas Tolfsen ❲:ato❳]

I don’t have bandwidth for this now.

Comment 24

[Chris Peterson [:cpeterson]]

Closing as invalid because remote pages are no longer included as iframes in about: pages.