https://w3c.github.io/webdriver/#new-window is pretty specific that the new window should be about:blank and instead opening with the new tab page is obsrvably different from this.

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

It means that we should hard-code opening the window as about:blank, and not relying on the newtabpage preference value.

Problem is that we are using OpenBrowserWindow(), and it doesn't seem to support specifying a URL to load.

Gijs, do you know of a way to force that method to open a new browser window displaying about:blank?

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

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

This seems fixable. Why can't we just set the pref for the wptrunner profile?

Gijs, do you know of a way to force that method to open a new browser window displaying about:blank?

Calling the relevant internal APIs by hand, which I don't want us to do because we should be reducing the number of ways we have of opening windows, and making their APIs sane, not adding more consumers of the internal window watcher APIs or similar (which will make any refactoring even harder than it already is).

Opening a new window opens your homepage, not the new tab page, although they can be set to be the same thing. I think you'd want to override the homepage pref like this https://hg.mozilla.org/try/rev/fe589126355655da96dc13cf47ec23095b2d191b

(Sorry, this was out of context.)

So I also noticed this as problem on Fennec while investigating bug 1536583. Reason is that Fennec doesn't obey browser.newtabpage.enabled and as such displays this page after opening a new tab. I can actually pretty easily fix that by passing about:blank into the browserApp.addTab() method.

For Firefox this is more complicated. As mentioned above whether OpenBrowserWindow() nor BrowserOpenTab() allow me to pass in a URL. Which means that tests for a different harness (like web-platform-tests) will fail, when browser.newtabpage.enabled = false isn't set.

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

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

This seems fixable. Why can't we just set the pref for the wptrunner profile?

James doesn't really want that. So maybe he can explain.

Calling the relevant internal APIs by hand, which I don't want us to do because we should be reducing the number of ways we have of opening windows, and making their APIs sane, not adding more consumers of the internal window watcher APIs or similar (which will make any refactoring even harder than it already is).

Having another look over those methods I can understand that. The target URL is kinda nested deep in sub methods. Also all the extra features eg. private browsing is not something I'm particularly interested in replicating, and as such say would need to be updated all the time when something changes in the browser too.

While I see it isn't elegant to rely on the preference while the WebDriver spec says to hard-code about:blank, each browser vendor has to find the optimal way to get that working. And hard-coding seems to be nearly impossible right now for an ideal solution.

This seems fixable. Why can't we just set the pref for the wptrunner profile?
James doesn't really want that. So maybe he can explain.

Only because the WebDriver spec requires that we get about:blank in new windows for any marionette session, so fixing this at the wptrunner level is wrong.

I don't mind if we rely on a pref, but marionette needs to ensure it's correctly set.

Please note that web-platform-tests don't use the preferences as set by marionette in geckoinstance.py, nor the recommended preferences when marionette.prefs.recommended is set to True. Instead the profile is created on its own by collecting the various preferences under testing/profiles. Note that Marionette doesn't use those yet.

I assume that you don't want that the recommended preferences are getting set by Marionette. See the list of prefs here:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionette.js#62

As such I can only see that we add browser.newtabpage.enabled = false to testing/profiles/web-platform/user.js.

Well we could also use the prefs service to ensure the pref is set before trying to open the new window.

Oh! That is actually a perfect solution. I totally missed that. So yes, that sounds like the proper solution here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0054681c542c7f21e571aafb674f4e82f536b96

James, would you mind trying https://hg.mozilla.org/try/rev/a9f2bf3bdb792b0c4b3e6d1bf7ea7b6544aa0c91 if that fixes the problem for you? Thanks!

Depends on D34831

Moving the needinfo request to the revision in Phabricator itself.

Note, the patch for Fennec will be moved out to bug 1506782.

This covers desktop only now. So moving the whiteboard entry also to bug 1506782.

Seeing how we make progress on bug 1506782 I wonder if the proposed solution for Firefox is even correct. It would also be affected by preference name changes.

What if we simply wait for the initial page load (whatever that loads) in a new tab and window, and then do another navigation to "about:blank". Especially for mobile applications based on GeckoView this seems to be the only available option.

James and Andreas, what do you think?

Preference name changes should presumably be handled by the person renaming preferences grepping the source and replacing all occurences of the pref name. I suppose there's a problem that we want stronger compatibility guarantees than the frontend code, but I wonder how often we expect prefs to change. By contrast having multiple loads in the window seems like a constant performance hit and adds additional complexity that other code may need to account for.

There is a lot of context here that I may not have amassed, but if
WPT is explicitly disabling the recommended preferences, it needs
to bear the responsibility of that by setting the necessary preferences
in the profile it prepares.

Is there a particular reason why WPT does not want the recommended
preferences?

We do have the distinction between recommended and required
preferences in Marionette, but the latter is typically reserved
only for the preferences needed for the Marionette server to operate
correctly, and not for enforcing WebDriver conformance.

I also agree that a second navigation to about:blank is undesirable.
Am I missing the point, or could WPT not just set
browser.newtabpage.enabled or adopt marionette.prefs.recommended?

Of course we can set that pref, but it feels like the WebDriver implementation should follow the spec by opening about:blank for new tabs even if you've set some other site as the homepage for wahtever reason.

So what about a compromise here. Given that it is a feature highly needed for getting the New Window command used in WPT we set the preference for now in the profile. Then we can file a new bug to get a sane implementation without using this and probably other necessary preferences. As we know it's still more complicated for Fennec or mobile in general. See bug 1506782.

(In reply to James Graham [:jgraham] from comment #19)

Of course we can set that pref, but it feels like the WebDriver
implementation should follow the spec by opening about:blank for
new tabs even if you've set some other site as the homepage for
wahtever reason.

I see, so the distinction is that if there’s a user pref overriding
the recommended new tab page, that URL is opened instead on new
tabs.

The recommended preferences in Marionette deliberately does not
overrule user prefs, except for marionette.port which is required
for the server to function correctly. This behaviour means the users
can—at their own risk—override prefs necessary for WebDriver
conformance. This behaviour is transparent, because whatever pref
the user writes to the profile ends up taking effect. There’s no
misunderstanding why a set pref ends up not being respected.

If we were to change the recommended preferences to in essence act
as required, or over overriding preferences, we lose that transparency.
Suddenly you need the prerequisite knowledge to first disable
marionette.prefs.recommended in order for prefs set in user.js
to take effect. I think this is unfortunate because it encodes
behaviour that is not obvious from just reading user.js.

For the record I think the hardcoded option to enforcing about:blank
by setting the pref and resetting to its original value before the
opening a tab is worse in this regard, because then it’s not obvious
from either reading prefs.js or about:config what rules are in
effect.

I’m just trying to explain the reasoning behind the current model.
I’m open to arguments to the contrary!

All that said, I couldn’t find the instance where WPT explicitly
turns off marionette.prefs.recommended (the default is true),
which means it must some place set browser.newtabpage.enabled to
true for it to see this behaviour, right?

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

As we know it's still more complicated for Fennec or mobile in
general. See bug 1506782.

Judging by the look of it, using an intent argument will cause the
first run pane to disappear:
https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/firstrun/OnboardingHelper.java#89

I guess there’s no real concept of a new tab page in GeckoView? At
least getting rid of an onboarding pane seems sensible.

I talked to Gijs yesterday, and he is fine for having an additional argument for openBrowserWindow() and openBrowserTab() which allows consumers to override the initial URL. As he mentioned too for the latter method it should already be possible. So I will have to check that the next days, and will provide a patch for Firefox which adds it to openBrowserWindow().

Please lets not continue to discuss the topic about mobile on this bug but use bug 1506782 for that.

Currently I'm not working on this bug.

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

I talked to Gijs yesterday, and he is fine for having an additional argument for openBrowserWindow() and openBrowserTab() which allows consumers to override the initial URL. As he mentioned too for the latter method it should already be possible. So I will have to check that the next days, and will provide a patch for Firefox which adds it to openBrowserWindow().

Maybe I used the wrong method names previously or they have been refactored meanwhile. As it looks like we are looking now for BrowserOpenTab() and OpenBrowserWindow(). I cannot see that any of them actually allow to pass-in an initial URL to load.

Gijs, are those the correct methods for a possible modification?

As short-term workaround we could set browser.newtabpage.enabled = false and restore the value right after opening the tab/window.

The context is 2 years old so I re-read this bug, and a lot of it doesn't really make sense to me.

Either marionette/webdriver/geckodriver/wptrunner can set prefs and rely on them, or it cannot. It doesn't seem to make sense to both have a set of prefs you use to ensure solid operation, and also say "but consumers could change any of them" and yet also say, "but we should still behave solidly in the face of arbitrary pref changes by consumers". That way lies madness.

So I still think changing prefs is likely what you want here, and the real question is what is the right level/framework at/in which to set those prefs, and I cannot answer that. I don't think pref renames breaking you is a realistic concern; if we rename things we'll need to get whatever framework it is to set both prefs for a bit for compat, and then remove the old pref; that seems OK.

If, for some reason I'm completely missing, changing the prefs is really not feasible, then you probably want openLinkIn with the correct URL, a suitable triggering principal - maybe null principal if you don't have one, maybe system; and the right target ("window", or "tab").

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

If, for some reason I'm completely missing, changing the prefs is really not feasible, then you probably want openLinkIn with the correct URL, a suitable triggering principal - maybe null principal if you don't have one, maybe system; and the right target ("window", or "tab").

Thank you for the reply Gijs! It seems that your comment slipped through and I didn't notice it within the last 10 months. But while working on the improved handling of the initial page loading (bug 1747359) of a newly created tab / window I just saw it.

Basically the WebDriver specification requires us to always open a new tab or window with about:blank. And any settings the user did for preferences shouldn't affect this behavior. Having to set and to restore preferences just for this operation sounds fragile and an unnecessary step. So I'll indeed try to see if openLinkIn will be a good replacement here. Thanks.

Voting to add this bug to M3 with 1 point. Julian, do you agree?

Let's discuss this in triage today

Backed out for causing multiple web platform tests failures

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | /webdriver/tests/element_click/click.py | test_no_browsing_context - AssertionError: assert 'no such element' == 'no such window'
  log 2: https://treeherder.mozilla.org/logviewer?job_id=377985354&repo=autoland
  log 3: https://treeherder.mozilla.org/logviewer?job_id=377978826&repo=autoland
  range: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunning%2Cpending%2Crunnable&fromchange=4aa40d4543da126a325b799542bd3fa72777bb4b&searchStr=wd&tochange=7e6d07bef039bb85f64ba40b72cd2dc148ef80ad&selectedTaskRun=f-cVDbVKSfWxGCzfwvAUhQ.0

Affected test jobs are only the no-Fission wdspec tests on Linux 18.04 x64 WebRender Shippable. Other platforms and build types are ok. I'll have to check next week why it's failing here. As it looks like the new tab gets created as expected but closing the window doesn't work so that it stays around.

The failures only seem to happen for test_no_browsing_context tests across all supported commands. These tests open a new tab, load a frameset, switch to the inner-most frame and then delete this active frame by clicking a link. In the failing cases the click event is never received by by the element, and as such the frame is not removed. This results in a timeout of the TimedPromise when waiting for flushing the event loop:

https://searchfox.org/mozilla-central/rev/da6a85e615827d353e5ca0e05770d8d346b761a9/remote/marionette/interaction.js#407-434

Given that this code actually doesn't throw but only prints a log message the next WebDriver command will then fail.

I was not able to reproduce the problem locally yet and I wonder if this might be a race condition by not waiting long enough for the tab be fully ready. As such I pushed a try build to check if the click might be delayed but that's not the case and even after 5s no click event has been received:

https://treeherder.mozilla.org/jobs?repo=try&revision=f03cfe8ba5a65fa561dd68822e9b9e9518ae03fb&searchStr=wd

As the try build from the last comment shows it's actually not a race condition. It's a permanent situation with the focus in which Marionette is not able to simulate a click onto an element located in the newly created tab. It's interesting to see this difference between the this.window.BrowserOpenTab() and gBrowser.addTab() methods. Workaround would be to explicitly focus the new tab if needed, and otherwise focus back the former tab. So my suspicion would be that BrowserOpenTab() might also be affected in case of not immediately activating the new tab as what it actually does. I'll check this assumption now.

FWIW, tab selection changes on modern Firefox are never completely synchronous, see https://searchfox.org/mozilla-central/source/browser/modules/AsyncTabSwitcher.jsm .

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

FWIW, tab selection changes on modern Firefox are never completely synchronous, see https://searchfox.org/mozilla-central/source/browser/modules/AsyncTabSwitcher.jsm .

Yes, we are aware of that and as such are waiting for the TabSelect event on the browser object (gBrowser in Firefox). We are also aware that this event might be fired a little bit too early and it would be better to use TabSwitched instead, but that's also behaving not as expected in all the scenarios that we cover (see bug 1767314).

In regards of AsyncTabSwitcher I can see it used by gBrowser code internally, so I doubt that we have to use it directly. Is that correct? So far by using BrowserOpenTab() it was all fine and we didn't experience focus issues, but now it fails (Linux only) when we make use of gBrowser.addTab().

Gijs, do you have any other suggestion what we could do to figure out what exactly is broken here? I would appreciate. Thanks.

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

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

FWIW, tab selection changes on modern Firefox are never completely synchronous, see https://searchfox.org/mozilla-central/source/browser/modules/AsyncTabSwitcher.jsm .

Yes, we are aware of that and as such are waiting for the TabSelect event on the browser object (gBrowser in Firefox). We are also aware that this event might be fired a little bit too early and it would be better to use TabSwitched instead, but that's also behaving not as expected in all the scenarios that we cover (see bug 1767314).

In regards of AsyncTabSwitcher I can see it used by gBrowser code internally, so I doubt that we have to use it directly. Is that correct?

Well, there should be events for what you need, which it sounds like you'd already tried (but I don't have time to look at 1767314 right now)

So far by using BrowserOpenTab() it was all fine and we didn't experience focus issues, but now it fails (Linux only) when we make use of gBrowser.addTab().

This doesn't make much sense - BrowserOpenTab ends up calling the same code (ie addTab), via openTrustedLinkIn and then openLinkIn and then loadOneTab. Your question implies you're doing exactly the same thing, but get different behaviours, so the obvious follow-up question from me is: how is the eventual call to addTab different from the call you are now making? Are they both loading the same page? If so, are they both passing the same set of options?

If I had to guess, if you're using BrowserOpenTab you're always loading the new tab page, and that is preloaded (unless you turn that off with browser.newtab.preload to false), and perhaps with addTab you're either not loading the same thing or you're passing arguments in a way where the preload doesn't happen, and so in one case your page is loaded and ready for interaction more quickly than in the other.

Gijs, do you have any other suggestion what we could do to figure out what exactly is broken here? I would appreciate. Thanks.

Check whether the docshell has focus and/or the page is loaded (including all frames), in the child process, when the click event sent by your test arrives.

Gijs, thanks for your reply and the extra information. I tried to run of these checks in CI to verify what's wrong but then I actually found the underlying issue. It's indeed not related to both of these methods but happens because I pushed my patch without a proper rebase on top of the changes on bug 1749666 that landed a bit earlier. On that given bug a method that my patch now calls has been made async, and in my patch the await statement was missing. I pushed another try build which looks promising now:

https://treeherder.mozilla.org/jobs?repo=try&revision=f424b6a6086676ec73a4af52bf671301a9831dcf

I'm sure that we can land this patch pretty soon.

It looks like we are still not there yet. I can still see failures for Mn jobs in these CI jobs:
https://treeherder.mozilla.org/jobs?repo=try&revision=4e7e5e608121085f5db97579d042e70ddef1a444

So far I'm not able to reproduce them locally but will continue trying...

Today I have rebased my patch against latest mozilla-central and pushed a couple of try builds. The formerly seen failures are actually not visible anymore on the Linux machines in CI. Here a most recent try push with a couple of extra test jobs that haven't finished yet:

https://treeherder.mozilla.org/jobs?repo=try&revision=e78be2333a0d1a802082cc74d36e03ebc07d3526

Here a complete try push for the remote-protocol preset:
https://treeherder.mozilla.org/jobs?repo=try&revision=aae101b2395750ec8e533ddd67cbed80a56c1897

If no failure is seen anymore we may be able to get this pushed again.

We still have failures. While these have a lower frequency for Mn jobs it's probably likely that we would hit one definitely in the wdspec tests. As such we should further investigate and fix the issue.

In this wdspec job it's hanging now awaiting the element to be in view. I'll push some more try builds with additional logging in getPointerInteractablePaintTree to figure out what's wrong.

Given that I was not able to find out the reason for the intermittent failures on Linux in CI I wonder if the following change might be fine:

 function BrowserOpenTab(event) {
-  let where = "tab";
   let relatedToCurrent = false;
+  let url = BROWSER_NEW_TAB_URL;
+  let where = "tab";

   if (event) {
     where = whereToOpenLink(event, false, true);

     switch (where) {
       case "tab":
       case "tabshifted":
         // When accel-click or middle-click are used, open the new tab as
         // related to the current tab.
         relatedToCurrent = true;
         break;
       case "current":
         where = "tab";
         break;
     }
+
+    if (event.url) {
+      url = event.url;
+    }
   }
..
   Services.obs.notifyObservers(
     {
       wrappedJSObject: new Promise(resolve => {
-        openTrustedLinkIn(BROWSER_NEW_TAB_URL, where, {
+        openTrustedLinkIn(url, where, {
           relatedToCurrent,
           resolveOnNewTabCreated: resolve,
         });
       }),

Gijs, would that be ok with you? Given that it uses the same API as before I should be able to finish that up probably today to get you unblocked.

I'll just go ahead and submit a patch soon. If you don't like it we can abandon and use the former method, but not sure how quickly we can have this fixed due to the limitation that it only fails in CI and it's very hard to investigate.

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

I'll just go ahead and submit a patch soon. If you don't like it we can abandon and use the former method, but not sure how quickly we can have this fixed due to the limitation that it only fails in CI and it's very hard to investigate.

I don't think I understand the patch. Nothing sets event.url. So how would this help webdriver?

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

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

I'll just go ahead and submit a patch soon. If you don't like it we can abandon and use the former method, but not sure how quickly we can have this fixed due to the limitation that it only fails in CI and it's very hard to investigate.

I don't think I understand the patch. Nothing sets event.url. So how would this help webdriver?

Please see the patches. Marionette will call that method with the about:blank as url in the events parameter.

https://hg.mozilla.org/mozilla-central/rev/8f609d2ba0ed
https://hg.mozilla.org/mozilla-central/rev/c3b8a498bb35