286034 - (eminstall) Support Extension In/Uninstallation by simply adding/removing extension dir

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: progress, unfinished - does not work

Comment 2

[Ian Pottinger]

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.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: works! but limited features, more coming...

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: definitely does not work, progress.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more more progress

far from even compiling without errors.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress, does not work

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: progres...

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more progress...

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: still not done...

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

still not done, however:

- installation by adding/removing directory works
- installation by adding/removing link text file (and target) works.

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

... now supports installation/uninstallation from web.

Comment 25

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

more documentation and tidy up...

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... (documentation)

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more.... upgrading now works yay

Comment 30

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... (patch no longer works, untested - datasource tidy up)

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more, does not work (got displayDescription to work better)

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more.... update logging....

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... location priority...

Comment 35

[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

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... (not working!)

Comment 37

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more (also not working... fixes template builder inconsistency...)

Comment 38

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: closer to working...

Comment 39

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: nah.... let's redo the startup install finalization process again!

Comment 40

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more.

Comment 41

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 42

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: bah. getting closer

Comment 43

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... very close to being done now! prioritization sort of works

Comment 44

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: veeery veeery close... more testing needed...

Comment 45

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more... show confirmation UI when handling dropped files

Comment 46

[Darin Fisher]

> 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 47

[Steffen Wilberg]

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.

Comment 48

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: fixing various cases...

Comment 49

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more bugfixin'

Comment 50

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: WORKS! w00t.

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

Comment 51

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: getting themes to work... almost done...

Comment 52

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: closer...

Comment 53

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 54

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: pretty much done, needs last sanity check...

Comment 55

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: ok

Comment 56

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: final patch

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

Comment 57

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Full EM file, for ease of reading

Comment 58

[Benjamin Smedberg]

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.

Comment 59

[Darin Fisher]

Attachment: Some initial review comments from Darin (up to nsIExtensionDownloadProgressListener)

Comment 60

[Benjamin Smedberg]

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.

Comment 61

[Benjamin Smedberg]

*** 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 62

[Benjamin Smedberg]

Comment on attachment 181346 [details] [diff] [review]
final patch

Marking r- mainly because of the relative/persistent descriptor issues.

Comment 63

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: address darin/bsmedberg's review comments

Comment 64

[Benjamin Smedberg]

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

Comment 65

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 66

[Darin Fisher]

Attachment: Final review comments from Darin

No major problems.  Looks really good overall.

Comment 67

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: address darin's comments

Comment 68

[Darin Fisher]

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

sr=darin

Comment 69

[Brendan Eich [:brendan]]

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

Comment 70

[Ben Goodger (use ben at mozilla dot org for email)]

landed 1.8b2/1.1a

Comment 71

[Robert Strong (they/them - no direct email)]

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>

Comment 72

[Robert Strong (they/them - no direct email)]

Attachment: followup patch

This addresses the two issues in my previous comment

Comment 73

[Anbo Motohiko]

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);

Comment 74

[Robert Strong (they/them - no direct email)]

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

Comment 75

[Robert Strong (they/them - no direct email)]

this caused bug 291572

Comment 76

[Anbo Motohiko]

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

Comment 77

[Michiel van Leeuwen (email: mvl+moz@)]

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)

Comment 78

[Robert Strong (they/them - no direct email)]

this caused bug 291673 and bug 291666

Comment 79

[Robert Strong (they/them - no direct email)]

Also caused bug 291675

Comment 80

[Scott MacGregor]

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

Comment 81

[Ben Fowler]

Attachment: Patch for typo

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

which cause a compiler warning. Trivial.

Comment 82

[[e-mail]]

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

Comment 83

[Benjamin Smedberg]

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