Closed
Bug 923581
Opened 11 years ago
Closed 9 years ago
Distribution: race between extension extraction and extension discovery
Categories
(Firefox for Android Graveyard :: Add-on Manager, defect)
Tracking
(firefox41 fixed, fennec+)
RESOLVED
FIXED
Firefox 41
People
(Reporter: jchen, Assigned: Margaret)
References
Details
Attachments
(1 file, 1 obsolete file)
5.34 KB,
patch
|
mossop
:
review+
rnewman
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
Assignee | ||
Comment 1•10 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.
Comment 2•10 years ago
|
||
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)
Updated•10 years ago
|
Blocks: downloaddistro
Updated•10 years ago
|
Flags: needinfo?(irving)
Flags: needinfo?(dtownsend+bugmail)
Flags: needinfo?(bmcbride)
Comment 3•10 years ago
|
||
(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)
Comment 5•10 years ago
|
||
Not a P5: relevant to distributions and partnerships. Blocked on AddonManager code.
Assignee: rnewman → nobody
Priority: P5 → --
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 6•9 years ago
|
||
This is mostly inspired by XPIProvider#installDistributionAddons.
Attachment #8604964 -
Flags: review?(rnewman)
Attachment #8604964 -
Flags: review?(dtownsend)
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Component: General → Add-on Manager
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2295894af36b
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2295894af36b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•