See http://wiki.mozilla.org/wiki/Firefox:1.1_Extension_Manager_Upgrades

From the wiki: >Ability to shut down extensions remotely using a blacklist that we control. >Client checks in periodically for additions and if it finds one, disables the >extension. Firefox should ask the user if the extension should be disabled (default "YES") and give a brief explaination. It would really pissed me off if, after wondering why an extension was not working, I found it had been disable by someone else who believes he knows better than me what I want or need.

far from even compiling without errors.

still not done, however: - installation by adding/removing directory works - installation by adding/removing link text file (and target) works.

... now supports installation/uninstallation from web.

more documentation and tidy up...

As per my added "comment" on the wiki discussion, I very much welcome the facility to do a clean extension install/uninstall but also very much hope the implementation will not prevent the continued use of the very simple but very reliable "copy profile, test new feature, delete profile, copy back profile" secure and rollback end-user recovery technique. Best regards, RDL

> As per my added "comment" on the wiki discussion, I very much welcome the > facility to do a clean extension install/uninstall but also very much hope the > implementation will not prevent the continued use of the very simple but very > reliable "copy profile, test new feature, delete profile, copy back profile" > secure and rollback end-user recovery technique. Best regards, RDL That technique should be unaffected by this patch. It will still be the case that all extension manager state will be recorded in the profile directory.

Comment on attachment 180123 [details] [diff] [review] more... show confirmation UI when handling dropped files >Index: browser/locales/en-US/searchplugins/list.txt >=================================================================== >-dictionary >+answers The to-be-introduced answers.com searchplugin is not part of this patch.

...some user error detection required, then ready for review.

Index: browser/app/profile/firefox.js - increment extensions version number - default prefs Index: browser/app/profile/extensions/Extensions.rdf - unified item root container Index: browser/app/profile/extensions/Makefile.in Index: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/Makefile.in Index: browser/installer/unix/packages-static Index: browser/installer/windows/packages-static - no longer use installed-extensions.txt Index: browser/app/profile/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/install.rdf.in - update compatibility range, em:type on default theme Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd - remove unused strings - these now come from the datasource, not different XBL bindings Index: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties - add new strings for various states of item installation Index: toolkit/locales/en-US/chrome/mozapps/xpinstall/xpinstallConfirm.properties - add strings to notify better of errors Index: toolkit/mozapps/extensions/content/extensions.css Index: toolkit/mozapps/extensions/content/extensions.xml - no longer a plethora of XBL bindings for different install states, or item types, mode is just toggled on single binding. Index: toolkit/mozapps/extensions/content/extensions.js - single item rdf container - update for API changes - update display description of items more reliably when theme switches - improvement to command controller, ensuring various functions aren't available when item is in newborn (not installed yet) state Index: toolkit/mozapps/extensions/content/extensions.xul - new content generation template to handle single item root container, content built depending on type literal - allow for hidden items Index: toolkit/mozapps/extensions/public/nsIExtensionManager.idl - api changes to EM - install location interface - extension state change notifications (installed, uninstalled, etc) Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in - most of the changes, see http://www.mozilla.org/projects/firefox/extensions/em-changes.html Index: toolkit/mozapps/shared/content/richview.xml - allow for hidden items in the extensions/themes view - make sure they don't show up in key navigation Index: toolkit/mozapps/update/content/errors.xul - log errors better during extension update Index: toolkit/mozapps/update/content/update.js - EM API changes - log errors better during extension update Index: toolkit/mozapps/update/public/nsIUpdateService.idl - add location key to nsIUpdateItem - make item types long instead of short to allow for more update item types Index: toolkit/mozapps/update/src/nsUpdateService.js.in - fix implementation of UpdateItem to correspond to interface changes - simplify version checker "valid version?" test using regexp Index: toolkit/mozapps/xpinstall/content/xpinstallConfirm.js - allow for confirm install dialog to notify user of dropped in xpis Index: toolkit/xre/nsAppRunner.cpp - remove EM register call, since all data is now stored in user profile directory running first with -register it is no longer required. - remove support for lock/unlock etc command line flags Index: toolkit/xre/nsXREDirProvider.cpp - make extensions.ini live only in profile directory (and make paths in extensions.ini be all absolute rather than relative) - make extensions.ini not use a redundant Count variable (error check result instead) Index: xpinstall/src/nsSoftwareUpdateRun.cpp - change XPInstall call sites to use new EM function names

This is going to have to be a multi-part review to keep me sane. Some of it is nit-picky and some is architectural, all mixed up. nsExtensionManager.js.in: getFile() - The comment is missing a phrase (directories created along the way?) ensureExtensionsFiles() - remove the obsolete comment about contents.rdf/overlayinfo stackTraceFunctionFormat() and stackTrace() - these don't seem to be called... why are they here? moveDirectory() - Why can't we move in one operation? Darin's probably the expert here, I defer completely to him. baby's awake, more later.

In the StartupCache, we're using persistentdescriptors. What happens when the profile changes filepaths (as happens on windows roaming profiles with some frequency)? This is why I used the relative-descriptor-or-persistent-descriptor duality in profiles.ini and other places, to avoid hardcoding absolute paths that might change for roaming profiles. In the nsExtensionManager() constructor: When you initialize gPref, why not just ask for nsIPrefBranch2 off the bat? That way you can avoid the "instanceof". The interfaces inherit. In nsExtensionManager._profileSelected: You don't need to wait for the profile-after-change notification to do most of this work. I coded the "ProfDS" directoryservice key a while back, which indicates the profile path even before the profile has been "started". In nsExtensionManager._shutdown Do you need to test gPref before you call .removeObserver on it? Same question for gRDF. I think the safe-mode stuff is too complicated, but I'm willing to let it land like this and I'll fix it after 1.1a. Basically, I think that safe-mode should be detected by the apprunner and set a flag on nsIXULAppInfo. In that case, it won't even bother asking the EM for extensions, and the chrome registry can handle the default-skin bits. In nsExtensionManager.handleCommandLineArgs: I'm not following the exact startup sequence here, but I'm pretty sure that we should be setting .preventDefault on the commandline so that we don't open a browser window (or whatever) after a call to -install-global-* In nsExtensionManager._checkForGlobalInstalls: What type/format is the "path" arg? What is the #ifdef XP_UNIX bit for? We should probably be using the nsICommandLine.resolveFile method to do this work, since it has been relatively well-tested ;-). In nsExtensionManager._upgradeExtensionChrome: "needsRestart" is misleading. We don't actually need to restart the entire app if all we're doing is upgrading to flat chrome manifests. Just call void nsIChromeRegistry.checkForNewChrome() and continue. Is the _showProgressWindow() hack necessary? If so, do you remember why? Chrome registry certainly shouldn't care any more. Regarding ds.addItemMetadata() I think that we should disallow "locked" on everything except the default theme and whatever extension we ship in the installer that actually can't be turned off. I, for one, want to disable the Java Console extension if/when it ships with the JRE, and I can imagine other such integration hooks that I would prefer didn't clutter up my UI. in gModule, what's this bit about _firstTime and FACTORY_REGISTER_AGAIN? I don't see why it's needed.

*** firefox.js: Do we really want to bump extensions.version to 1.1 now? What about 1.0+? I would save 1.1 for 1.1b or even the first release candidate. ignoreMTimeChanges: We're really going to need documentation about all the EM prefs, whether they are required, etc. Ideally, the EM would continue to function with no prefs at all (try/catch around pref calls with appropriate default values). Since this will be part of the "Toolkit API", we should go ahead and create a wiki page. I'll do some additional logging work later: basically, error messages should be logged no matter what (to the JS console), but success messages can depend on a pref or something. *** Extensions.rdf I think that we can avoid the pre-built mostly-empty Extensions.rdf, which will make xulrunner lives a lot happier. I'll have a patch for that during 1.1b as well. *** install.rdf (default theme) What is <em:type>? Is it a new/required part of install.rdf? Documentation please! *** nsIExtensionManager.idl: Remove @UNDER_REVIEW from the interfaces... I don't see an immediate need to freeze the interfaces, as they are not really part of the "public API". What darin said about avoiding "string"... let's stick to the AString or AUTF8String types for new interfaces. nsXREDirProvider.cpp: LoadDirsIntoArray() Same issues as above with persistent/relative descriptor... perhaps we need nsILocalFile3 with a "RelativeOrPersistentDescriptor" method, to make this properly generic? Replace: if (NS_SUCCEEDED()) { ... } else break; With: if (NS_FAILED()) break; OK, I'm done with this review.

Comment on attachment 181346 [details] [diff] [review] final patch Marking r- mainly because of the relative/persistent descriptor issues.

Comment on attachment 181555 [details] [diff] [review] address darin/bsmedberg's review comments RCS file: /cvsroot/mozilla/toolkit/xre/nsXREDirProvider.cpp,v LoadDirsIntoArray(nsIFile* aComponentsList, const char* aSection, + char buf[18]; + PRInt32 i = 0; + do { + sprintf(buf, "Extension%d", i++); + + rv = parser.GetString(aSection, buf, parserBuf, MAXPATHLEN); + if (NS_SUCCEEDED(rv)) { Make this a NS_FAILED(rv) break; early. + nsCOMPtr<nsILocalFile> dir(do_CreateInstance("@mozilla.org/file/local;1")); + rv = dir->SetPersistentDescriptor(nsDependentCString(parserBuf)); + if (NS_FAILED(rv)) { + // Must be a relative descriptor, relative to the profile directory, + // try that instead. + nsCOMPtr<nsIFile> profileDir; + aComponentsList->GetParent(getter_AddRefs(profileDir)); + nsCOMPtr<nsILocalFile> lfProfileDir(do_QueryInterface(profileDir)); + dir->SetRelativeDescriptor(lfProfileDir, nsDependentCString(parserBuf)); Can we rv-check this call and "continue;" if it failed (with appropriate NS_WARNING or somesuch).

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=291515 for better handling of locked/hidden

No major problems. Looks really good overall.

Comment on attachment 181593 [details] [diff] [review] address darin's comments sr=darin

Comment on attachment 181593 [details] [diff] [review] address darin's comments That's the biggest patch I've seen that didn't overflow the bugzilla limits. a=me with some try/catch/finally symmetry. /be

landed 1.8b2/1.1a

The checkin included nsBrowserApp.cpp which wasn't part of the patch and it has the following where "Firefox Debug" should just be "Firefox" static const nsXREAppData kAppData = { "Mozilla", "Firefox Debug", Also, the patch no longer included +pref("app.extensions.version", "1.1"); but it still included + <em:minVersion>1.1</em:minVersion> + <em:maxVersion>1.1</em:maxVersion>

This addresses the two issues in my previous comment

Ben, After this, will the version string "0.10" not be allowed? This breaks some extensions, like All-in-One gestures. Current version of AiO is "0.14.1", and it cannot be used with beast-trunk builds. In isValidVersion() function, the regexp should be: + return /^([0-9]+\.){1,3}[0-9]+\+?$/.test(aVersion); instead of: + return /^([0-9]\.){1,3}[0-9]\+?$/.test(aVersion);

There is still a reference to addDownloadObserver in extensions.js that needs to be renamed to addDownloadListener

this caused bug 291572

(In reply to comment #73) I've filed: Bug 291582.

Since this landed, Thunderbird no longer starts. See tinderbox. i see this error: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.getBoolPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: file:///home/michiel/mozhack/tree5/obj-lightning/dist/bin/components/nsExtensionManager.js :: ExtensionManager :: line 1776" data: no] ************************************************************ (And i suspect that every other non-firefox app, like sunbird, also will no longer start)

this caused bug 291673 and bug 291666

Also caused bug 291675

hey ben, I can't get thunderbird to run anymore since I updated with these changes. Looks like the tinderbox tree is orange as well. See Bug #291836

There is a typo at http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/public/nsIExtensionManager.idl#352 which cause a compiler warning. Trivial.

So why the _makeFactory: #1= function(ctor) { and factory : #1#(ExtensionManager) instead of factory : _makeFactory(ExtensionManager) ?

*** Bug 252303 has been marked as a duplicate of this bug. ***