The extension manager should do extension install activities and write extensions.ini before the profile is started

VERIFIED FIXED in mozilla2.0b1

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: bsmedberg, Assigned: benedict)

Tracking

Trunk
mozilla2.0b1
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [AddonsRewrite][AOMTestday])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
In preparation for getting rid of the extension manager restart, the extension manager should do extension install/uninstall activities before the profile-after-change notification.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#74 is where the extension manager currently watches for profile-after-change.

It registers for profile-after-change in the category manager here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#173

The profile change notifications are fired from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#821

We should add a new notification prior to the profile actually being notified which the extension manager can use instead.

This may also involve teaching the pref service and perhaps some other services about loading data prior to the official profile-change notifications.
Is it something we wanna have for 1.9.3 final?
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [AddonsRewrite]
Version: unspecified → Trunk
I think Benjamin said we needed this for Fx4. This may well fix bug 567612
Blocks: 567612
blocking2.0: ? → beta1+
blocking2.0: beta1+ → final+
(Assignee)

Comment 3

7 years ago
Created attachment 448871 [details] [diff] [review]
work in progress

Creates observer notification 'em-start' that is sent before mProfileNotified is flipped to true or profile-do-change is sent in nsXREDirProvider. Modified extensionManager and prefService to start at this time. 

There's still an issue (in XPIProvider.jsm, I think) preventing extensions.ini from being written. Also, right now I'm just referencing ProfDS, bsmedberg suggested that I might want to fix NS_APP_PREFS_50_DIR/FILE to work before profile-do-change.
Just a thought, but maybe it would be easier to leave profile-do-change triggering the pref service load (and anything else the EM requires on) and then adding our new notification between that and profile-after-change. The new notification need not even go through the observer service as such, you could just instantiate the EM component and call it's nsIObserver interface directly.
(Reporter)

Comment 5

7 years ago
That would mean that extensions couldn't observer profile-do-change (only profile-after-change). I guess that might be ok.
(In reply to comment #5)
> That would mean that extensions couldn't observer profile-do-change (only
> profile-after-change). I guess that might be ok.

We're already removing the option of observing xpcom-startup and app-startup from extension components by making these changes (which we probably need to announce pretty soon). Without those it would be impossible to register to receive profile-do-change anyway.

The add-ons mxr seems broken right now but I don't believe that profile-do-change will be used much anyway. profile-after-change is the one I have always heard recommended for use by extensions and I have never come across a situation where the slightly earlier event is necessary.
(Assignee)

Comment 7

7 years ago
Created attachment 449903 [details] [diff] [review]
starts extension manager before profile-do-change

starts extension manager with new 'em-start' notification, before profile-do-change
Attachment #448871 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
Created attachment 450158 [details] [diff] [review]
starts extension manager after profile-do-change, before profile-after change

Patch using Mossop's suggested approach, seems a bit simpler.
(Assignee)

Comment 9

7 years ago
Created attachment 450273 [details] [diff] [review]
starts extension manager after profile-do-change, before profile-after change

a bit simpler than the original patch, and fixes xpcshell tests. still need to look into which mochitests to run.
Attachment #450158 - Attachment is obsolete: true
(Reporter)

Comment 10

7 years ago
Any reason we can't go ahead and get this reviewed while you're testing?
(Assignee)

Comment 11

7 years ago
Comment on attachment 450273 [details] [diff] [review]
starts extension manager after profile-do-change, before profile-after change

browser-chrome tests look good.
Attachment #450273 - Flags: review?(dtownsend)
Comment on attachment 450273 [details] [diff] [review]
starts extension manager after profile-do-change, before profile-after change

>diff --git a/toolkit/mozapps/extensions/addonManager.js b/toolkit/mozapps/extensions/addonManager.js

> amManager.prototype = {
>   observe: function AMC_observe(aSubject, aTopic, aData) {
>     let os = Cc["@mozilla.org/observer-service;1"].
>              getService(Ci.nsIObserverService);
> 
>     switch (aTopic) {
>-    case "profile-after-change":
>+    case "em-start":

I think we need a better name for this, maybe addons-startup

>   classDescription: "Addons Manager",
>   contractID: "@mozilla.org/addons/integration;1",
>   classID: Components.ID("{4399533d-08d1-458c-a87a-235f74451cfa}"),
>-  _xpcom_categories: [{ category: "profile-after-change" },
>+  _xpcom_categories: [{ category: "em-start" },

Registering it the category is unnecessary now.

>                       { category: "update-timer",
>                         value: "@mozilla.org/addons/integration;1," +
>                                "getService,addon-background-update-timer," +
>                                PREF_EM_UPDATE_INTERVAL + ",86400" }],
>   _xpcom_factory: {
>     createInstance: function(aOuter, aIid) {
>       if (aOuter != null)
>         throw Cr.NS_ERROR_NO_AGGREGATION;
>diff --git a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
>@@ -134,22 +134,21 @@
>   if (gInternalManager)
>     do_throw("Test attempt to startup manager that was already started.");
> 
>   if (aAppChanged === undefined)
>     aAppChanged = true;
> 
>   // Load the add-ons list as it was during application startup
>   loadAddonsList(aAppChanged);
>-
>+  

Don't add whitespace please.

>   gInternalManager = AM_Cc["@mozilla.org/addons/integration;1"].
>                      getService(AM_Ci.nsIObserver).
>                      QueryInterface(AM_Ci.nsITimerCallback);
>-

And leave this blank line in please.

>-  gInternalManager.observe(null, "profile-after-change", "startup");
>+  gInternalManager.observe(null, "em-start", "startup");

No need to pass "startup" to it anymore.
Attachment #450273 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 13

7 years ago
Created attachment 450421 [details] [diff] [review]
fixes review comments.

>I think we need a better name for this, maybe addons-startup
Changed.
>Registering it the category is unnecessary now.
Removed.
>No need to pass "startup" to it anymore.
Removed.

Sorry about the whitespace issues, fixed those.
Attachment #450273 - Attachment is obsolete: true
Attachment #450421 - Flags: review?(dtownsend)
Comment on attachment 450421 [details] [diff] [review]
fixes review comments.

Looks good
Attachment #450421 - Flags: review?(dtownsend) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [AddonsRewrite] → [AddonsRewrite], checkin-needed
Keywords: checkin-needed
Whiteboard: [AddonsRewrite], checkin-needed → [AddonsRewrite]

Comment 15

7 years ago
http://hg.mozilla.org/mozilla-central/rev/ee457bad4c8f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Is there anything we can have tests for? Whether automated or manual?
Flags: in-testsuite?
Flags: in-litmus?
Target Milestone: --- → mozilla1.9.3a6
We can't really test this specifically. Once bug 568691 lands we'll probably have testing of this by the side effect of extensions working and there being no EM restart. I think the automated suites will just cover that as-is.
Flags: in-litmus? → in-litmus-
Haven't discovered any problems since bug 568691 has been landed. There is no EM restart anymore. Lets mark it as verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [AddonsRewrite] → [AddonsRewrite][AOMTestday]
Per comment 17 we're doing as good as we can here I think
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.