1676671 - Refactor registration of browsers

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, task, P2)

Description

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

Currently the initial registration of browsers as done in WebDriver:NewSession is backed by Promises using the message manager from the framescript. When we want to stop running the framescript, that code needs to be refactored or at least conditionally executed if actors aren't enabled.

When actors are enabled we have to find a new way to register all open browsers. Given that actors are no longer immediately created we have no messages coming in from all the existent browsers anymore like for framescripts. So we need a different way in registering those.

In a proof of concept the following code actually works for content browsers as part of open tabs:

  if (MarionettePrefs.useActors) {
    for (let win of this.windows) {
      const tabBrowser = browser.getTabBrowser(win);
      for (const tab of tabBrowser.tabs) {
        const contentBrowser = browser.getBrowserForTab(tab);
        this.registerBrowser(contentBrowser.browsingContext.id, contentBrowser);
      }
    }
  } else {
    await registerBrowsers;
    await browserListening;
  }

But it misses remote browsers outside of tabs (note: we don't support those anyway right now).

Comment 1

[Chris Peterson [:cpeterson]]

Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).

Comment 2

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

Note that this also needs an update within setWindowHandle(). Sorry that I missed that before.

Comment 3

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

Here the appropriate PoC:

    if (!MarionettePrefs.useActors) {
      if (registerBrowsers && browserListening) {
        await registerBrowsers;
        const id = await browserListening;
        this.contentBrowsingContext = BrowsingContext.get(id);
      } else {
        this.contentBrowsingContext = null;
      }
    } else {
      if (winProperties.hasTabBrowser) {
        const tabBrowser = browser.getTabBrowser(winProperties.win);
        const contentBrowser = tabBrowser.selectedBrowser;
        logger.trace(pprint`** content browser: ${contentBrowser}`);
        this.contentBrowsingContext = contentBrowser.browsingContext;
        logger.trace(pprint`** content browsing context: ${this.contentBrowsingContext.id}`);

        this.registerBrowser(this.contentBrowsingContext.id, contentBrowser);
      } else {
        this.contentBrowsingContext = null;
      }

It actually would be good to do some more refactoring especially for registerBrowser(), which actually only needs the content browser to be passed over. The browsing context id it could automatically determine.

Comment 4

[Julian Descottes [:jdescottes]]

Previously unassigned myself because I was busy with DevTools tasks, which I should have mentioned here.
I cleared up my DevTools queue, so I am able to work on this now.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Register browsers without framescript events when using JSWindowActors

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Refactor driver::registerBrowser and browser::register to infer id from browser element

Depends on D98778

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Remove unused knownFrames array from browser.js

Depends on D98779

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Remove unused return value from driver::registerBrowser

Depends on D98780

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Stop listening to Marionette:Register when using JSWindowActors

Depends on D98781

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efa50f96f8ac
[marionette] Register browsers without framescript events when using JSWindowActors r=marionette-reviewers,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/68192555c760
[marionette] Refactor driver::registerBrowser and browser::register to infer id from browser element r=marionette-reviewers,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/94f71a0d5ee5
[marionette] Remove unused knownFrames array from browser.js r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/e8d3e51c0440
[marionette] Remove unused return value from driver::registerBrowser r=marionette-reviewers,maja_zf

Comment 11

[Atila Butkovits]

Backed out for telemetry failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/27fd7604ce17507322b95027c688154242e03560

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&revision=e8d3e51c0440803d7fbc9c80d97d837d9431e594&searchStr=os%2Cx%2C10.14%2Cwebrender%2Copt%2Ctelemetry%2Ctests%2Ctest-macosx1014-64-qr%2Fopt-telemetry-tests-client-e10s%2Cc&selectedTaskRun=cRaOfI0hRrOPWBKSgxRupw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=324171355&repo=autoland&lineNumber=2067

Comment 12

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

Julian, this is caused by a remoteness change. That part I recently mentioned on the reviews:
https://treeherder.mozilla.org/logviewer?job_id=324171355&repo=autoland&lineNumber=1963

I assume we miss to update a browser entry for the correct top-level browsing context. Can you add a new Mn unit test for it please?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] (away 12/07 - 12/11) from comment #12)

Julian, this is caused by a remoteness change. That part I recently mentioned on the reviews:
https://treeherder.mozilla.org/logviewer?job_id=324171355&repo=autoland&lineNumber=1963

I assume we miss to update a browser entry for the correct top-level browsing context. Can you add a new Mn unit test for it please?

Thanks for the heads up. I believe we need to update the registered id when we detect a remoteness change, around https://searchfox.org/mozilla-central/rev/4415bec7a49c50a338167d9c8934527b9cae59d0/testing/marionette/driver.js#921

A local quick fix seems to take care of the test failure. I'll write a test for it tomorrow.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676671 - [marionette] Remove unused return value from driver::registerBrowser

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18bbd9323bc9
[marionette] Register browsers without framescript events when using JSWindowActors r=marionette-reviewers,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/a0f2b8899193
[marionette] Refactor driver::registerBrowser and browser::register to infer id from browser element r=marionette-reviewers,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/8852c35eb897
[marionette] Remove unused knownFrames array from browser.js r=marionette-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/95cb222955ef
[marionette] Remove unused return value from driver::registerBrowser r=marionette-reviewers,maja_zf

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/18bbd9323bc9
https://hg.mozilla.org/mozilla-central/rev/a0f2b8899193
https://hg.mozilla.org/mozilla-central/rev/8852c35eb897
https://hg.mozilla.org/mozilla-central/rev/95cb222955ef