1692875 - Reduce the 300ms startup delay in XPIProvider.checkForChanges() for automation

Status: REOPENED

(Toolkit :: Add-ons Manager, defect, P2)

Description

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

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:87.0) Gecko/20100101 Firefox/87.0 ID:20210213095234

Once in a while I can see a startup delay by the Add-ons manager where it takes up to 300ms in XPIProvider.checkForChanges(). Not sure if those checks need to be synchronous during startup and have block the content processes to be created. If that is the case maybe we could improve the work as needed here to make it faster?

Here the gecko profile: https://share.firefox.dev/3dlVh9n

Comment 1

[Andrew Creskey [:acreskey]]

Ah, that's interesting :whimboo -- I have seen the same problem and logged it in Bug 1664025.

But in that case were running this code because of a mismatch between the profile and the gecko binary.
We determined that this should only occur on a version upgrade.

I'm curious as to what scenarios you are seeing this with?

Comment 2

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

I always see that when running tests with Marionette and geckodriver. Both create a fresh profile for each of the Firefox instances to start. As such it would affect the first time Firefox starts with a fresh profile.

But having this happening for a test framework that is launching the browser a lot of times this delay is kinda annoying, and also makes us way slower to startup as Chrome. See bug 1618354, which was filed by previous Puppeteer folks (Google).

Comment 3

[Andrew Swan [:aswan]]

If this is specifically about starting with a new profile, then I think its basically a dup of bug 1547139.

Comment 4

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

Thanks! Lets mark as dupe then.

Comment 5

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

(In reply to Shane Caraveo (:mixedpuppy) from bug 1547139 comment #12)

I'd move this to another bug.

Sure, no worries about that.

We might be able to do something to allow turning it off for automation. Do you know if Cu.isInAutomation is true when running under selenium/puppeteer? If so, you could hack in a check[1] on that to see if those test run fine and the perf improvement is seen, something like

If that does work for you, I'd probably recommend something involving a pref in combination with isInAutomation added to AddonSettings.jsm, where by default this will continue to run, but you can pref off for sets of tests.

[1] https://searchfox.org/mozilla-central/rev/31a3457890b5698af1277413ee9d9bd6c5955183/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2952

No, this will not work because Cu.isInAutomation is only true when (AFAIK) test jobs are run on TaskCluster CI, and anything around Selenium and Puppeteer is outside of our infrastructure.

Would making it also dependent on nsIMarionette.running and RemoteAgent.listening (as what we do in browser.js) work?

Comment 6

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

Here the link to the updater code that already has such a check:
https://searchfox.org/mozilla-central/rev/644e42ded761d4f3ce108fa776197730a9a2c535/toolkit/mozapps/update/UpdateService.jsm#3576-3589

Comment 7

[Shane Caraveo (:mixedpuppy)]

That looks like a reasonable approach (something close to what is in update service). I'm not clear about the use of RemoteAgent.listening, IIUC that is the remote debugger protocol. Do we really want to disable all the time for that? Some places also check an environment variable for xpcshell, I'm tempted to add that here, but I also don't really like that mechanism.

  get isInTestingMode() {
    let marionetteRunning = false;

    if ("nsIMarionette" in Ci) {
      marionetteRunning = Cc["@mozilla.org/remote/marionette;1"].createInstance(
        Ci.nsIMarionette
      ).running;
    }

    return (Cu.isInAutomation || marionetteRunning);
  }
  get disableForTestingMode() {
    return ((isInTestingMode || RemoteAgent.listening) &&
      Services.prefs.getBoolPref("extensions.disableForTestingMode", false)
    );
  }

Since this requires a pref check anyway, a simpler approach may be to add a extensions.skipEarlyStartupScan pref, and just check that where we call scan.

if (Services.prefs.getBoolPref("extensions.skipEarlyStartupScan", false)) {
  this.checkForChanges(aAppChanged, aOldAppVersion, aOldPlatformVersion);
}

Comment 8

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)

That looks like a reasonable approach (something close to what is in update service). I'm not clear about the use of RemoteAgent.listening, IIUC that is the remote debugger protocol. Do we really want to disable all the time for that?

No it's not the Firefox Remote Protocol as used by DevTools. It's the protocol used for our partial CDP implementation (utilized by Puppeteer), and which will serve the upcoming WebDriver BiDi implementation. I landed a patch today for force disabling updates that adds a check for the Remote Agent. See bug 1695639. As such we should have similar code.

  get isInTestingMode() {
    let marionetteRunning = false;

    if ("nsIMarionette" in Ci) {
      marionetteRunning = Cc["@mozilla.org/remote/marionette;1"].createInstance(
        Ci.nsIMarionette
      ).running;
    }

    return (Cu.isInAutomation || marionetteRunning);
  }
  get disableForTestingMode() {
    return ((isInTestingMode || RemoteAgent.listening) &&
      Services.prefs.getBoolPref("extensions.disableForTestingMode", false)
    );
  }

I wonder if there is a common module where we could actually share this code. Maybe it would be useful for even other components in the future.

Since this requires a pref check anyway, a simpler approach may be to add a extensions.skipEarlyStartupScan pref, and just check that where we call scan.

So you are saying that we should not bind the preference to when automation is enabled? It would allow users to simply turn it off. Which implications would that have?

Looks like the Addon Manager component is a better fit for this bug.

Comment 9

[Florian Quèze [:florian]]

The add-on manager startup cost is also significant when opening the Browser Toolbox: https://share.firefox.dev/3bNSZgw (I tried to hack around this in bug 1555377, but that hasn't landed).

Comment 10

[Shane Caraveo (:mixedpuppy)]

ISTM that isInTestingMode should go somewhere generic, maybe even just like Cu.isInAutomation, perhaps Cu.isInTestingMode, or Cu.testingMode that is an enum so you could even know that marionette is running.

disableForTestingMode could probably just be added in AddonSettings.jsm

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

We briefly discussed about this issue with Shane, and there are a couple of corner case that we may want to take into account (to avoid entering into some unexpected inconsistent state):

• marionette or selenium explicitly configured to run on an existing profile (which may have been created with an older browser version and we may have to scan for changes at startup to detect the updated system addons)
• marionette or selenium configured for sideloading addons (which may also require scan for changes to detect them)