Bug 286034 (eminstall)

Support Extension In/Uninstallation by simply adding/removing extension dir

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: bugs, Assigned: bugs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 54 obsolete attachments)

209.94 KB, patch
Details | Diff | Splinter Review
4.31 KB, text/plain
Details
11.73 KB, text/plain
Details
470.15 KB, patch
darin.moz
: superreview+
Details | Diff | Splinter Review
743 bytes, patch
bfowler
: review+
Details | Diff | Splinter Review

Comment 2

15 years ago
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.
Posted patch more progress (obsolete) — Splinter Review
Attachment #177559 - Attachment is obsolete: true
Posted patch more more progress (obsolete) — Splinter Review
far from even compiling without errors.
Attachment #177590 - Attachment is obsolete: true
Posted patch more progress, does not work (obsolete) — Splinter Review
Attachment #177674 - Attachment is obsolete: true
Posted patch progres... (obsolete) — Splinter Review
Attachment #177719 - Attachment is obsolete: true
Posted patch more progress... (obsolete) — Splinter Review
Attachment #177765 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #177768 - Attachment is obsolete: true

Updated

15 years ago
QA Contact: bugs → benjamin
Posted patch more progress... (obsolete) — Splinter Review
Attachment #177787 - Attachment is obsolete: true
Posted patch more (obsolete) — Splinter Review
Attachment #177950 - Attachment is obsolete: true
Posted patch more progress... (obsolete) — Splinter Review
Attachment #177976 - Attachment is obsolete: true
Attachment #178001 - Attachment description: patch → more progress...
Posted patch more progress... (obsolete) — Splinter Review
Attachment #178001 - Attachment is obsolete: true
Posted patch more progress... (obsolete) — Splinter Review
Attachment #178007 - Attachment is obsolete: true
Posted patch still not done... (obsolete) — Splinter Review
Attachment #178021 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
Attachment #178183 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178185 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178216 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178265 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178279 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
still not done, however:

- installation by adding/removing directory works
- installation by adding/removing link text file (and target) works.
Attachment #178340 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178347 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
... now supports installation/uninstallation from web.
Attachment #177461 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
more documentation and tidy up...
Attachment #178403 - Attachment is obsolete: true
Attachment #178449 - Attachment is obsolete: true
Posted patch more... (documentation) (obsolete) — Splinter Review
Attachment #178451 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178515 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178548 - Attachment is obsolete: true
Attachment #178617 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #178685 - Attachment is obsolete: true
Posted patch more.... update logging.... (obsolete) — Splinter Review
Attachment #178695 - Attachment is obsolete: true
Posted patch more... location priority... (obsolete) — Splinter Review
Attachment #178756 - Attachment is obsolete: true

Updated

14 years ago
Blocks: 248298

Comment 35

14 years ago
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

Posted patch more... (not working!) (obsolete) — Splinter Review
Attachment #178988 - Attachment is obsolete: true
Posted patch closer to working... (obsolete) — Splinter Review
Attachment #179201 - Attachment is obsolete: true
Posted patch more. (obsolete) — Splinter Review
Attachment #179432 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #179617 - Attachment is obsolete: true
Posted patch bah. getting closer (obsolete) — Splinter Review
Attachment #179792 - Attachment is obsolete: true

Updated

14 years ago
Blocks: 272644

Updated

14 years ago
Blocks: 272653

Comment 46

14 years ago
> 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

14 years ago
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.
Posted patch fixing various cases... (obsolete) — Splinter Review
Attachment #180123 - Attachment is obsolete: true
Posted patch more bugfixin' (obsolete) — Splinter Review
Attachment #180418 - Attachment is obsolete: true
Posted patch WORKS! w00t. (obsolete) — Splinter Review
...some user error detection required, then ready for review.
Attachment #180424 - Attachment is obsolete: true
Posted patch closer... (obsolete) — Splinter Review
Attachment #180527 - Attachment is obsolete: true
Posted patch more... (obsolete) — Splinter Review
Attachment #180659 - Attachment is obsolete: true

Updated

14 years ago
Blocks: 265859
Alias: eminstall

Updated

14 years ago
Blocks: 291162
Posted patch ok (obsolete) — Splinter Review
Attachment #181093 - Attachment is obsolete: true
Posted patch final patch (obsolete) — Splinter 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
Attachment #181303 - Attachment is obsolete: true
Attachment #181346 - Flags: superreview?(darin)
Attachment #181346 - Flags: review?(benjamin)

Comment 58

14 years ago
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 60

14 years ago
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

14 years ago
*** 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

14 years ago
Comment on attachment 181346 [details] [diff] [review]
final patch

Marking r- mainly because of the relative/persistent descriptor issues.
Attachment #181346 - Flags: review?(benjamin) → review-
Attachment #181346 - Attachment is obsolete: true
Attachment #181555 - Flags: superreview?(darin)
Attachment #181555 - Flags: review?(benjamin)

Comment 64

14 years ago
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).
Attachment #181555 - Flags: review?(benjamin) → review+
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=291515 for better handling
of locked/hidden
Status: NEW → ASSIGNED

Comment 66

14 years ago
No major problems.  Looks really good overall.
Attachment #181593 - Flags: approval-aviary1.1a?

Comment 68

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

sr=darin
Attachment #181593 - Flags: superreview+

Updated

14 years ago
Attachment #181555 - Flags: superreview?(darin)

Updated

14 years ago
Attachment #181346 - Flags: superreview?(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
Attachment #181593 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
landed 1.8b2/1.1a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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>
Posted patch followup patch (obsolete) — Splinter Review
This addresses the two issues in my previous comment
Attachment #181601 - Flags: superreview?(bugs)
Attachment #181601 - Flags: review?(bugs)

Comment 73

14 years ago
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

Comment 76

14 years ago
(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)
Attachment #181601 - Attachment is obsolete: true
Attachment #181601 - Flags: superreview?(bugs)
Attachment #181601 - Flags: review?(bugs)

Updated

14 years ago
Blocks: 291709

Updated

14 years ago
Blocks: 291572

Updated

14 years ago
Blocks: 291673

Updated

14 years ago
Blocks: 291666

Updated

14 years ago
Blocks: 291675

Updated

14 years ago
Blocks: 291582

Updated

14 years ago
Blocks: 291726

Updated

14 years ago
Blocks: 291790

Updated

14 years ago
Blocks: 291791

Updated

14 years ago
Blocks: 291792

Updated

14 years ago
No longer blocks: 291162

Updated

14 years ago
Blocks: 291807

Updated

14 years ago
Blocks: 291808

Comment 80

14 years ago
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

Updated

14 years ago
Blocks: 291836

Updated

14 years ago
Depends on: 291946
No longer depends on: 291946
Flags: blocking-aviary1.1?

Updated

14 years ago
Blocks: 291946

Updated

14 years ago
Attachment #182049 - Flags: review+

Updated

14 years ago
Blocks: 292248

Updated

14 years ago
Flags: blocking-aviary1.1?
So why the
  _makeFactory: #1= function(ctor) {
and
               factory    : #1#(ExtensionManager)
instead of
               factory    : _makeFactory(ExtensionManager)
?

Comment 83

14 years ago
*** Bug 252303 has been marked as a duplicate of this bug. ***
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.