594058 - invalidate cache by statting contents of extensions directory

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benedict Hsieh [:benedict]]

After Bug 533038 lands, there should be relatively few files in the extension directory for normal users. We can stat them all and compare their timestamps to the timestamp of the fastload/startupcache files, and invalidate the caches if the caches are older than the files in the extensions directory.

We are not attempting to preserve the old dependency-tracking code, we will simply stat everything in the appropriate directory (or some easily-determined subset like .xul/.js files). Not sure yet whether to automatically invalidate upon finding a pointer file (indicates that the user is a developer), or follow the pointer.

The result should be that when a file is edited in the extension directory, the caches are invalidated and written out again on the next run. Normal users should not experience a large Ts hit, because their extensions should be jarred. Extension developers can use unpacked extensions and edit those files directly.

Comment 1

[Benedict Hsieh [:benedict]]

Implementation question: should I call into the extension manager to either do the statting, or get the appropriate directory to stat? Some code in XPIProvider.jsm seems to indicate that there could be more than one directory where extensions are installed, and some code already exists in there to walk the directories. The problem is that the extension manager starts up later than the startupcache, and also after XPCOM init.

Comment 2

[Benjamin Smedberg]

No, you should invalidate startupcache after startup in that case.

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: stats extension directory to invalidate startupcache

Can get dwitte to review startupcache bits if you're not comfortable looking at that, but I think the more important part is the EM bit + use of observer service to pass the message.

The hope is that total stats will be relatively few once bug 533038 lands.

Comment 4

[Benedict Hsieh [:benedict]]

Attachment: invalidates the fastload-backed XULcache

This patch won't be necessary if I can manage to land bug 592943 first to switch XULCache to use startupcache instead of fastload. I'm pretty unhappy with this patch, tbh.

The current startup process is:
startupcache inits, then EM inits, then XULCache inits. That means that XULCache misses the notification that EM sends out, so either it needs to ask either startupcache or EM for information on the result of the statting of the extension files. 

I chose to ask startupcache so I didn't have to add additional interfaces to EM, but from an organizational point of view that doesn't make much sense. Asking EM is more logical, but also kind of awkward (information needs to be retrieved from XPIProvider, which is another level deep). I want this patch to go away quickly, so adding an interface for it is also kind of lame.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 473361 [details] [diff] [review]
stats extension directory to invalidate startupcache

We already gather last modified times for extensions elsewhere, might as well do the more thorough check there and get it right in the first place. Also not sure we need to care about what the latest last modified time is, if any extension has changed then we should just be invalidating the cache then (which we already try to do in invalidateCachesOnRestart).

Comment 6

[Benedict Hsieh [:benedict]]

Attachment: modifies EM to stat recursively, invalidates cache when needed

Is this along the lines of what you had in mind? The existing code does a second getInstallLocations() that I think could be removed -- with this patch, that call gets a lot more expensive so I want to look at getting rid of it. See the comment in-line.

I'll update the XULcache patch once we decide on a good approach for this one.

Comment 7

[Benedict Hsieh [:benedict]]

Attachment: modifies EM to stat recursively, invalidates cache when needed

Took a shot at removing that second getInstallLocationStates() call.

The reason that that call gets more expensive with this patch is that it calls getAddonStates(), which used to stat a directory but now recursively stats the directory. 

Although, I just realized in a jarred-extension world it's not so bad, only 1 avoidable stat per extension. If this feels dangerous, I can just leave the original getInstallLocationStates() call in place.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 473422 [details] [diff] [review]
modifies EM to stat recursively, invalidates cache when needed

Please provide patches with 8 lines of context.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -882,6 +882,36 @@
> }
> 
> /**
>+  * Returns the timestamp of the most recently modified file in a directory,
>+  * or simply the file's own timestamp if it is not a directory.
>+  * 
>+  * @param aFile
>+  * A non-null nsIFile object
>+  * @return -1 for empty directory, otherwise epoch time.
>+  */
>+function recrLastModifiedTime(aFile) {

Don't really like the name but I'd prefer the more explicit recursiveLastModifiedTime.

>+  // For this function, a return value of -1 means no valid files found.
>+  if (aFile.isFile())
>+    return aFile.lastModifiedTime;

Just return aFile.lastModifiedTime if the file is not a directory and then continue, it should always give us an answer of sorts.

>+  if (aFile.isDirectory()) {
>+    let entries = aFile.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
>+    let entry, time;
>+    let maxTime = -1;

Just set this to 0, I am mildly troubled by negative values for timestamps.

>+    while (entry = entries.nextFile) {
>+      if (!entry instanceof Ci.nsILocalFile)
>+        continue;

What is this necessary for?

>+      time  = recrLastModifiedTime(entry);

Too many spaces

>+      if (time > maxTime)
>+        maxTime = time;

maxTime = Math.max(time, maxTime)

>       let addons = XPIDatabase.getAddonsInLocation(aLocation);
>       addons.forEach(function(aOldAddon) {
>         changed = removeMetadata(aLocation, aOldAddon) || changed;
>+        cacheInvalidate = true;
>       }, this);
>     }, this);
> 
>     // Cache the new install location states
>-    cache = JSON.stringify(this.getInstallLocationStates());
>+    cache = JSON.stringify(newAddonStates);

I'm pretty sure this isn't building up the cache correctly. This means that the subsequent restart will be forced down the slow patch again unnecessarily. I suggest putting some logging comparing the results of this vs. getInstallLocationStates and comparing them under some of the test runs to see how it behaves.

>     Services.prefs.setCharPref(PREF_INSTALL_CACHE, cache);
>+    
>+    if (cacheInvalidate)
>+      Services.obs.notifyObservers(null, "startupcache-invalidate", null);

Where does cacheInvalidate differ from changed here?

Comment 9

[Benedict Hsieh [:benedict]]

Attachment: fixes review comments, and fixes xul cache

>Just return aFile.lastModifiedTime if the file is not a directory and then
>continue, it should always give us an answer of sorts.
Sometimes there is a symlink leading to a missing file in here, so I wrapped the whole thing in try-catch.

>What is this necessary for?
Cribbed it from elsewhere in XPIProvider.jsm 5959.

>I'm pretty sure this isn't building up the cache correctly. This means that the
>subsequent restart will be forced down the slow patch again unnecessarily. I
>suggest putting some logging comparing the results of this vs.
>getInstallLocationStates and comparing them under some of the test runs to see
>how it behaves.
You're right, changed this back.

>Where does cacheInvalidate differ from changed here?
The observer actually invalidates immediately, the previous call on appInfo followed the earlier .autoreg semantics of "invalidate on next startup". I can file a follow-up to remove the latter if it's no longer necessary (along with some dependency code that should also be no longer necessary).

Comment 10

[Dave Townsend [:mossop]]

Caching of add-ons is actually a bit broken without this now we no longer have the EM restart. Not my component but I'd suggest this should block betaN

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 475353 [details] [diff] [review]
fixes review comments, and fixes xul cache

(In reply to comment #9)
> Created attachment 475353 [details] [diff] [review]
> fixes review comments, and fixes xul cache
> 
> >Just return aFile.lastModifiedTime if the file is not a directory and then
> >continue, it should always give us an answer of sorts.
> Sometimes there is a symlink leading to a missing file in here, so I wrapped
> the whole thing in try-catch.
> 
> >What is this necessary for?
> Cribbed it from elsewhere in XPIProvider.jsm 5959.

We need it there as we have to access properties of nsILocalFile on the add-on's directory. Unless you need a property of nsILocalFile in this loop (and I can't see one) then it is unnecessary.

> >Where does cacheInvalidate differ from changed here?
> The observer actually invalidates immediately, the previous call on appInfo
> followed the earlier .autoreg semantics of "invalidate on next startup". I can
> file a follow-up to remove the latter if it's no longer necessary (along with
> some dependency code that should also be no longer necessary).

What I meant was, why do we have two different variables here? The "changed" variable was meant to track any case where an add-on change had been made that would require rewriting extensions.ini and hence invalidating the cache after the restart. I'd expect that to be the same cases where we want to invalidate the startup cache so I'm not sure we need the extra variable.

cacheInvalidate is certainly true in more cases since right now it is invalidating even when changes are made to non-visible extensions. Just using changed as-is would avoid that.

You should be able to add some code to tests to verify that the cache notifications are and aren't being sent out appropriately.

>diff --git a/content/xul/document/src/nsXULPrototypeCache.cpp b/content/xul/document/src/nsXULPrototypeCache.cpp
>--- a/content/xul/document/src/nsXULPrototypeCache.cpp
>+++ b/content/xul/document/src/nsXULPrototypeCache.cpp
>@@ -176,17 +176,17 @@
> nsXULPrototypeCache::Observe(nsISupports* aSubject,
>                              const char *aTopic,
>                              const PRUnichar *aData)
> {
>     if (!strcmp(aTopic, "chrome-flush-skin-caches")) {
>         FlushSkinFiles();
>     }
>     else if (!strcmp(aTopic, "chrome-flush-caches")) {
>-        Flush();
>+        AbortFastLoads();

Is this meant to be part of this patch?

>diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp
>--- a/startupcache/StartupCache.cpp
>+++ b/startupcache/StartupCache.cpp
>@@ -33,16 +33,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "prio.h"
> #include "prtypes.h"
>+#include "prlong.h"

Not sure this is necessary.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm

>+function recursiveLastModifiedTime(aFile) {
>+
>+  function recursiveLastModifiedTimeHelper(aFile) {
>+    if (aFile.isDirectory()) {
>+      let entries = aFile.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
>+      let entry, time;
>+      let maxTime = 0;
>+      while (entry = entries.nextFile) {
>+	if (!entry instanceof Ci.nsILocalFile)
>+	  continue;
>+	time = recursiveLastModifiedTimeHelper(entry);
>+	maxTime = Math.max(time, maxTime);
>+      }
>+      entries.close();
>+      return maxTime;
>+    }
>+    return aFile.lastModifiedTime;
>+  }
>+
>+  try {
>+    return recursiveLastModifiedTimeHelper(aFile);
>+  } catch (e) {
>+    LOG("Caught exception " + e + " in recursiveLastModifiedTime.");
>+
>+    // Most likely, symlink to missing file.
>+    // XXX Neither 0 nor PR_INT64_MAX seem quite right here.
>+    // We probably want this to invalidate the first time, and since
>+    // we use != to compare mtimes elsewhere, this will have that behavior..
>+    return Math.pow(2, 32) - 1;
>+  }

This isn't correct. It means that a directory containing an invalid symlink will always return the same mtime regardless of how the other files in the directory change. I hadn't considered the symlink case so you should probably just go back to how this function looked before, just return 0 when it isn't a file or directory.

Comment 12

[Benedict Hsieh [:benedict]]

Attachment: fixes review comments, including test

>cacheInvalidate is certainly true in more cases since right now it is
>invalidating even when changes are made to non-visible extensions. Just using
>changed as-is would avoid that.
>You should be able to add some code to tests to verify that the cache
>notifications are and aren't being sent out appropriately.
Ah okay. I wasn't sure what the 'changed' variable was, but that makes sense and seems to work correctly.

>Is this meant to be part of this patch?
Yes. It seems more correct to invalidate the on-disk cache when invalidating the in-memory one, and it also helps with this patch.

Fixed other review comments, and added a test.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 477370 [details] [diff] [review]
fixes review comments, including test

>diff --git a/content/xul/document/src/nsXULPrototypeCache.cpp b/content/xul/document/src/nsXULPrototypeCache.cpp
>--- a/content/xul/document/src/nsXULPrototypeCache.cpp
>+++ b/content/xul/document/src/nsXULPrototypeCache.cpp
>@@ -176,17 +176,17 @@ NS_IMETHODIMP
> nsXULPrototypeCache::Observe(nsISupports* aSubject,
>                              const char *aTopic,
>                              const PRUnichar *aData)
> {
>     if (!strcmp(aTopic, "chrome-flush-skin-caches")) {
>         FlushSkinFiles();
>     }
>     else if (!strcmp(aTopic, "chrome-flush-caches")) {
>-        Flush();
>+        AbortFastLoads();
>     }
>     else {
>         NS_WARNING("Unexpected observer topic.");
>     }
>     return NS_OK;
> }
> 
> nsXULPrototypeDocument*

Can you get someone else to just verify this, I know very little about this code.

>diff --git a/toolkit/mozapps/extensions/test/addons/test_bug594058/install.rdf b/toolkit/mozapps/extensions/test/addons/test_bug594058/install.rdf
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/addons/test_bug594058/install.rdf
>@@ -0,0 +1,21 @@
>+<?xml version="1.0"?>
>+
>+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
>+
>+  <Description about="urn:mozilla:install-manifest">
>+    <em:id>[e-mail]">[e-mail]</em:id>
>+    <em:version>1.0</em:version>
>+    
>+    <em:targetApplication>
>+      <Description>
>+        <em:id>[e-mail]</em:id>
>+        <em:minVersion>1</em:minVersion>
>+        <em:maxVersion>1</em:maxVersion>
>+      </Description>
>+    </em:targetApplication>
>+     <em:name>bug 594058</em:name>

Slight indent problem

>+    <em:description>stat-based invalidation</em:description>
>+    <em:unpack>true</em:unpack>
>+  </Description>      
>+</RDF>
>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_bug594058.js b/toolkit/mozapps/extensions/test/xpcshell/test_bug594058.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_bug594058.js
>@@ -0,0 +1,86 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation
>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Benedict Hsieh <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL
>+ *
>+ * ***** END LICENSE BLOCK *****
>+ */

We're using the public domain header for new tests, please switch to that.

>+
>+// Disables security checking our updates which haven't been signed
>+Services.prefs.setBoolPref("extensions.checkUpdateSecurity", false);
>+
>+const extDir = gProfD.clone();
>+extDir.append("extensions");
>+
>+/**
>+ * Start the test by installing extensions.
>+ */
>+function run_test() {
>+  do_test_pending();
>+  let cachePurged = false;
>+
>+  let obs = AM_Cc["@mozilla.org/observer-service;1"].
>+    getService(AM_Ci.nsIObserverService);
>+  obs.addObserver({
>+    observe: function(aSubject, aTopic, aData) {
>+      dump("observed: " + aSubject + "\n");
>+      cachePurged = true;
>+    }
>+  }, "startupcache-invalidate", false);
>+  createAppInfo("[e-mail]", "XPCShell", "1", "1");
>+  startupManager();
>+
>+  installAllFiles([do_get_addon("test_bug594058")], function() {
>+    restartManager();
>+    do_check_true(cachePurged);
>+    cachePurged = false;
>+
>+    let extFile = extDir.clone();
>+    extFile.append("[e-mail]">[e-mail]");
>+    extFile.append("directory");
>+    extFile.append("file1");
>+    extFile.remove(false);
>+    extFile.create(Components.interfaces.nsILocalFile.NORMAL_FILE_TYPE,
>+                  700);

Use Ci.

>+
>+    restartManager();
>+    do_check_true(cachePurged);
>+    cachePurged = false;
>+
>+    restartManager();
>+    do_check_true(!cachePurged);
>+
>+    do_test_finished();
>+  });  
>+}

Comment 14

[Benedict Hsieh [:benedict]]

Attachment: fixes review comments

bsmedberg, can you look over this part? Basically the changes we talked about to flush XULPrototypeCache, except that AbortFastLoads() already calls Flush() instead of the other way around.

>diff --git a/content/xul/document/src/nsXULPrototypeCache.cpp b/content/xul/document/src/nsXULPrototypeCache.cpp
>--- a/content/xul/document/src/nsXULPrototypeCache.cpp
>+++ b/content/xul/document/src/nsXULPrototypeCache.cpp
>@@ -176,17 +176,17 @@ NS_IMETHODIMP
> nsXULPrototypeCache::Observe(nsISupports* aSubject,
>                              const char *aTopic,
>                              const PRUnichar *aData)
> {
>     if (!strcmp(aTopic, "chrome-flush-skin-caches")) {
>         FlushSkinFiles();
>     }
>     else if (!strcmp(aTopic, "chrome-flush-caches")) {
>-        Flush();
>+        AbortFastLoads();
>     }
>     else {
>         NS_WARNING("Unexpected observer topic.");
>     }
>     return NS_OK;
> }
> 
> nsXULPrototypeDocument*

Comment 15

[Benjamin Smedberg]

Comment on attachment 478067 [details] [diff] [review]
fixes review comments

I don't really know XUL fastload, punting to bz.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I don't really know that code either.  I can read up in it a bit if needed, but maybe Brendan remembers something about it?  This stuff isn't really owned.  :(

Comment 17

[Benedict Hsieh [:benedict]]

I'm going to change this so that xulPrototypeCache just listens for the same message that startupCache does, and invalidates its own cache (via AbortFastLoads()). The intuition I had was, if we're flushing the in-memory cache we probably want to flush the backing on-disk cache as well. But it's not important for the purpose of this patch, especially if the code isn't understood that well.

I do know the fastload part of the code, somewhat, and AbortFastLoads() is a pretty common way to recover if the fastload process goes wrong somehow.

Maybe Jonas is a possible reviewer if Brendan is busy? He reviewed some of this code for my attempt to make xulPrototypeCache use startupCache, and this will be literally orders of magnitude smaller (about 3 lines in this section of code).

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 478067 [details] [diff] [review]
fixes review comments

I don't see any old Ben Goodger's or my old FastLoad code in the patch, so what has ownership of fastload (or lack of it) to do with reviewing this patch? Fresh eyes would be good for sure.

Is the AbortFastLoads() code unchanged?

Nit: space at front of first line cited below:

+ const Ci = Components.interfaces;
+const extDir = gProfD.clone();

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

Brendan, nsXULPrototypeCache's integration with fastload is not something you and Ben wrote?  Who did?

Anyway, does AbortFastLoads nuke the fastload file?  If it does, this looks reasonable.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> Brendan, nsXULPrototypeCache's integration with fastload is not something you
> and Ben wrote?  Who did?

You know we wrote it. My point in comment 18 is clear: the patch does not touch lines we wrote AFAICR, although it changes a Flush call to AbortFastLoads (which is a method I did write).

> Anyway, does AbortFastLoads nuke the fastload file?

That function is easily found, it has comments, and it is clear what it does even without comments. It ends thus:

    // Now rename or remove the file.
    if (file) {
#ifdef DEBUG
        // Remove any existing Aborted.mfasl files generated in previous runs.
        nsCOMPtr<nsIFile> existingAbortedFile;
        file->Clone(getter_AddRefs(existingAbortedFile));
        if (existingAbortedFile) {
            existingAbortedFile->SetLeafName(NS_LITERAL_STRING("Aborted.mfasl"));
            PRBool fileExists = PR_FALSE;
            existingAbortedFile->Exists(&fileExists);
            if (fileExists)
                existingAbortedFile->Remove(PR_FALSE);
        }
        file->MoveToNative(nsnull, NS_LITERAL_CSTRING("Aborted.mfasl"));
#else
        file->Remove(PR_FALSE);
#endif
    }

    // If the list is empty now, the FastLoad process is done.
    NS_RELEASE(gFastLoadService);
    NS_RELEASE(gFastLoadFile);
}

I don't think it's productive to talk about code not being owned (comment 16). "This code" is being revamped extensively and nicely by Benedict, I've reviewed many of his patches, so have others who have had to step up and read some code. (Ben Goodger of course works on Chrome now.)

So this last patch does not require a guru from 10 years ago to review. It needs some of us still around, arguably including content folks such as yourself (but not if you are overloaded, for sure), simply to review the patch and its dependencies with fresh eyes and not avert your eyes from code that is easily found, such as AbortFastLoads. This is the essence of ownership.

Again if you're too busy, please say so. But don't make me the "bus problem" (you know, if I'm hit by a bus...). Because this code is not that complicated, and it is being owned.

/be

Comment 21

[Ben Bucksch (:BenB)]

> #ifdef DEBUG

(maybe obvious note: If you're developing with debug builds, also test with release builds. I'd do it, but I don't know what corners to test. BTW: Thanks for working on it!)

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> > #ifdef DEBUG

I can't be sure you were replying to comment 20, but that ifdef'ed code is just saving the aborted fastload file for post-mortem studies. It doesn't affect how AbortFastLoads works, just whether the file is removed vs. renamed.

/be

Comment 23

[Boris Zbarsky [:bzbarsky]]

> although it changes a Flush call to AbortFastLoads (which is a method I did
> write).

Yep.

> Again if you're too busy, please say so.

Of course I'm too busy!  Not least because people keep asking me for review on code I'm not familiar with, in the full expectation that I will learn and understand the code and then evaluate their patch.  And I can do that, and do do that, but it takes a lot of time dammit.  Which is why sometimes I ask the patch author questions like the one in comment 19 (and sorry if you took that question to apply to you; I should have been clearer) instead of spending the time to go read the code myself as a first cut.  And sometimes I try to look for an alternate or better-qualified reviewer, as I did in comment 16.  And sometimes my proposed alternate reviewer can do the review, and sometimes he can't, and sometimes he gets all upset at me for some reason.

> I don't think it's productive to talk about code not being owned 

The XUL prototype cache?  You own it?  I sure don't...  And just above here you disclaim any responsibility for the part of it that's being touched here, right?  I don't think anyone else is stepping up to claim it either.

Anyway, I'm not sure discussing all this in the bug will get us far; please catch me on irc if you want to talk about it?  I have quite a bit to say about module ownership and the like that I probably shouldn't say in a kid-friendly safe-for-work forum like this.  ;)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478067 [details] [diff] [review]
fixes review comments

r=me per comment 20, though I still think Brendan is a better reviewer here.  ;)

Comment 25

[Brendan Eich [:brendan]]

I replied privately to bz.

/be

Comment 26

[Benedict Hsieh [:benedict]]

Attachment: few test fixes

Mossop, do you want to take a look at this interdiff quickly? 

I fixed up a few tests which were affected because I take the max time of a directory and its files instead of just the directory. I also changed the recursiveLastModifiedTime() function to include the modifiedTime of the directory rather than just its contents to make a test happy.

Sorry I didn't see these failures before, not quite sure why.

Comment 27

[Benedict Hsieh [:benedict]]

Attachment: few test fixes

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 480805 [details] [diff] [review]
few test fixes

Looks ok

Comment 29

[Benedict Hsieh [:benedict]]

Attachment: ready to go

Full patch ready to go when the tree opens for betaN. Thanks everyone.

Comment 30

[Benedict Hsieh [:benedict]]

http://hg.mozilla.org/mozilla-central/rev/5ede38e20e25