1795841 - Use TabManager.addTab to open new tabs from marionette with Firefox Desktop

Status: ASSIGNED

(Remote Protocol :: Marionette, task, P3)

Description

[Julian Descottes [:jdescottes]]

This was initially supposed to be handled in Bug 1533058, but for now we will keep using BrowserOpenTab to open tabs on Firefox desktop in marionette/browser.sys.mjs openTab.

We should consistently use TabManager.addTab to create tabs in the remote codebase, but doing so seems to make some tests intermittently fail.

We should also use the occasion to review which API should be used behing the scenes to open the tab. Right now TabManager.addTab uses tabBrowser.addTab, but maybe we should rely on window.BrowserOpenTab

Comment 1

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

By using BrowserOpenTab() to open a new tab in Firefox there is also the browser-open-newtab-start notification that gets send out for performance tracking. If we stop using this method and instead use gBrowser.addTab() we would loose that (or if needed would have to send it out ourselves?).

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

If we are going with gBrowser.addTab() we might have to wait for the Ubuntu 22.04 upgrade of test machines (bug 1725245) because using this API causes test failures with focus on Linux machines in CI only.

Comment 2

[:Gijs (he/him)]

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

By using BrowserOpenTab() to open a new tab in Firefox there is also the browser-open-newtab-start notification that gets send out for performance tracking. If we stop using this method and instead use gBrowser.addTab() we would loose that (or if needed would have to send it out ourselves?).

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

Yeah, I don't think the notification is relevant.

If we are going with gBrowser.addTab() we might have to wait for the Ubuntu 22.04 upgrade of test machines (bug 1725245) because using this API causes test failures with focus on Linux machines in CI only.

Do we know why? This smells like a race condition of some kind in marionette. The whole thing doesn't make much sense because BrowserOpenTab still ends up calling addTab at some point down the line. So presumably the only difference between the case where we lose the race and where we win the race is a parameter of some kind? I guess bug 1533058 comment 38 asked the same question.

From looking at that discussion and the issues that only reproduce on infra, I can't quite figure out whether there was analysis done in terms of what the difference for the two calls to addTab ends up being? That seems like the most straightforward way to resolve the potential difference so you end up with an identical call to addTab... you can just use local logging - you don't need to reproduce any failures on infra.

I guess if this switches to gBrowser.addTab we can revert the change in bug 1533058 to allow overriding the url for BrowserOpenTab...

Comment 3

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

(In reply to :Gijs (he/him) from comment #2)

Gijs, do you think that this notification could be important for some parts in Firefox or tests? By default we turn off Telemetry anyway in Marionette so maybe its irrelevant?

Yeah, I don't think the notification is relevant.

Thanks. That gives me confidence.

Do we know why? This smells like a race condition of some kind in marionette. The whole thing doesn't make much sense because BrowserOpenTab still ends up calling addTab at some point down the line. So presumably the only difference between the case where we lose the race and where we win the race is a parameter of some kind? I guess bug 1533058 comment 38 asked the same question.

Sadly not given that any additional logging that I was using on try was not helpful yet to figure out the issue. Also interactive tasks were broken since the infra moved to GCP. With bug 1786307 fixed yesterday I'm finally able to use interactive tasks again. I might have the time to start looking at this bug by next week. It's not a priority for now so it might take a bit.

From looking at that discussion and the issues that only reproduce on infra, I can't quite figure out whether there was analysis done in terms of what the difference for the two calls to addTab ends up being? That seems like the most straightforward way to resolve the potential difference so you end up with an identical call to addTab... you can just use local logging - you don't need to reproduce any failures on infra.

I would like to see the real effect of behavior in CI when I make changes to the code. That would make it faster in discovering at which layer on top of gBrowser.addTab() the broken behavior gets introduced. Once found me can use several MOZ_LOG features (like focus, focus manager, widget) to check the differences of focus handling.

I guess if this switches to gBrowser.addTab we can revert the change in bug 1533058 to allow overriding the url for BrowserOpenTab...

Yes, absolutely!

Comment 4

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

Maybe my patch on bug 1798655 might have helped here. As we noticed BrowerOpenTab() doesn't actually focus the tab's content but sets the focus to the chrome window. If code in content then tries to focus an element it will take longer and you cannot assume that it's focused when the call to focus() returns. As such we explicitly set the focus to the linkedBrowser now when switching between tabs.

Comment 5

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

I pushed a try build to check if it works now with the focus fix included:
https://treeherder.mozilla.org/jobs?repo=try&revision=2e10b64d972b1d927281572467d15fe248457620

Comment 6

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

As the try build shows the issue with scrolling an element into view is still remaining on Linux shippable builds. As such we will pick-up this bug again when there is a bit more time or maybe when we got new Linux workers with a more up-to-date Ubuntu release.

Comment 7

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

Attachment: Bug 1795841 - [marionette] Use TabManager.addTab to open new tabs from marionette with Firefox Desktop.

Comment 8

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

I've pushed another try build to check if that may work nowadays:
https://treeherder.mozilla.org/jobs?repo=try&revision=ffa70322e87f35b5dab5c048ef4645b067e47a68

Comment 9

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

As it looks like all the tests are passing now. I would consider finishing the patch and get it landed again.

Comment 10

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

Actually quite a lot of tests are failing now because Element Click commands do not really trigger a click event. As such especially for Wd spec tests the no_browsing_context (no frame) tests fail because the frame doesn't get removed within 500ms when clicking the remove frame link. I cannot make up a connection between this behavior and using gBrowser.addTab().

Given that I don't have the time for further checks right now we may have to indeed wait until the new Wayland Linux machines are used for both Marionette and Wdspec.

Comment 11

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

Out of curiosity I just pushed the same set of tests for the Wayland test machines (via --full) to see if it makes a difference:
https://treeherder.mozilla.org/jobs?repo=try&revision=e981e079467637d019b63b27c942114f8da4a993

Comment 12

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

So the new Wayland workers show the exact same problem. As such we have to find a way to get this working in a different way.