Distribution: race between extension extraction and extension discovery

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jchen, Assigned: Margaret)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, fennec+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
For distributions, we extract extensions here on the background thread, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Distribution.java#95

But distribution extensions are discovered and installed here on the Gecko thread during startup, http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#2374

If the extensions are extracted before the Gecko thread code runs, the extensions will be installed. Otherwise, the extensions are not installed.

I noticed this issue when making the SummitFox distro. To workaround it, I ended up editing the apk to put the extensions at the beginning, so that they get extracted first.
tracking-fennec: --- → ?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
(Assignee)

Comment 1

4 years ago
I haven't had time to address this, but adding to rnewman's radar in case it's something he's come across.
Well, that's a concern. Thanks for the headsup, margaret. I'm gonna yoik this, assuming you won't mind :)



As far as I can tell, we can prompt for distribution add-ons to be loaded explicitly: 

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2560

so we could make that a post-distribution Gecko task, analogous to our loading of bookmarks.

Mossop or Irving, whoever gets to this first:

We're going to be fetching distributions from the network on first run, so they'll typically be extracted after a couple of seconds. Sometimes that'll take long enough that Gecko has laboriously hauled itself to the starting line and begun to initialize.

Is it harmful for XPIProvider.installDistributionAddons to be called independently of XPIProvider's own startup?

Is it harmful for it to be called before, during, or after XPIProvider's own startup?

Is it harmful for it to be called twice?

Is there a better solution for this problem?
Assignee: margaret.leibovic → rnewman
Flags: needinfo?(irving)
Flags: needinfo?(dtownsend+bugmail)
Blocks: 1013024
Flags: needinfo?(irving)
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(bmcbride)
(In reply to Richard Newman [:rnewman] from comment #2)
> Is it harmful for XPIProvider.installDistributionAddons to be called
> independently of XPIProvider's own startup?

It's not *harmful*. But it won't exactly work as you want, either. It'll move the distro add-ons into the profile, but they won't be loaded. And on restart, they'll be detected as foreign install (on Android they won't get auto-disabled like on desktop, but we still don't want them marked as a foreign install).


> Is it harmful for it to be called before, during, or after XPIProvider's own
> startup?

Beyond when it's already automatically called, it's only safe to call *after* startup. But, XPIProvider's startup is synchronous anyway, so you don't have to worry about calling it during that process.


> Is it harmful for it to be called twice?

No.


> Is there a better solution for this problem?

I think so! Bug 879480 will implement XPIProvider.addInstalledAddon(). This will lets add-ons be dropped into an install location during runtime (which is what installDistributionAddons() does) and install it similar to how we detect such installs on startup - including automatically start restartless extensions. 

The only modification needed to the code implemented in bug 879480 would be to detect if the add-on file was in the distribution directory, and call installDistributionAddons() automatically in that case (perhaps with some refactoring to allow it to act on a single file rather than the entire directory). Then we'd just need code to hook up calling AddonManager.addInstallAddon() when a distro add-on is put in the right place during rumtime.

I'll see about getting the ball rolling on that bug again, since I haven't been able to come up with an alternate solution that feels comfortable. IIRC, it was relatively close to finished.
Flags: needinfo?(bmcbride)
filter on [mass-p5]
Priority: -- → P5
Not a P5: relevant to distributions and partnerships.

Blocked on AddonManager code.
Assignee: rnewman → nobody
Priority: P5 → --
(Assignee)

Updated

3 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 6

3 years ago
Created attachment 8604964 [details] [diff] [review]
Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

This is mostly inspired by XPIProvider#installDistributionAddons.
Attachment #8604964 - Flags: review?(rnewman)
Attachment #8604964 - Flags: review?(dtownsend)
Comment on attachment 8604964 [details] [diff] [review]
Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

Review of attachment 8604964 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +7804,5 @@
>        }
>      });
> +  },
> +
> +  installDistroAddons: Task.async(function* () {

Presumably all this can run asynchronously, so lets use OS.File in here to avoid the main thread IO

@@ +7845,5 @@
> +        let id = name.substring(0, name.length - 4);
> +        if (install.addon.id != id) {
> +          Cu.reportError("File entry " + entry.path + " contains an add-on with an incorrect ID");
> +          continue;
> +        }

Kind of up to you if you want to keep this restriction, but it probably makes sense
Attachment #8604964 - Flags: review?(dtownsend) → review-
Comment on attachment 8604964 [details] [diff] [review]
Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

Review of attachment 8604964 [details] [diff] [review]:
-----------------------------------------------------------------

Aside from OS.File, I'm also going to poke around in IntelliJ to make sure Distribution:Set is the right message. (We might also want to watch for Distribution:Changed, or only :Changed.)
Comment on attachment 8604964 [details] [diff] [review]
Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

Review of attachment 8604964 [details] [diff] [review]:
-----------------------------------------------------------------

The hook parts lgtm.

::: mobile/android/chrome/content/browser.js
@@ +7831,5 @@
> +    let entry;
> +    while ((entry = entries.nextFile)) {
> +      // Only support extensions that are zipped in .xpi files.
> +      let name = entry.leafName;
> +      if (!entry.isFile() || name.substring(name.length - 4).toLowerCase() !== ".xpi") {

If you're willing to exclude foo.XPI (which is fine by me), then

  !name.endsWith(".xpi")
Attachment #8604964 - Flags: review?(rnewman)
(Assignee)

Comment 10

3 years ago
Created attachment 8605448 [details] [diff] [review]
(v2) Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

I also updated this to avoid showing a notification after installing the distribution add-ons.

I'll file a follow-up bug to investigate fixing our add-on install observer to only observe add-on installs from the web.
Attachment #8604964 - Attachment is obsolete: true
Attachment #8605448 - Flags: review?(rnewman)
Attachment #8605448 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Blocks: 1162531
Comment on attachment 8605448 [details] [diff] [review]
(v2) Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

Review of attachment 8605448 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +6250,5 @@
> +    // Ignore distribution add-ons.
> +    if (Distribution.pendingAddonInstalls.has(aInstall)) {
> +      Distribution.pendingAddonInstalls.delete(aInstall);
> +      return;
> +    }

You probably want this in onInstallFailed too

@@ +7842,5 @@
> +    }
> +
> +    let it = new OS.File.DirectoryIterator(distroPath);
> +    try {
> +      yield it.forEach((entry, index, it) => {

You don't use index or it here so just entry => { works

@@ +7860,5 @@
> +          }
> +          this.pendingAddonInstalls.add(install);
> +          install.install();
> +        }).catch(e => {
> +          Cu.reportError("Error installing distribution add-on: " + e);

You might want to include the path here too
Attachment #8605448 - Flags: review?(dtownsend) → review+
Comment on attachment 8605448 [details] [diff] [review]
(v2) Install distro add-ons after Distribution:Set to avoid race condition between distribution and XPIProvider

Review of attachment 8605448 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +7810,5 @@
>        }
>      });
> +  },
> +
> +  pendingAddonInstalls: new Set(),

Worth commenting here that we *only* track pending installs so that we can short-circuit the onInstallEnded handler, and thus not show prompts, and that this approach rests on the assumption that all distro add-ons are restartless or don't care about restart prompts.

It'd be convenient if AddonInstall had a "silent" flag so we didn't have to keep track of this ourselves, but hey.

@@ +7830,5 @@
> +    } catch (e) {
> +      return;
> +    }
> +
> +    let exists = yield OS.File.exists(distroPath);

Can you get rid of this and just use the error case of OS.File.stat? Might as well only hit the filesystem once.
Attachment #8605448 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
Component: General → Add-on Manager
(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8605448 [details] [diff] [review]
> (v2) Install distro add-ons after Distribution:Set to avoid race condition
> between distribution and XPIProvider
> 
> Review of attachment 8605448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +7810,5 @@
> >        }
> >      });
> > +  },
> > +
> > +  pendingAddonInstalls: new Set(),
> 
> Worth commenting here that we *only* track pending installs so that we can
> short-circuit the onInstallEnded handler, and thus not show prompts, and
> that this approach rests on the assumption that all distro add-ons are
> restartless or don't care about restart prompts.
> 
> It'd be convenient if AddonInstall had a "silent" flag so we didn't have to
> keep track of this ourselves, but hey.

The intention is for all installs to be silent by default and apps to handle their own notification of installs they start (using addListener on the install object) and to use the observer notifications instead for web installs.
https://hg.mozilla.org/mozilla-central/rev/2295894af36b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.