1559120 - [meta] Fix remaining issues with GeckoView in Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[James Graham [:jgraham]]

TypeError: this.tabBrowser.addTab is not a function

Comment 1

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

This comes from: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251607369&repo=try&lineNumber=1225

Here the JS stack:

[task 2019-06-13T10:10:48.665Z] 10:10:48 WARNING - openTab@chrome://marionette/content/browser.js:430:31
[task 2019-06-13T10:10:48.665Z] 10:10:48 WARNING - GeckoDriver.prototype.newWindow@chrome://marionette/content/driver.js:2761:39

It's called here:

https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/testing/marionette/browser.js#414

What I wonder is why Services.appinfo.name.toLowerCase() returns fennec and not something else.

Beside that it means we have to stop using it because geckoview is embedded and can have multiple names. As such we will have to do method sniffing instead.

Note that there are also other areas where updates are necessary. So updating the summary accordingly.

Comment 2

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

As James said it works in general, but there are some commands which need to be fixed.

Comment 3

[Andreas Tolfsen ❲:ato❳]

I’m confused. Is this a tracking bug for remaining GeckoView issues,
about supporting the New Window command, or for changing the product
comparison code? (We’ve had similar woes about using the app ID in
the past, for the same reasons.)

tl;dr: What is this bug about?

Comment 4

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

As it has been mentioned on comment 0 browser.js will be one spot. And there might be others.

Actually I missed in my original comment to also add the link to the try build James was running with the necessary patches to be applied, so here is the list:

https://hg.mozilla.org/try/rev/ff78f063e42cc503fd6e79525c54c09a2a45437a
https://hg.mozilla.org/try/rev/41c1c942dfcb4f37580ff0a860e38e9d1dba9c66

Comment 5

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

Note that there is Services.androidBridge.isFennec available to check if it is Fennec or a GeckoView based browser:

Here the interface definition:
https://searchfox.org/mozilla-central/rev/8ed8474757695cdae047150a0eaf94a5f1c96dbe/widget/android/nsIAndroidBridge.idl#83

Comment 6

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

There is one behavior of GeckoView based applications which isn't clear to me yet. What should happen when window.open() gets called? Is a new tab opened or a new window. Or does it even depend on the vehicle itself how it handles that? If it is the latter what is the best way to wait for the new window/tab to have been opened? What kind of event or observer could be used here? Or would we have to poll a specific state?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #6)

There is one behavior of GeckoView based applications which isn't clear to me yet. What should happen when window.open() gets called? Is a new tab opened or a new window. Or does it even depend on the vehicle itself how it handles that?

I think it must depend on the vehicle itself. I'm free-handing this, but the mental model I'm using is by comparison with an Android WebView. For a browser-like vehicle, window.open() should do what browsers do on Android, which is generally open a new tab (with window.opener set, etc). But for, say, browsing inside the Facebook App, there's no concept of tabs, so I don't know what happens. Maybe the current window.location is replaced (so we load in the same WebView)? Maybe the function is just ignored? That depends on the vehicle, but what is clear is that WebView has a callback interface for the consumer to say what they want; see, for example this discussion (which itself links to API docs). GeckoView must (or already does) provide some version of that callback interface API.

If it is the latter what is the best way to wait for the new window/tab to have been opened? What kind of event or observer could be used here? Or would we have to poll a specific state?

My expectation is that we can have GeckoView's callback interface API include enough richness that Marionette (and other automation-y things) can discover that the window.open has been processed and either a new window has been opened (or a new tab?), or the request has been denied. Here I really am free-handing, 'cuz I don't know what GeckoView mechanisms there are for this. I really hope we can avoid polling!

What is it that Marionette actually needs to know about the window.open? Is the goal for Marionette to be able to open tabs/windows to implement the Web Driver API (or equivalent)? Or does Marionette need to know when arbitrary content does such opening, so that it can instrument the opened Web Contexts in some way?

Looking at #c0, the answer must be "don't use tabBrowser" 'cuz that's Fennec-specific. But that doesn't actually help you :(

Comment 8

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

(In reply to Nick Alexander :nalexander [he/him] from comment #7)

What is it that Marionette actually needs to know about the window.open? Is the goal for Marionette to be able to open tabs/windows to implement the Web Driver API (or equivalent)? Or does Marionette need to know when arbitrary content does such opening, so that it can instrument the opened Web Contexts in some way?

It's the former. And here especially to get the New Window command implemented. For Firefox there are dedicated methods to open a new browser window or a tab, so that I know what I have to wait for.

Looking at #c0, the answer must be "don't use tabBrowser" 'cuz that's Fennec-specific. But that doesn't actually help you :(

Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?

Comment 9

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)

Looking at #c0, the answer must be "don't use tabBrowser" 'cuz that's Fennec-specific. But that doesn't actually help you :(

Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?

Just to clarify. The term tabBrowser is a wrapper around gBrowser for Firefox, and BrowserApp for Fennec. So you are saying that BrowserApp should not be used, right?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #9)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)

Looking at #c0, the answer must be "don't use tabBrowser" 'cuz that's Fennec-specific. But that doesn't actually help you :(

Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?

Just to clarify. The term tabBrowser is a wrapper around gBrowser for Firefox, and BrowserApp for Fennec. So you are saying that BrowserApp should not be used, right?

Now I don't even know, 'cuz (very recently!) a BrowserApp was added to GeckoView for certain Web Extension API support: see https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewTab.jsm#81 and the commits that added it.

Agi, can you suggest how Henrik might use the newly added support for tab (and window?) management built into GeckoView from Marionette? I'm not familiar with the new changes.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

We can most likely reuse the infrastructure we just built for WebExtension support. I believe we do use BrowserApp for it, but that was supposed to change soon? robwu should know.

As an aside, window.open calls into onNewSession in GeckoView and then it's up to the embedder to decide what to do. Looks like the TestRunnerActivity creates a background session: https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java#67-69

Comment 12

[Rob Wu [:robwu]]

BrowserApp and gBrowser should not be used on GeckoView. They are very minimal transitional aids to help with getting the extension API to work. Its implementation is at: https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/mobile/android/modules/geckoview/GeckoViewTab.jsm

To support the tabs.create extension API, we added new delegates in the app side, so that apps can customize how to handle tab open requests.

If you're able to implement (part of) Marionette's functionality with a WebExtension, then the same infrastructure can be reused.
Otherwise, you have to reuse something else (maybe methods on the nsIBrowserDOMWindow interface?), or roll your own. If you're able to call window.open from a content page, then that may also serve your needs.

(window.open doesn't work in the browser process, see bug 1560842)

Comment 13

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

I don't think that we will change parts of Marionette to let it depend on the WebExtension APIs. Does that mean we shouldn't use anything from the GeckoViewTab.jsm module in Marionette?

The code of Marionette which is basically affected is in:
https://searchfox.org/mozilla-central/source/testing/marionette/browser.js

What we mostly use it for is to get the linked browser for a tab, and to add, switch, and close different tabs.

If there is no gBrower or BrowserApp some questions come up:

1. Is there a way to retrieve all available instances of GeckoView (whether those are windows or tabs) within a vehicle? To test it which applications would you suggest? As best I would need one which has tabs, and one which only allows windows; not sure if there is one for both.

2. If we don't use gBrowser and BrowserApp for GeckoView, how could we determine if tabs are supported? If yes, what would be the best way to retrieve the currently selected tab, or to change that?

3. How do we get to the linked browser (and its browsing context) of the current tab/window from inside the parent process where the extension code is running?

4. The nsIBrowserDOMWindow interface looks fine, but different open methods still expect a where argument. So based on my question 1) we would have to know if tabs/windows are supported, and open it appropriately.

Comment 14

[Rob Wu [:robwu]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #13)

I don't think that we will change parts of Marionette to let it depend on the WebExtension APIs. Does that mean we shouldn't use anything from the GeckoViewTab.jsm module in Marionette?

Indeed, don't use anything from GeckoViewTab.jsm.

What we mostly use it for is to get the linked browser for a tab, and to add, switch, and close different tabs.

GeckoView doesn't have tabs, only windows. Each window contains one browser, see geckoview.xul/geckoview.js.

1. Is there a way to retrieve all available instances of GeckoView (whether those are windows or tabs) within a vehicle? To test it which applications would you suggest? As best I would need one which has tabs, and one which only allows windows; not sure if there is one for both.

Use Services.wm - see nsIWindowMediator.idl. Use "geckoview:browser" instead of "navigator:browser".

2. If we don't use gBrowser and BrowserApp for GeckoView, how could we determine if tabs are supported? If yes, what would be the best way to retrieve the currently selected tab, or to change that?

GeckoView doesn't know about tabs. An app may be extended to have tabs UI (via an android component), but afaik it is not exposed to GeckoView. Someone from the GeckoView or mobile teams may offer a more complete answer here.

I don't know if it is possible to switch windows from frontend (JS) code in GeckoView. This is still a to-do in the extension API, and dependent on bug 1565536.

3. How do we get to the linked browser (and its browsing context) of the current tab/window from inside the parent process where the extension code is running?

let win = Services.wm.getMostRecentWindow("navigator:geckoview");
let browser = win.browser;

4. The nsIBrowserDOMWindow interface looks fine, but different open methods still expect a where argument. So based on my question 1) we would have to know if tabs/windows are supported, and open it appropriately.

You can look up the implementation at https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm

Comment 15

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

Currently I'm not working on this bug.

Comment 16

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

Bug 1628117 makes some changes to browser.js, which might already help here. Lets see what else is remaining once that other patch got landed!

Comment 17

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

Note that reftests are currently always setting window.gBrowser and that leads to a wrong detection of the tabBrowser on Android. A fix for that is part of my patch series on bug 1661495.

Comment 18

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

On bug 1749444 I'm actually going to enable the tests first. Once done it will be easier to get the remaining features added given that we have a test bed.

Comment 19

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

All dependencies are finally fixed. As such I would close this meta bug, and for other upcoming issues and enhancements we will create individual bugs.