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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Is it something we wanna have for 1.9.3 final?

Comment 2

[Dave Townsend [:mossop]]

I think Benjamin said we needed this for Fx4. This may well fix bug 567612

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: 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.

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Benjamin Smedberg]

That would mean that extensions couldn't observer profile-do-change (only profile-after-change). I guess that might be ok.

Comment 6

[Dave Townsend [:mossop]]

(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.

Comment 7

[Benedict Hsieh [:benedict]]

Attachment: starts extension manager before profile-do-change

starts extension manager with new 'em-start' notification, before profile-do-change

Comment 8

[Benedict Hsieh [:benedict]]

Attachment: starts extension manager after profile-do-change, before profile-after change

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

Comment 9

[Benedict Hsieh [:benedict]]

Attachment: 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.

Comment 10

[Benjamin Smedberg]

Any reason we can't go ahead and get this reviewed while you're testing?

Comment 11

[Benedict Hsieh [:benedict]]

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

browser-chrome tests look good.

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Benedict Hsieh [:benedict]]

Attachment: 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.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 450421 [details] [diff] [review]
fixes review comments.

Looks good

Comment 15

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/ee457bad4c8f

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Is there anything we can have tests for? Whether automated or manual?

Comment 17

[Dave Townsend [:mossop]]

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.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Haven't discovered any problems since bug 568691 has been landed. There is no EM restart anymore. Lets mark it as verified fixed.

Comment 19

[Dave Townsend [:mossop]]

Per comment 17 we're doing as good as we can here I think