The default bug view has changed. See this FAQ.

EM gets confused between multiple installations of the same version

RESOLVED FIXED in mozilla1.8beta4

Status

()

Toolkit
Startup and Profile System
P1
major
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

unspecified
mozilla1.8beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact] has patch, needs review darin)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

12 years ago
The Extension Manager cache (extensions-startup.manifest) can get confused if
you have two Firefoxen of the same app.extensions.version installed in different
directories: it will frequently end up trying to load an extension from
A/extensions/<foo> when you launch B/firefox.

In addition, the EM will cease to work at all in some cases because it uses
nsILocalFile.[set]persistentDescriptor without a try/catch block.
[set]persistentDescriptor can and will fail on mac if the parent directory has
been removed.

I intend to solve this problem through some defense-in-depth:

1) store the last-run-from path and revalidate the startup cache if we changed paths

2) use a new form of relative descriptor in the extensions-startup cache,
similar to the form used in compreg.dat: it will be

  <dirservicekey>:<relativedescriptor> or
  abs:<persistentdescriptor>

3) Add lots of additional sanity-checking to the startup cache, and freely
invalidate it if things go wonky, and don't invalidate extensions.rdf if
extensins-startup.manifest was removed.
(Assignee)

Updated

12 years ago
Depends on: 297315
Blocks: 291807
(Assignee)

Comment 1

12 years ago
Created attachment 187969 [details] [diff] [review]
Part 1, rev. 1: make a nsIPersistentDescriptorService

This is a persistent-descriptor service that can be used to implement #2 here.
I ended up using percent-sign instead of colon because resource:app has a colon
in it and that can confuse things.
Attachment #187969 - Flags: first-review?(shaver)
(Assignee)

Comment 2

12 years ago
Need to solve this at least one way for 1.8b4.
Flags: blocking1.8b4+
Comment on attachment 187969 [details] [diff] [review]
Part 1, rev. 1: make a nsIPersistentDescriptorService

>+    /**
>+     * Get a string descriptor that represents an nsILocalFile relative
>+     * to a list of directoryservice keys specified by aCategory. If the
>+     * file is not relative to any specified directoryservice key, an absolute
>+     * descriptor will be used.
>+     */
>+    ACString getDescriptor(in nsILocalFile aFile, in string aCategory);

Where are the categories likely to be documented, or at least enumerated,
for consumers?

>+  getClassObject : function mod_gch(compMgr, cid, iid) {

"gch"?

>Index: xpcom/io/nsLocalFileWin.cpp
> NS_IMETHODIMP
> nsLocalFile::Equals(nsIFile *inFile, PRBool *_retval)
> {
>     NS_ENSURE_ARG(inFile);
>     NS_ENSURE_ARG(_retval);
> 
>     nsCAutoString inFilePath;
>     inFile->GetNativePath(inFilePath);
> 
>-    *_retval = inFilePath.Equals(mWorkingPath);
>+    *_retval = (_mbsicmp((unsigned char*) inFilePath.get(),
>+                         (unsigned char*) mWorkingPath.get()) == 0);
>     return NS_OK;
> }

I don't like this change; Windows can have case-sensitive filesystems, and
changing the behaviour of our Equals method isn't something we should do
lightly.  If they match case-insensitively, _and_ they resolve to the same
inode or Windows equivalent, I could maybe see us treating them as equal, but
just throwing case-folding in there doesn't seem right.  (And you still have
the \-vs.-/ issue, I suspect.)

r- on the basis of the case-sensitivity change.
Attachment #187969 - Flags: first-review?(shaver) → first-review-
Comment on attachment 187969 [details] [diff] [review]
Part 1, rev. 1: make a nsIPersistentDescriptorService

After more research, I've changed my mind: case-sensitivity will throw Windows
into a tizzy anyway, and we will still be case preserving even with this
change. r=shaver
Attachment #187969 - Flags: first-review-
Attachment #187969 - Flags: first-review+
Attachment #187969 - Flags: approval1.8b3?
(Assignee)

Updated

12 years ago
Priority: -- → P1

Updated

12 years ago
Attachment #187969 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 5

12 years ago
Comment on attachment 187969 [details] [diff] [review]
Part 1, rev. 1: make a nsIPersistentDescriptorService

I checked in only the nsLocalFileWin change, as the rest of this code is not
being used yet.
(Assignee)

Comment 6

12 years ago
Created attachment 188692 [details] [diff] [review]
Part 2, rev. 1: use the new relative/absolute descriptors and store the last-run-from path in compatibility.ini
Attachment #188692 - Flags: second-review?(darin)
Attachment #188692 - Flags: first-review?(rob_strong)
(Assignee)

Updated

12 years ago
Attachment #188692 - Attachment description: Part 2, rev. 1: use the new relate/absolute descriptors and store the last-run-from path in compatibility.ini → Part 2, rev. 1: use the new relative/absolute descriptors and store the last-run-from path in compatibility.ini
(Assignee)

Updated

12 years ago
Whiteboard: has patch, needs review robstrong+darin
(Assignee)

Updated

12 years ago
Whiteboard: has patch, needs review robstrong+darin → [no l10n impact] has patch, needs review robstrong+darin

Comment 7

12 years ago
Why not just use relative descriptors for app-global extensions?  Instead of
making them relative to the profile directory, make them relative to the
application directory.  EM already supports uninstalling extensions that no
longer exist, so this would seem to do the trick.  It is probably just a few
lines of code to implement this change in nsExtensionManager.js.  Am I missing
something?  Sorry for not commenting sooner, but this is the first I've seen of
this bug.
(Assignee)

Comment 8

12 years ago
If we were only dealing with app and global it wouldn't be a big deal, but as we
get more extension install paths (toolkit extensions) and have to deal with
roaming profiles and USBstick-installations it starts to break down. This path
solution works a lot better and is fairly robust.
I won't have time to do a proper review until around 7 PM PDT.

What happens if the app path changes, the original app path contained an
extension that is only compatible with 1.0+, and the new app path contains a
newer version of the same extension that is only compatible with 1.1? Is the
metadata updated so it isn't disabled at some point now that the extensions
datasource is not deleted?

-      if (val)
+      if (val) {
         autoregFile.create(nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
+      }
       else if (!autoregFile.exists())
nit: there's no reason for this change and it's not the style for the majority
of this file.

   addItem: function(opType, entry) {
-    if (!(opType in this._ops))
-      this._ops[opType] = [];
-    this._ops[opType].push(entry);
+    if (opType == "") {
+      delete this._ops[entry.id];
+    }
+    else {
+      entry.opType = opType;
+      this._ops[entry.id] = entry;
+    }
nit: I believe that opType == "" should be opType == OP_NONE

Comment 10

12 years ago
> If we were only dealing with app and global it wouldn't be a big deal, but as we
> get more extension install paths (toolkit extensions) and have to deal with
> roaming profiles and USBstick-installations it starts to break down. This path
> solution works a lot better and is fairly robust.

I don't understand why the extension install location type is not sufficient to
select a base directory.  You could have a different type for app global and
toolkit global (heck call them app-global and toolkit-global).

As for roaming profiles and USBstick-installations, can you describe the problem
in more detail?

Comment 11

12 years ago
Another thing.  If you want to use directory service keys, then you should
really consider using resource:// URLs since unlike the component registry, EM
has access to Necko.
Comment on attachment 188692 [details] [diff] [review]
Part 2, rev. 1: use the new relative/absolute descriptors and store the last-run-from path in compatibility.ini

Now that the extensions.rdf is not deleted this causes problems in
_upgradeExtensionChrome and _upgradeThemeChrome when an item no longer exists
in the extensions directory and the app path changes. Something along the lines
of the following should resolve it. Just checking itemLocation won't work since
getItemLocation creates the directory if it doesn't exist.

     for (i = 0; i < extensions.length; ++i) {
       var e = extensions[i];
       var itemLocation = e.location.getItemLocation(e.id);
+      var installRDF = itemLocation.clone();
+      installRDF.append(FILE_INSTALL_MANIFEST);
+      if (!installRDF.exists()) {
+	 var installLocation = this.getInstallLocation(e.id);
+	 StartupCache.put(installLocation, e.id, OP_NEEDS_UNINSTALL, true);
+	 StartupCache.write();
+	 continue;
+      }
       var manifest = itemLocation.clone();
       manifest.append(FILE_CHROME_MANIFEST);
Attachment #188692 - Flags: first-review?(rob_strong) → first-review-
The extensions directory i was referring to is the app's extensions directory
(e.g. app-global).
I just noticed that it leaves the itemLocation behind so that will also need to
be removed.
(Assignee)

Updated

12 years ago
Attachment #188692 - Flags: second-review?(darin)
(Assignee)

Comment 15

12 years ago
Created attachment 189182 [details] [diff] [review]
Part 2, rev. 2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini
Attachment #189182 - Flags: first-review?(rob_strong)

Comment 16

12 years ago
Benjamin and I discussed this over IRC, and we agreed that deleting
extensions-startup.manifest from nsAppRunner is a bad idea because it records
state information such as "needs-install" that could be lost.  We also talked
about the fact that file paths with spaces in them could be problematic.
(Assignee)

Comment 17

12 years ago
Created attachment 189197 [details] [diff] [review]
Part 2, rev. 2.1: use relative/absolute descriptors and store the last-run-from path in compatibility.ini

This version does not remove extensions-startup.manifest on a
path-change/upgrade, and because I am changing the format of
exentensions-startup I have renamed it to extensions.cache.
Attachment #188692 - Attachment is obsolete: true
Attachment #189182 - Attachment is obsolete: true
Attachment #189197 - Flags: second-review?(darin)
Attachment #189197 - Flags: first-review?(rob_strong)
I tried out the patch and with a new or an existing profile all extensions were
repeatedly installed during each startup until nsAppRunner prevented the
restart. This includes subsequent restarts. I'll take a look at what could be
causing this later tonight.
Here is where it is failing for me with setRelativeDescriptor requiring two args
+  if (m[1] == "rel") {
+    location.setRelativeDescriptor(decodeURI(installLocation.location, m[2]));
+  }
Comment on attachment 189197 [details] [diff] [review]
Part 2, rev. 2.1: use relative/absolute descriptors and store the last-run-from path in compatibility.ini

>-const FILE_INSTALLED_EXTENSIONS       = "installed-extensions.txt"
>-const FILE_INSTALLED_EXTENSIONS_PROCESSED = "installed-extensions-processed.txt"
There are several more constants that can be removed if you are willing. :)
DIR_DEFAULTS_PREFS
DIR_UNINSTALL
DIR_COMPONENTS
DIR_DEFAULTS
DIR_DEFAULTS_PREFS
DIR_DEFAULTS_EXTENSIONS

>- * @param   locationKey
>- *          The name of the Install Location where the item is installed
>  * @param   descriptor
>  *          The descriptor that locates the directory
>  * @returns The nsILocalFile object representing the location of the item
>  */
>-function getFileFromDescriptor(locationKey, descriptor) {
<snip>
>+function getFileFromDescriptor(descriptor, installLocation) {
Needs a @param installLocation description

>-  var lf = Components.classes["@mozilla.org/file/local;1"]
>-                      .createInstance(nsILocalFile);
nsILocalFile is declared as a constant and the style should be
var location = Components.classes["@mozilla.org/file/local;1"]
			 .createInstance(nsILocalFile);

>+  if (m[1] == "rel") {
>+    location.setRelativeDescriptor(decodeURI(installLocation.location, m[2]));
>+  }
>+  else {
>+    location.persistentDescriptor = decodeURI(m[2]);
>+  }
setRelativeDescriptor requires two args with a nsILocalFile for the first arg.
What requirement is there to use encodeURI and decodeURI?

>+            var line = locationKey + "\t" + id + "\t" + entry.descriptor + "\t" +
>+                       itemMTime + "\t" + entry.op + "\r\n";
>+            fos.write(line, line.length);
>+          }
>+          catch (e) { }
Might be a good idea to LOG the error here.

>+      var installRDF = itemLocation.clone();
>+      installRDF.append(FILE_INSTALL_MANIFEST);
>+      if (!installRDF.exists()) {
>+        var installLocation = this.getInstallLocation(e.id);
>+        StartupCache.put(installLocation, e.id, OP_NEEDS_UNINSTALL, true);
>+        StartupCache.write();
>+        continue;
>+      }
This does not appear to be needed anymore now that the extension's cache file
is not deleted. I don't believe it will cause any harm and it may in some
instances prevent and / or hide problems.

>+      var installRDF = itemLocation.clone();
>+      installRDF.append(FILE_INSTALL_MANIFEST);
>+      if (!installRDF.exists()) {
>+        var installLocation = this.getInstallLocation(item.id);
>+        StartupCache.put(installLocation, item.id, OP_NEEDS_UNINSTALL, true);
>+        StartupCache.write();
>+        continue;
>+      }
Same here

>-      if (val)
>+      if (val) {
>         autoregFile.create(nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
>+      }
>       else if (!autoregFile.exists())
nit: the added braces aren't the style of the majority of the file
Attachment #189197 - Flags: first-review?(rob_strong) → first-review-
Attachment #189182 - Flags: first-review?(rob_strong)

Comment 21

12 years ago
(In reply to comment #16)
> ... We also talked
> about the fact that file paths with spaces in them could be problematic.

I should have done my homework better.  The code uses tabs instead of spaces as
delimiters in extensions-startup.manifest, so there is no concern about
filenames that contain spaces.
(Assignee)

Comment 22

12 years ago
Comment on attachment 189197 [details] [diff] [review]
Part 2, rev. 2.1: use relative/absolute descriptors and store the last-run-from path in compatibility.ini

The URIencode/decode stuff was added at the last second without proper testing:
since the manifest is tab-delimited anyway I will remove that code.
Attachment #189197 - Flags: second-review?(darin)
(Assignee)

Comment 23

12 years ago
Created attachment 189326 [details] [diff] [review]
Part 2, rev. 2.2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini (no encodeURI/decodeURI)
Attachment #189326 - Flags: second-review?(darin)
Attachment #189326 - Flags: first-review?(rob_strong)
Comment on attachment 189326 [details] [diff] [review]
Part 2, rev. 2.2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini (no encodeURI/decodeURI)

brilliant
Attachment #189326 - Flags: first-review?(rob_strong) → first-review+
(Assignee)

Updated

12 years ago
Whiteboard: [no l10n impact] has patch, needs review robstrong+darin → [no l10n impact] has patch, needs review darin

Comment 25

12 years ago
Comment on attachment 189326 [details] [diff] [review]
Part 2, rev. 2.2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini (no encodeURI/decodeURI)

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>-function getDescriptorFromFile(locationKey, itemLocation) {
>+function getRelativeDescriptor(itemLocation, installLocation) {

I still prefer the previous name since the descriptor is not
always relative.  It also goes well with getFileFromDescriptor.


>+function getFileFromDescriptor(descriptor, installLocation) {
>+  var location = Components.classes["@mozilla.org/file/local;1"]
>+                           .createInstance(nsILocalFile);
>+
>+  var m = descriptor.match(/^(abs|rel)\%(.*)$/);
>+  if (!m)
>+    throw Components.results.NS_ERROR_INVALID_ARG;
>+
>+  if (m[1] == "rel") {
>+    location.setRelativeDescriptor(installLocation.location, m[2]);
>+  }
>+  else {
>+    location.persistentDescriptor = m[2];
>+  }
>+
>+  return location;
> }

This function only works if descriptor was generated by
getRelativeDescriptor right?  It doesn't work if given
a raw persistentDescriptor, right?


>Index: toolkit/xre/nsAppRunner.cpp

>+CompatibilityCheck(nsIFile* aProfileDir, const nsCString& aVersion,
>+                   nsIFile* aXULRunnerDir, nsIFile* aAppDir)

DoCompatibilityCheck?  or maybe CheckCompatibility?  either one
of those seems like a better name to me.  i prefer the second.


>Index: toolkit/xre/nsXULAppAPI.h

>+ * A directory service key which provides the platform-correct
>+ * "application data" directory.
>+ * Windows: Documents and Settings\<User>\Application Data\<Vendor>\<Application>
>+ * Unix: ~/.<vendor>/<application>
>+ * Mac: ~/Library/Application Supports/<Application>
>+ */
>+#define XRE_USER_APP_DATA_DIR "UAppData"

So, I don't see any consumer of this key in this patch.  Do
you really mean to add it?  Or, is it leftover from a previous
version of this patch?


>Index: xpcom/io/nsLocalFileWin.cpp

> NS_IMETHODIMP
> nsLocalFile::Equals(nsIFile *inFile, PRBool *_retval)
...
>+    DWORD thisr = GetShortPathName(mWorkingPath.get(), thisshort, sizeof(thisshort));
>+    DWORD thatr = GetShortPathName(inFilePath.get(), thatshort, sizeof(thatshort));

It'll be interesting to see what kind of impact if any this may
have on performance.
Attachment #189326 - Flags: second-review?(darin) → second-review+
(Assignee)

Comment 26

12 years ago
Created attachment 190246 [details] [diff] [review]
Part 2, rev 2.3 (final)

This is the final patch for checkin, self-approving per deerpark triage.
Attachment #187969 - Attachment is obsolete: true
Attachment #189197 - Attachment is obsolete: true
Attachment #189326 - Attachment is obsolete: true
Attachment #190246 - Flags: approval1.8b4+
(Assignee)

Comment 27

12 years ago
Fixed on trunk for 1.8b4
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Comment 28

12 years ago
> >+#define XRE_USER_APP_DATA_DIR "UAppData"
> 
> So, I don't see any consumer of this key in this patch.  Do
> you really mean to add it?  Or, is it leftover from a previous
> version of this patch?

I did add it for a previous revision, but I still think it's a good idea...
people have been using APP_REGISTRY_DIR for this, but APP_REGISTRY_DIR may
(will) change in the future.

Comment 29

12 years ago
*** Bug 301135 has been marked as a duplicate of this bug. ***

Comment 30

12 years ago
This bug might have caused a memory leak. It's either this, or bug 300731.

Before:
RLk:4.55KB
Lk:300KB
MH:7.69MB
A:215K

After:
RLk:4.55KB
Lk:338KB <--
MH:8.15MB <--
A:249K <---
I have no clue if it has to do with this bug, but I see a terrible memory leak
too. I surfed for an hour with only two tabs open, no java, no flash, and
suddenly I could hardly scroll. The browser scrolled laggy and with delay on a
normal site like bugzilla. There was only 32 MB RAM left and the taskmanager
told me that Firefox was using 146.200 kB.
(Assignee)

Comment 32

12 years ago
*** Bug 301965 has been marked as a duplicate of this bug. ***

Updated

9 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
You need to log in before you can comment on or make changes to this bug.