297312 - EM gets confused between multiple installations of the same version

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: 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.

Comment 2

[Benjamin Smedberg]

Need to solve this at least one way for 1.8b4.

Comment 3

[Mike Shaver (:shaver emeritus)]

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.

Comment 4

[Mike Shaver (:shaver emeritus)]

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

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[Benjamin Smedberg]

Attachment: Part 2, rev. 1: use the new relative/absolute descriptors and store the last-run-from path in compatibility.ini

Comment 7

[Darin Fisher]

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.

Comment 8

[Benjamin Smedberg]

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.

Comment 9

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

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

[Darin Fisher]

> 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

[Darin Fisher]

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 12

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

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

Comment 13

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

The extensions directory i was referring to is the app's extensions directory
(e.g. app-global).

Comment 14

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

I just noticed that it leaves the itemLocation behind so that will also need to
be removed.

Comment 15

[Benjamin Smedberg]

Attachment: Part 2, rev. 2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini

Comment 16

[Darin Fisher]

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.

Comment 17

[Benjamin Smedberg]

Attachment: 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.

Comment 18

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

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.

Comment 19

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

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 20

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

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

Comment 21

[Darin Fisher]

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

Comment 22

[Benjamin Smedberg]

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.

Comment 23

[Benjamin Smedberg]

Attachment: Part 2, rev. 2.2: use relative/absolute descriptors and store the last-run-from path in compatibility.ini (no encodeURI/decodeURI)

Comment 24

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

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

Comment 25

[Darin Fisher]

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.

Comment 26

[Benjamin Smedberg]

Attachment: Part 2, rev 2.3 (final)

This is the final patch for checkin, self-approving per deerpark triage.

Comment 27

[Benjamin Smedberg]

Fixed on trunk for 1.8b4

Comment 28

[Benjamin Smedberg]

> >+#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

[Hermann Schwab]

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

Comment 30

[Caleb]

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

Comment 31

[Ria Klaassen (not reading all bugmail)]

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.

Comment 32

[Benjamin Smedberg]

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