Last Comment Bug 568348 - The extension manager should do extension install activities and write extensions.ini before the profile is started
: The extension manager should do extension install activities and write extens...
Status: VERIFIED FIXED
[AddonsRewrite][AOMTestday]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla2.0b1
Assigned To: Benedict Hsieh [:benedict]
:
Mentors:
Depends on:
Blocks: 567612
  Show dependency treegraph
 
Reported: 2010-05-26 15:00 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2011-03-30 10:21 PDT (History)
3 users (show)
dtownsend: in‑testsuite+
hskupin: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
work in progress (6.44 KB, patch)
2010-06-02 15:27 PDT, Benedict Hsieh [:benedict]
no flags Details | Diff | Review
starts extension manager before profile-do-change (13.88 KB, patch)
2010-06-08 10:57 PDT, Benedict Hsieh [:benedict]
no flags Details | Diff | Review
starts extension manager after profile-do-change, before profile-after change (2.54 KB, patch)
2010-06-09 10:56 PDT, Benedict Hsieh [:benedict]
no flags Details | Diff | Review
starts extension manager after profile-do-change, before profile-after change (4.12 KB, patch)
2010-06-09 18:38 PDT, Benedict Hsieh [:benedict]
dtownsend: review-
Details | Diff | Review
fixes review comments. (3.98 KB, patch)
2010-06-10 12:48 PDT, Benedict Hsieh [:benedict]
dtownsend: review+
Details | Diff | Review

Description Benjamin Smedberg [:bsmedberg] 2010-05-26 15:00:28 PDT
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) 2010-05-28 15:17:17 PDT
Is it something we wanna have for 1.9.3 final?
Comment 2 Dave Townsend [:mossop] 2010-06-01 10:59:48 PDT
I think Benjamin said we needed this for Fx4. This may well fix bug 567612
Comment 3 Benedict Hsieh [:benedict] 2010-06-02 15:27:12 PDT
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.
Comment 4 Dave Townsend [:mossop] 2010-06-02 15:45:39 PDT
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 [:bsmedberg] 2010-06-03 10:07:32 PDT
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] 2010-06-08 10:25:16 PDT
(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] 2010-06-08 10:57:07 PDT
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
Comment 8 Benedict Hsieh [:benedict] 2010-06-09 10:56:36 PDT
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.
Comment 9 Benedict Hsieh [:benedict] 2010-06-09 18:38:57 PDT
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.
Comment 10 Benjamin Smedberg [:bsmedberg] 2010-06-09 18:53:40 PDT
Any reason we can't go ahead and get this reviewed while you're testing?
Comment 11 Benedict Hsieh [:benedict] 2010-06-10 11:06:04 PDT
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] 2010-06-10 11:32:12 PDT
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] 2010-06-10 12:48:01 PDT
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.
Comment 14 Dave Townsend [:mossop] 2010-06-10 22:53:21 PDT
Comment on attachment 450421 [details] [diff] [review]
fixes review comments.

Looks good
Comment 16 Henrik Skupin (:whimboo) 2010-06-17 08:20:42 PDT
Is there anything we can have tests for? Whether automated or manual?
Comment 17 Dave Townsend [:mossop] 2010-06-17 08:53:45 PDT
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) 2010-12-03 18:18:27 PST
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] 2011-03-30 10:21:50 PDT
Per comment 17 we're doing as good as we can here I think

Note You need to log in before you can comment on or make changes to this bug.