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.

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.

Need to solve this at least one way for 1.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.

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

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.

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

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

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

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.

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.

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.

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

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

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

Fixed on trunk for 1.8b4

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

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

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.

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