759416 - XPCOM FileWatcherService

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement, P5)

Description

[Paul Dagnelie]

Attachment: Patch

This is the prototype implementation of the xpcom layer for onChange notifications for linux, and the idl file describing the interface.

Comment 1

[Paul Dagnelie]

Attachment: Patch version 2, now with compilation

Untested still, but updated to actually compile

Comment 2

[Paul Dagnelie]

Attachment: Full patch with all changes, including module infrastructure

Comment 3

[Paul Dagnelie]

Attachment: Patch passing provided tests

For some reason, this code causes a segfault in xpcshell itself, in arena_dalloc_small, on exit (or thereabouts).  Not sure why this is, but it shouldn't(?) be the fault of the code.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 629971 [details] [diff] [review]
Patch passing provided tests

Review of attachment 629971 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really great start and it is very close.  Many of my comments are nits and style.  Do apply those changes throughout the patch.

I hope that we can make the locking simpler.  I'll take a closer look in the next patch.

Good stuff!!

::: xpcom/build/nsXPComInit.cpp
@@ +59,5 @@
>  
>  #include "nsILocalFile.h"
>  #include "nsLocalFile.h"
> +#include "nsIFileWatcherService.h"
> +#include "nsFileWatcherService.h"

why are these needed?  is it because of the macros?

::: xpcom/io/nsDirectoryService.cpp
@@ +77,5 @@
>  
> +    if(rv){
> +
> +    }
> +

remove

::: xpcom/io/nsFileWatcherService.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsFileWatcherService.h"

separate out gecko headers into one set of header includes.  Use "".
system headers can use <>

@@ +37,5 @@
> +class FileChangeEvent : public nsRunnable
> +{
> +  nsCOMPtr<nsIObserver> obs;
> +  nsCOMPtr<nsILocalFile> aFile;
> +  const char *type;

Member variables - mCamelCase

@@ +39,5 @@
> +  nsCOMPtr<nsIObserver> obs;
> +  nsCOMPtr<nsILocalFile> aFile;
> +  const char *type;
> +public:
> +  FileChangeEvent(nsCOMPtr<nsIObserver> observer, nsCOMPtr<nsILocalFile> file, const char *typestring) :

FileChangeEvent(nsIObserver* aObserver, nsILocalFile* aFile, const char *aTypeString) :


Note the parameter names as well as no passing nsCOMPtr's like that.

@@ +44,5 @@
> +    obs(observer) , aFile(file), type(typestring) {};
> +  
> +  NS_IMETHOD Run(){
> +    NS_ASSERTION(NS_IsMainThread(), "Not main thread!");
> +    return obs->Observe(aFile,type,nsnull);

spaces after commas

@@ +53,5 @@
> +class nsFileWatcherService::FdObserverPair
> +{
> +public:
> +  int first;
> +  nsCOMPtr<nsIObserver> second;

the names are really bad here. :)

can you name these to "mWatchDescriptor" and "mObserver", or something that is descriptive.

@@ +74,5 @@
> +}
> +void nsFileWatcherService::EventLoop()
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  struct pollfd *fds = (struct pollfd *)NS_Alloc(sizeof(struct pollfd)*2);

This leaks when this thread is interrupted, right?

@@ +101,5 @@
> +      bool dir = (event->len !=0);
> +      const char *type = NULL;
> +      const char *created = "created";
> +      const char *deleted = "deleted";
> +      const char *modified = "modified";

go ahead and make this static at the top of this file.

@@ +133,5 @@
> +      str.AppendWithConversion(storedName);
> +      nsIObserver *obs = TableLookup(event->wd,&str);
> +      
> +      nsCString fullName;
> +      fullName.Append(storedName);

.Assign()

@@ +134,5 @@
> +      nsIObserver *obs = TableLookup(event->wd,&str);
> +      
> +      nsCString fullName;
> +      fullName.Append(storedName);
> +      nsCOMPtr<nsILocalFile> file;

There is patch that is landing soon that will remove nsILocalfile.  Nothing you really need to do right now, but be aware when you rebase you might see that nsILocalFile is no longer defined.  You'll just use nsIFile here instead.

@@ +138,5 @@
> +      nsCOMPtr<nsILocalFile> file;
> +      NS_NewNativeLocalFile(fullName,true,getter_AddRefs(file));
> +      if (dir){
> +        nsCString leafName;
> +        leafName.Append(event->name);

.Assign()

@@ +155,5 @@
> +nsFileWatcherService::nsFileWatcherService()
> +{
> +  table.Init();
> +  WdLookup.Init();
> +  inotify_fd = inotify_init();

inotify works on linux.  maybe the mac too.  But we'll need to #ifdef this to make it compile.

@@ +159,5 @@
> +  inotify_fd = inotify_init();
> +  int arr[2];
> +  pipe(arr);
> +  read_fd=arr[0];
> +  shutdown_fd=arr[1];

pipe(2) returns -1 on failure.  Please test for that.  Also, when accessing read_fd/shutdown_fd, you'll need to test for uninitialized values.

@@ +163,5 @@
> +  shutdown_fd=arr[1];
> +  TableLock = PR_NewLock();
> +  LookupLock = PR_NewLock();
> +  nsRefPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &nsFileWatcherService::EventLoop);
> +  NS_NewThread(getter_AddRefs(helper),r);

Test for null result.

@@ +166,5 @@
> +  nsRefPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &nsFileWatcherService::EventLoop);
> +  NS_NewThread(getter_AddRefs(helper),r);
> +  
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);

So much can fail in this constructor, it is probably a good idea to create an Init() method so that you can return a result to XPCOM.

@@ +167,5 @@
> +  NS_NewThread(getter_AddRefs(helper),r);
> +  
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> +          /* member initializers and constructor code */

Remove

@@ +172,5 @@
> +}
> +
> +nsFileWatcherService::~nsFileWatcherService()
> +{
> +  close(inotify_fd);

As I mentioned above, you'll have to test for these things being uninit'ed.

@@ +176,5 @@
> +  close(inotify_fd);
> +  PRThread *thread;
> +  helper->GetPRThread(&thread);
> +  PR_Interrupt(thread);
> +  helper->Shutdown();

You might as well file a new bug to add Interrupt() to nsIThread.  We should just fix it.

@@ +179,5 @@
> +  PR_Interrupt(thread);
> +  helper->Shutdown();
> +  PR_DestroyLock(TableLock);
> +  PR_DestroyLock(LookupLock);
> +          /* destructor code */

remove comment.

@@ +193,5 @@
> +  nsAutoString path;
> +  aFile->GetPath(path);
> +  const char *tmpbuf = NS_ConvertUTF16toUTF8(path).get();
> +  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
> +  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);

nsAutoCString path
aFile->GetNativePath(path);

const char* buf = path.Get()

I *hate* our string classes.  But try the above.

@@ +195,5 @@
> +  const char *tmpbuf = NS_ConvertUTF16toUTF8(path).get();
> +  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
> +  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);
> +  
> +  PR_Lock(TableLock);

why do we have to lock the table?  This code should only be called on the main thread, right?

@@ +216,5 @@
> +  if (!isdir)
> +    return NS_OK;
> +  
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

Explain this part to me.  Why do we need to do this?  Doesn't inotify to this for us?

@@ +288,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(result);
> +  NS_ENSURE_NO_AGGREGATION(outer);
> +  
> +  *result = nsnull;

You don't need this assignment.

@@ +291,5 @@
> +  
> +  *result = nsnull;
> +  
> +  nsRefPtr<nsFileWatcherService> inst = new nsFileWatcherService();
> +  if (!inst)

new doesn't return null when it fails.  No test needed.

::: xpcom/io/nsFileWatcherService.h
@@ +11,5 @@
> +#include "nsDataHashtable.h"
> +#include "nsHashKeys.h"
> +#include "nsTArray.h"
> +#include <nsIThread.h>
> +#include <prlock.h>

"" not <>

@@ +25,5 @@
> +
> +class nsFileWatcherService : public nsIFileWatcherService, public nsIObserver
> +{
> +  public:
> +  NS_DECL_ISUPPORTS

two space indent

public:
  NS_XXXXX

@@ +29,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +  NS_DECL_NSIFILEWATCHERSERVICE
> +
> +      nsFileWatcherService();

this is not indented right.

@@ +40,5 @@
> +  protected:
> +  class FdObserverPair;
> +  void EventLoop();
> +
> +  int inotify_fd;

mInotifyFD

@@ +41,5 @@
> +  class FdObserverPair;
> +  void EventLoop();
> +
> +  int inotify_fd;
> +  int shutdown_fd;

mShutdownFD, etc.


basically class members should be mCamelCase

@@ +46,5 @@
> +  int read_fd;
> +  nsClassHashtable<nsStringHashKey, nsTArray<FdObserverPair *> >table;
> +  nsDataHashtable<nsUint64HashKey,char *>WdLookup;
> +  nsIObserver *TableLookup(int, nsAString *);
> +  PRLock *TableLock, *LookupLock;

Take a look at mozilla::Mutex.  Easier to use.  What people advocate using over PRLock.

@@ +48,5 @@
> +  nsDataHashtable<nsUint64HashKey,char *>WdLookup;
> +  nsIObserver *TableLookup(int, nsAString *);
> +  PRLock *TableLock, *LookupLock;
> +  
> +  nsCOMPtr<nsIThread> helper;

better name?

@@ +50,5 @@
> +  PRLock *TableLock, *LookupLock;
> +  
> +  nsCOMPtr<nsIThread> helper;
> +  
> +  /* additional members */

remove boiler plate.

::: xpcom/io/nsIFileWatcherService.idl
@@ +9,5 @@
> +
> +[scriptable, builtinclass, uuid(add0ae9f-5460-404b-8037-1390a8fc4292)]
> +interface nsIFileWatcherService : nsISupports
> +{
> +

remove extra whitespace here.

@@ +23,5 @@
> +     *  directory, all files will be watched recursively, not
> +     *  following symlinks.
> +     */
> +    void addFileWatcher(in nsIFile aFile, in nsIObserver aObserver);
> +

remove space on this line.

::: xpcom/io/nsLocalFileUnix.cpp
@@ +1527,4 @@
>      NS_ENSURE_ARG_POINTER(_retval);
>      *_retval = false;
>  
> +    nsCString inPath;

Remove this change.

::: xpcom/io/nsLocalFileUnix.h
@@ +19,4 @@
>  #include "nsReadableUtils.h"
>  #include "nsIHashable.h"
>  #include "nsIClassInfoImpl.h"
> +#include "nsLocalFile.h"

why this change?

::: xpcom/tests/unit/test_fwsadd.js
@@ +5,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +function cleanup(){

I think mozilla code style says that there is a a space between the function() and the first curly:

not:
foo(){

but:
foo() {

@@ +7,5 @@
> +const Ci = Components.interfaces;
> +
> +function cleanup(){
> +  try{
> +  var file= do_get_cwd();

space after file:

var file = do...

@@ +17,5 @@
> +}
> +
> +var observer = {
> +  observe: function observe(subject, topic, data){
> +    try{

you probably can remove this try now, right?  Didn't you just add it for debugging?

@@ +30,5 @@
> +};
> +
> +function create(){
> +  var file = do_get_file('fwstestfile', true);
> +  file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE,0777);

space after the comma.  Also, you can use Ci:

 file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0777);

@@ +33,5 @@
> +  var file = do_get_file('fwstestfile', true);
> +  file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE,0777);
> +}
> +
> +var fws

don't bother creating a local.  Lets make it so that the nsIFileWatcherService is access with .getService(). This way, XPCOM will keep only one instance around forever.

@@ +37,5 @@
> +var fws
> +function run_test(){
> +  do_register_cleanup(cleanup);
> +  var file = do_get_cwd();
> +  fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);

fws = Cc["@mozilla.org/filewatch;1"].getService(Ci.nsIFileWatcherService);

@@ +39,5 @@
> +  do_register_cleanup(cleanup);
> +  var file = do_get_cwd();
> +  fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
> +  fws.addFileWatcher(file,observer);
> +  do_timeout(500,create);

why 500? Can it just be zero?

::: xpcom/tests/unit/test_fwsdel.js
@@ +7,5 @@
> +const Ci = Components.interfaces;
> +
> +var observer = {
> +  observe: function(subject, topic, data){
> +    try{

drop try.

@@ +27,5 @@
> +
> +var fws;
> +function run_test(){
> +  var file = do_get_file('fwstestfile',true);
> +  file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE,0777);

use Ci.

@@ +28,5 @@
> +var fws;
> +function run_test(){
> +  var file = do_get_file('fwstestfile',true);
> +  file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE,0777);
> +  fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);

same thing.  .getService

::: xpcom/tests/unit/test_fwsexist.js
@@ +6,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +var fws
> +function run_test(){

You can delete this text.  It isn't required since other tests test exactly this.

::: xpcom/tests/unit/test_fwsmod.js
@@ +27,5 @@
> +
> +function deletes(){
> +  var file = do_get_file('fwstestfile');
> +  var foStream = Components.classes["@mozilla.org/network/file-output-stream;1"].  
> +    createInstance(Components.interfaces.nsIFileOutputStream);

Use Ci, CC here.

Comment 5

[Paul Dagnelie]

Attachment: Patch after first revision

::: xpcom/build/nsXPComInit.cpp
@@ +59,5 @@
>  
>  #include "nsILocalFile.h"
>  #include "nsLocalFile.h"
> +#include "nsIFileWatcherService.h"
> +#include "nsFileWatcherService.h"

why are these needed?  is it because of the macros?

They're needed so that the component will be registered as a component, which is important so it can be accessed by Components.classes and Components.interfaces via js.

@@ +155,5 @@
> +nsFileWatcherService::nsFileWatcherService()
> +{
> +  table.Init();
> +  WdLookup.Init();
> +  inotify_fd = inotify_init();

inotify works on linux.  maybe the mac too.  But we'll need to #ifdef this to make it compile.

inotify is strictly linux-only.  Mac and windows should honestly probably be separate implementations.  None of these are going to have much in common aside from the thread loop.

@@ +176,5 @@
> +  close(inotify_fd);
> +  PRThread *thread;
> +  helper->GetPRThread(&thread);
> +  PR_Interrupt(thread);
> +  helper->Shutdown();

You might as well file a new bug to add Interrupt() to nsIThread.  We should just fix it.

I don't know what you mean?  Also, this code has been changed, as it doesn't reliably shut down the thread in question.

@@ +193,5 @@
> +  nsAutoString path;
> +  aFile->GetPath(path);
> +  const char *tmpbuf = NS_ConvertUTF16toUTF8(path).get();
> +  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
> +  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);

nsAutoCString path
aFile->GetNativePath(path);

const char* buf = path.Get()

I *hate* our string classes.  But try the above.

The problem is that I want a buffer that won't go out of scope when the function ends, since i'm storing it in a table.

@@ +195,5 @@
> +  const char *tmpbuf = NS_ConvertUTF16toUTF8(path).get();
> +  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
> +  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);
> +  
> +  PR_Lock(TableLock);

why do we have to lock the table?  This code should only be called on the main thread, right?

Yes, but the worker thread also accesses the table, and that could cause a race condition.

@@ +216,5 @@
> +  if (!isdir)
> +    return NS_OK;
> +  
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

Explain this part to me.  Why do we need to do this?  Doesn't inotify to this for us?

No, it doesn't.  inotify only watches the given directory, not the subdirectories, and so we have to add those watches manually.

@@ +33,5 @@
> +  var file = do_get_file('fwstestfile', true);
> +  file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE,0777);
> +}
> +
> +var fws

don't bother creating a local.  Lets make it so that the nsIFileWatcherService is access with .getService(). This way, XPCOM will keep only one instance around forever.

How do I make it a service?

Comment 6

[Paul Dagnelie]

Attachment: Repeat of patch after first revision

This has the tests call the code with GetService rather than CreateInstance.

Comment 7

[Paul Dagnelie]

Comment on attachment 630644 [details] [diff] [review]
Repeat of patch after first revision

>diff --git a/config/system-headers b/config/system-headers
>--- a/config/system-headers
>+++ b/config/system-headers
>@@ -703,6 +703,7 @@
> sys/filio.h
> sys/frame.h
> sys/immu.h
>+sys/inotify.h
> sys/inttypes.h
> sys/ioccom.h
> sys/ioctl.h
>diff --git a/js/src/config/system-headers b/js/src/config/system-headers
>--- a/js/src/config/system-headers
>+++ b/js/src/config/system-headers
>@@ -703,6 +703,7 @@
> sys/filio.h
> sys/frame.h
> sys/immu.h
>+sys/inotify.h
> sys/inttypes.h
> sys/ioccom.h
> sys/ioctl.h
>diff --git a/xpcom/build/XPCOM.h b/xpcom/build/XPCOM.h
>--- a/xpcom/build/XPCOM.h
>+++ b/xpcom/build/XPCOM.h
>@@ -78,6 +78,7 @@
> #include "nsIExceptionService.h"
> #include "nsIFactory.h"
> #include "nsIFile.h"
>+#include "nsIFileWatcherService.h"
> #include "nsIHashable.h"
> #include "nsIINIParser.h"
> #include "nsIInputStream.h"
>diff --git a/xpcom/build/XPCOMModule.inc b/xpcom/build/XPCOMModule.inc
>--- a/xpcom/build/XPCOMModule.inc
>+++ b/xpcom/build/XPCOMModule.inc
>@@ -66,6 +66,8 @@
> 
>     COMPONENT(UUID_GENERATOR, nsUUIDGeneratorConstructor)
> 
>+    COMPONENT(FILE_WATCHER_SERVICE, nsFileWatcherService::nsFileWatcherServiceConstructor)
>+
> #if defined(XP_WIN)
>     COMPONENT(WINDOWSREGKEY, nsWindowsRegKeyConstructor)
> #endif
>diff --git a/xpcom/build/nsXPCOM.h b/xpcom/build/nsXPCOM.h
>--- a/xpcom/build/nsXPCOM.h
>+++ b/xpcom/build/nsXPCOM.h
>@@ -57,6 +57,7 @@
> DECL_CLASS(nsIServiceManager);
> DECL_CLASS(nsIFile);
> DECL_CLASS(nsILocalFile);
>+DECL_CLASS(nsIFileWatcherService);
> DECL_CLASS(nsIDirectoryServiceProvider);
> DECL_CLASS(nsIMemory);
> DECL_CLASS(nsIDebug);
>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp
>--- a/xpcom/build/nsXPComInit.cpp
>+++ b/xpcom/build/nsXPComInit.cpp
>@@ -59,6 +59,8 @@
> 
> #include "nsILocalFile.h"
> #include "nsLocalFile.h"
>+#include "nsIFileWatcherService.h"
>+#include "nsFileWatcherService.h"
> #if defined(XP_UNIX) || defined(XP_OS2)
> #include "nsNativeCharsetUtils.h"
> #endif
>diff --git a/xpcom/io/Makefile.in b/xpcom/io/Makefile.in
>--- a/xpcom/io/Makefile.in
>+++ b/xpcom/io/Makefile.in
>@@ -46,6 +46,7 @@
> 		nsIOUtil.cpp \
> 		nsWildCard.cpp \
> 		SpecialSystemDirectory.cpp \
>+		nsFileWatcherService.cpp \
> 		$(NULL)
> 
> ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>@@ -78,6 +79,7 @@
> 		nsStringStream.h \
> 		nsStreamUtils.h \
> 		nsWildCard.h \
>+		nsFileWatcherService.h \
> 		$(NULL)			
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
>@@ -121,6 +123,7 @@
> 		nsIIOUtil.idl \
> 		nsISafeOutputStream.idl \
> 		nsIScriptableBase64Encoder.idl \
>+		nsIFileWatcherService.idl  \
> 		$(NULL)
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),os2)
>diff --git a/xpcom/io/nsFileWatcherService.cpp b/xpcom/io/nsFileWatcherService.cpp
>new file mode 100755
>--- /dev/null
>+++ b/xpcom/io/nsFileWatcherService.cpp
>@@ -0,0 +1,336 @@
>+/* -*- mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsFileWatcherService.h"
>+#include "nsClassHashtable.h"
>+#include "nsDataHashtable.h"
>+#include "nsHashKeys.h"
>+#include "nsThreadUtils.h"
>+#include "nsIThread.h"
>+#include "prlock.h"
>+#include "nsIObserver.h"
>+#include "nsLocalFile.h"
>+#include "nsISimpleEnumerator.h"
>+#include "prthread.h"
>+#include "nsIObserverService.h"
>+#include "nsCRT.h"
>+#include "mozilla/Services.h"
>+#include "mozilla/Mutex.h"
>+#include "mozilla/Util.h"
>+
>+#include <fcntl.h>
>+#include <unistd.h>
>+#include <sys/types.h>
>+#include <sys/stat.h>
>+#include <unistd.h>
>+#include <sys/inotify.h>
>+#include <sys/ioctl.h>
>+#include <poll.h>
>+
>+using namespace mozilla;
>+
>+NS_IMPL_THREADSAFE_ISUPPORTS1(nsFileWatcherService, nsIFileWatcherService)
>+
>+const char *created = "created";
>+const char *deleted = "deleted";
>+const char *modified = "modified";
>+
>+class FileChangeEvent : public nsRunnable
>+{
>+  nsCOMPtr<nsIObserver> mObs;
>+  nsCOMPtr<nsILocalFile> mFile;
>+  const char *mType;
>+public:
>+  FileChangeEvent(nsIObserver *aObserver, nsILocalFile *aFile, const char *aTypeString) :
>+    mObs(aObserver) , mFile(aFile), mType(aTypeString) {};
>+  
>+  NS_IMETHOD Run(){
>+    NS_ASSERTION(NS_IsMainThread(), "Not main thread!");
>+    return mObs->Observe(mFile, mType, nsnull);
>+  }
>+};
>+
>+
>+class nsFileWatcherService::FdObserverPair
>+{
>+public:
>+  int mWatchDescriptor;
>+  nsCOMPtr<nsIObserver> mObserver;
>+  FdObserverPair(){
>+    mWatchDescriptor = 0;
>+    mObserver = NULL;
>+    }
>+  
>+  FdObserverPair(int a, nsIObserver *b) : mWatchDescriptor(a) , mObserver(b) {};
>+};
>+  
>+nsIObserver *nsFileWatcherService::TableLookup(int wd, nsCString *buf)
>+{
>+  nsTArray<nsFileWatcherService::FdObserverPair *> *list = mTable.Get(*buf);
>+  for (unsigned int j=0; j<list->Length(); j++){
>+    if ((*list)[j]->mWatchDescriptor == wd)
>+      return (*list)[j]->mObserver;
>+  }
>+  return NULL;
>+}
>+void nsFileWatcherService::EventLoop()
>+{
>+  MOZ_ASSERT(!NS_IsMainThread());
>+  struct pollfd fds[2];// = (struct pollfd *)NS_Alloc(sizeof(struct pollfd)*2);
>+  while (1){
>+    fds[0]= {mInotifyFd, POLLIN, 0};
>+    fds[1]= {mReadFd, POLLIN, 0};
>+    int ret = poll(fds, 2, -1);
>+    if (ret<1){
>+              //TODO: Need to handle
>+    }
>+    if(fds[1].revents != 0){
>+      return;
>+    }
>+    unsigned int queue_len;
>+    ret = ioctl(mInotifyFd,FIONREAD, &queue_len);
>+    
>+    char *events = (char *)NS_Alloc(queue_len);
>+    ssize_t i=0, len = read(mInotifyFd, events,queue_len);
>+    if (len<0){
>+              //handle error
>+    }
>+    while (i<len){
>+      struct inotify_event *event = (struct inotify_event *) (events+i);
>+      
>+      bool dir = (event->len !=0);
>+      const char *type = NULL;
>+      if (event->mask & (IN_CREATE | IN_MOVED_TO)){
>+        type = created;
>+      }
>+      else if (event->mask & (IN_DELETE | IN_DELETE_SELF | IN_MOVED_FROM)){
>+        type = deleted;
>+      }
>+      else if (event->mask & (IN_MODIFY | IN_MOVE_SELF)){
>+        type = modified;
>+      }
>+      if (event->mask & IN_IGNORED){
>+        i+=sizeof(struct inotify_event) + event->len;
>+        continue; 
>+                //if the watch has been removed, the data is no longer in the mTables, so ignore
>+      }
>+      if (event->mask & IN_Q_OVERFLOW){
>+                //TODO: handle this
>+      }
>+      PR_Lock(mTableLock);
>+      PR_Lock(mLookupLock);
>+      char *storedName = mWdLookup.Get(event->wd);
>+      nsCString str;
>+      if (storedName == NULL){ 
>+                //if the watch has been removed, but we haven't gotten an IGNORED event just yet, move along.
>+        PR_Unlock(mTableLock);
>+        PR_Unlock(mLookupLock);
>+        continue;
>+      }
>+      str.Append(storedName,strlen(storedName));
>+      nsIObserver *obs = TableLookup(event->wd, &str);
>+      
>+      nsCString fullName;
>+      fullName.Assign(storedName);
>+      nsCOMPtr<nsILocalFile> file;
>+      NS_NewNativeLocalFile(fullName,true,getter_AddRefs(file));
>+      if (dir){
>+        nsCString leafName;
>+        leafName.Assign(event->name);
>+        file->AppendRelativeNativePath(leafName);
>+      }
>+      PR_Unlock(mTableLock);
>+      PR_Unlock(mLookupLock);
>+      NS_DispatchToMainThread(new FileChangeEvent(obs,file,type));
>+      i+=sizeof(struct inotify_event) + event->len;
>+    }
>+    NS_Free(events);
>+    
>+  }
>+}
>+
>+nsFileWatcherService::nsFileWatcherService()
>+{
>+  Init();
>+}
>+NS_METHOD nsFileWatcherService::Init(){
>+  mInotifyFd = mReadFd = mShutdownFd = -1;
>+  mTable.Init();
>+  mWdLookup.Init();
>+  mInotifyFd = inotify_init();
>+  if(mInotifyFd<0){
>+    return NS_ERROR_FAILURE;
>+  }
>+  int arr[2];
>+  int result = pipe(arr);
>+  if(result<0){
>+    close(mInotifyFd);
>+    return NS_ERROR_FAILURE;
>+  }
>+  mReadFd=arr[0];
>+  mShutdownFd=arr[1];
>+  mTableLock = PR_NewLock();
>+  mLookupLock = PR_NewLock();
>+  nsRefPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &nsFileWatcherService::EventLoop);
>+  if(!r){
>+    close(mReadFd);
>+    close(mShutdownFd);
>+    close(mInotifyFd);
>+    return NS_ERROR_FAILURE;
>+  }
>+  NS_NewThread(getter_AddRefs(mWorkerThread),r);
>+  
>+  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
>+  if(!observerService){
>+    write(mShutdownFd,"shutdown",9);
>+    close(mShutdownFd);
>+    close(mReadFd);
>+    close(mInotifyFd);
>+    return NS_ERROR_FAILURE;
>+  }
>+  observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
>+  return NS_OK;
>+}
>+
>+nsFileWatcherService::~nsFileWatcherService()
>+{
>+  if(mShutdownFd!=-1)
>+    write(mShutdownFd,"shutdown",9);
>+  if(mShutdownFd !=-1)
>+    close(mShutdownFd);
>+  if(mReadFd !=-1)
>+    close(mReadFd);
>+  if(mInotifyFd !=-1)
>+    close(mInotifyFd);
>+}
>+
>+/** void addFileWatcher (in nsIFile aFile, in nsIObserver aObserver); 
>+ *  Note: this code makes many linux-specific assumptions.
>+ *  Also note: this code does not function if both 1) aFile is a file,
>+ *  not a directory and 2) aFile does not exist yet.  In other words,
>+ *  you cannot add a watcher for a non-existent file and then watch
>+ *  for its creation.
>+ */
>+NS_IMETHODIMP nsFileWatcherService::AddFileWatcher(nsIFile *aFile, nsIObserver *aObserver)
>+{
>+  nsCString path;
>+  aFile->GetNativePath(path);
>+  const char *tmpbuf = path.get();
>+  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
>+  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);
>+  
>+  PR_Lock(mTableLock);
>+  int wd = inotify_add_watch(mInotifyFd, buf, IN_CREATE | IN_DELETE | IN_DELETE_SELF |
>+                             IN_MODIFY | IN_MOVE | IN_MOVE_SELF);
>+  FdObserverPair *fopair = new FdObserverPair(wd, aObserver);
>+  nsTArray<FdObserverPair *> *arr = mTable.Get(path);
>+  if (!arr){
>+    arr = new nsTArray<FdObserverPair *>();
>+    mTable.Put(path,arr);
>+  }
>+  arr->AppendElement(fopair);
>+  PR_Lock(mLookupLock);
>+  PR_Unlock(mTableLock);
>+  mWdLookup.Put(wd,buf);
>+  PR_Unlock(mLookupLock);
>+  bool isdir;
>+  aFile->IsDirectory(&isdir);
>+  if (!isdir)
>+    return NS_OK;
>+
>+//inotify only watches the given directory, not the subdirectories, so we add those watches manually.  
>+
>+  nsCOMPtr<nsISimpleEnumerator> enumerator;
>+  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));
>+  bool hasMore;
>+  enumerator->HasMoreElements(&hasMore);
>+  while (hasMore){
>+    nsCOMPtr<nsIFile> subfile;
>+    enumerator->GetNext(getter_AddRefs(subfile));
>+    bool issym;
>+    subfile->IsDirectory(&isdir);
>+    subfile->IsSymlink(&issym);
>+    if (isdir && !issym)
>+      AddFileWatcher(subfile,aObserver);
>+    enumerator->HasMoreElements(&hasMore);
>+  }
>+  
>+  return NS_OK;
>+}
>+
>+/* void removeFileWatcher (in nsIFile aFile, in nsIObserver aObserver); */
>+NS_IMETHODIMP nsFileWatcherService::RemoveFileWatcher(nsIFile *aFile, nsIObserver *aObserver)
>+{
>+  nsCString path;
>+  aFile->GetNativePath(path);
>+  PR_Lock(mTableLock);
>+  nsTArray<FdObserverPair *> *arr= mTable.Get(path);
>+  if (arr==nsnull)
>+    return NS_OK; //TODO: this probably should do something besides silently fail
>+  int wd=-1;
>+  for (unsigned int i=0;i<arr->Length();i++){
>+    if ((*arr)[i]->mObserver==aObserver){
>+      wd = (*arr)[i]->mWatchDescriptor;
>+      inotify_rm_watch(mInotifyFd, wd);
>+      delete (*arr)[i];
>+      arr->RemoveElementAt(i);
>+      break;
>+    }
>+  }
>+  if (arr->Length() == 0)
>+    mTable.Remove(path);
>+  PR_Lock(mLookupLock);
>+  PR_Unlock(mTableLock);
>+  char *buf = mWdLookup.Get(wd);
>+  NS_Free(buf);
>+  mWdLookup.Remove(wd);
>+  PR_Unlock(mLookupLock);
>+  bool isdir;
>+  aFile->IsDirectory(&isdir);
>+  if (!isdir)
>+    return NS_OK;
>+  nsCOMPtr<nsISimpleEnumerator> enumerator;
>+  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));
>+  bool hasMore;
>+  enumerator->HasMoreElements(&hasMore);
>+  while (hasMore){
>+    nsCOMPtr<nsIFile> subfile;
>+    enumerator->GetNext(getter_AddRefs(subfile));
>+    bool isdir;
>+    bool issym;
>+    subfile->IsDirectory(&isdir);
>+    subfile->IsSymlink(&issym);
>+    if (isdir && !issym)
>+      RemoveFileWatcher(subfile,aObserver);
>+    enumerator->HasMoreElements(&hasMore);
>+  }
>+  return NS_OK;
>+}
>+
>+NS_METHOD
>+nsFileWatcherService::nsFileWatcherServiceConstructor(nsISupports *outer, REFNSIID iid, void **result)
>+{
>+  NS_ENSURE_ARG_POINTER(result);
>+  NS_ENSURE_NO_AGGREGATION(outer);
>+  
>+  nsRefPtr<nsFileWatcherService> inst = new nsFileWatcherService();
>+  
>+  return inst->QueryInterface(iid, result);
>+}
>+
>+NS_IMETHODIMP nsFileWatcherService::Observe(nsISupports *support,  const char *topic, const PRUnichar *aData)
>+{
>+  if (!nsCRT::strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)){
>+    if(mShutdownFd!=-1)
>+      write(mShutdownFd,"shutdown",9);
>+    if(mShutdownFd !=-1)
>+      close(mShutdownFd);
>+    if(mReadFd !=-1)
>+      close(mReadFd);
>+    if(mInotifyFd !=-1)
>+      close(mInotifyFd);
>+  }
>+  return NS_OK;
>+}
>diff --git a/xpcom/io/nsFileWatcherService.h b/xpcom/io/nsFileWatcherService.h
>new file mode 100755
>--- /dev/null
>+++ b/xpcom/io/nsFileWatcherService.h
>@@ -0,0 +1,59 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef _NS_FILE_WATCHER_SERVICE_h_
>+#define _NS_FILE_WATCHER_SERVICE_h_
>+
>+#include "nsIFileWatcherService.h"
>+#include "nsClassHashtable.h"
>+#include "nsDataHashtable.h"
>+#include "nsHashKeys.h"
>+#include "nsTArray.h"
>+#include "nsIThread.h"
>+#include "prlock.h"
>+#include "nsIObserver.h"
>+#include "mozilla/Mutex.h"
>+
>+#define NS_FILE_WATCHER_SERVICE_CLASSNAME "nsFileWatcherService"
>+
>+#define NS_FILE_WATCHER_SERVICE_CONTRACTID "@mozilla.org/filewatch;1"
>+
>+#define NS_FILE_WATCHER_SERVICE_CID {0x879618e4, 0xd419, 0x468e, {0x8a, 0x33, 0x0e, 0x35, 0xa1, 0x0e, 0x6b, 0xb0}}
>+
>+NS_DEFINE_STATIC_CID_ACCESSOR(NS_FILE_WATCHER_SERVICE_CID)
>+
>+class nsFileWatcherService : public nsIFileWatcherService, public nsIObserver
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+  NS_DECL_NSIFILEWATCHERSERVICE
>+  
>+  nsFileWatcherService();
>+  NS_METHOD Init();
>+  
>+  static nsresult nsFileWatcherServiceConstructor(nsISupports *, REFNSIID, void **);
>+
>+  private:
>+  ~nsFileWatcherService();
>+
>+  protected:
>+  class FdObserverPair;
>+  void EventLoop();
>+
>+  int mInotifyFd;
>+  int mShutdownFd;
>+  int mReadFd;
>+  nsClassHashtable<nsCStringHashKey, nsTArray<FdObserverPair *> >mTable;
>+  nsDataHashtable<nsUint64HashKey,char *>mWdLookup;
>+  nsIObserver *TableLookup(int, nsCString *);
>+  PRLock *mTableLock, *mLookupLock;
>+  
>+  nsCOMPtr<nsIThread> mWorkerThread;
>+  
>+};
>+
>+
>+#endif
>diff --git a/xpcom/io/nsIFileWatcherService.idl b/xpcom/io/nsIFileWatcherService.idl
>new file mode 100755
>--- /dev/null
>+++ b/xpcom/io/nsIFileWatcherService.idl
>@@ -0,0 +1,32 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+#include "nsIFile.idl"
>+#include "nsIObserver.idl"
>+
>+[scriptable, builtinclass, uuid(add0ae9f-5460-404b-8037-1390a8fc4292)]
>+interface nsIFileWatcherService : nsISupports
>+{
>+    /**
>+     *  addFileWatcher  
>+     *
>+     *  Adds an nsIObserver to the service, and notifies it of changes
>+     *  that occur to a particular nsIFile.  The observe method of
>+     *  aObserver will be called every time the file changes. The file
>+     *  that changed will be passed in as the aSubject, and the type
>+     *  of change will be passed in as aTopic; all changes are one of
>+     *  "created", "deleted", and "modified".  If the nsIFile is a
>+     *  directory, all files will be watched recursively, not
>+     *  following symlinks.
>+     */
>+    void addFileWatcher(in nsIFile aFile, in nsIObserver aObserver);
>+    /**
>+     *  removeFileWatcher
>+     *
>+     *  Stops notifying the given nsIObserver about the given nsIFile.
>+     */
>+    void removeFileWatcher(in nsIFile aFile, in nsIObserver aObserver);
>+};
>diff --git a/xpcom/tests/unit/test_fwsadd.js b/xpcom/tests/unit/test_fwsadd.js
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/tests/unit/test_fwsadd.js
>@@ -0,0 +1,40 @@
>+/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ * This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+function cleanup() {
>+  var file = do_get_cwd();
>+  var fws = Cc["@mozilla.org/filewatch;1"].getService(Ci.nsIFileWatcherService);
>+  fws.removeFileWatcher(file,observer);
>+  do_get_file('fwstestfile').remove(false);
>+}
>+
>+var observer = {
>+  observe: function observe(subject, topic, data){
>+    subject = subject.QueryInterface(Ci.nsIFile);
>+    do_check_true(subject.leafName == "fwstestfile");
>+    do_check_eq(topic,'created');
>+    do_test_finished();
>+  }
>+};
>+
>+function create(){
>+  var file = do_get_file('fwstestfile', true);
>+  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0777);
>+}
>+
>+function run_test(){
>+  do_register_cleanup(cleanup);
>+  var file = do_get_cwd();
>+  var fws = Cc["@mozilla.org/filewatch;1"].getService(Ci.nsIFileWatcherService);
>+  print(file.path);
>+  fws.addFileWatcher(file,observer);
>+  do_timeout(0,create);
>+  do_test_pending();
>+}
>+
>+    
>\ No newline at end of file
>diff --git a/xpcom/tests/unit/test_fwsdel.js b/xpcom/tests/unit/test_fwsdel.js
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/tests/unit/test_fwsdel.js
>@@ -0,0 +1,34 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ * This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+var observer = {
>+  observe: function(subject, topic, data){
>+    subject = subject.QueryInterface(Ci.nsIFile);
>+    do_check_eq(subject.leafName, "fwstestfile");
>+    do_check_eq(topic, "deleted");
>+    var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
>+    fws.removeFileWatcher(do_get_file('fwdtestfile', true),observer);
>+    do_test_finished();
>+  }
>+};
>+
>+function deletes(){
>+  var file = do_get_file('fwstestfile');
>+  file.remove(false);
>+}
>+
>+function run_test(){
>+  var file = do_get_file('fwstestfile', true);
>+  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0777);
>+  var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
>+  fws.addFileWatcher(file,observer);
>+  do_timeout(0,deletes);
>+  do_test_pending();
>+}
>+    
>+    
>\ No newline at end of file
>diff --git a/xpcom/tests/unit/test_fwsmod.js b/xpcom/tests/unit/test_fwsmod.js
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/tests/unit/test_fwsmod.js
>@@ -0,0 +1,49 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ * This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+Components.utils.import("resource://gre/modules/NetUtil.jsm");  
>+Components.utils.import("resource://gre/modules/FileUtils.jsm");
>+
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+function cleanup(){
>+  var file= do_get_file('fwstestfile');
>+  var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
>+  fws.removeFileWatcher(file,observer);
>+  file.remove(false);
>+}
>+
>+var observer = {
>+  observe: function(subject, topic, data){
>+    subject = subject.QueryInterface(Ci.nsIFile);
>+    do_check_eq(subject.leafName,"fwstestfile");
>+    do_check_eq(topic,'modified');
>+    do_test_finished();
>+  }
>+};
>+
>+function deletes(){
>+  var file = do_get_file('fwstestfile');
>+  var foStream = Cc["@mozilla.org/network/file-output-stream;1"].  
>+    createInstance(Ci.nsIFileOutputStream);
>+  foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0);
>+  foStream.write("foo",3);
>+  foStream.close();
>+}
>+
>+function run_test(){
>+  do_register_cleanup(cleanup);
>+  var file = do_get_file('fwstestfile', true);
>+  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0777);
>+  var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
>+  fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
>+  fws.addFileWatcher(file,observer);
>+  do_timeout(0,deletes);
>+  do_test_pending();
>+}
>+    
>+    
>\ No newline at end of file
>diff --git a/xpcom/tests/unit/xpcshell.ini b/xpcom/tests/unit/xpcshell.ini
>--- a/xpcom/tests/unit/xpcshell.ini
>+++ b/xpcom/tests/unit/xpcshell.ini
>@@ -48,3 +48,9 @@
> [test_windows_shortcut.js]
> [test_bug745466.js]
> skip-if = os == "win"
>+run-if = os == "linux"
>+[test_fwsadd.js]
>+run-if = os == "linux"
>+[test_fwsmod.js]
>+run-if = os == "linux"
>+[test_fwsdel.js]

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 630644 [details] [diff] [review]
Repeat of patch after first revision

Review of attachment 630644 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good.  Keep it up.

Change the license of the test_( files to this:

/* Any copyright is dedicated to the Public Domain.
   http://creativecommons.org/publicdomain/zero/1.0/ */

::: xpcom/build/XPCOMModule.inc
@@ +66,5 @@
>  
>      COMPONENT(UUID_GENERATOR, nsUUIDGeneratorConstructor)
>  
> +    COMPONENT(FILE_WATCHER_SERVICE, nsFileWatcherService::nsFileWatcherServiceConstructor)
> +

make sure that there isn't an extra space on this new line.

::: xpcom/io/nsFileWatcherService.cpp
@@ +34,5 @@
> +NS_IMPL_THREADSAFE_ISUPPORTS1(nsFileWatcherService, nsIFileWatcherService)
> +
> +const char *created = "created";
> +const char *deleted = "deleted";
> +const char *modified = "modified";

use this instead:
   static const char *const type[] = "blahblash"

@@ +43,5 @@
> +  nsCOMPtr<nsILocalFile> mFile;
> +  const char *mType;
> +public:
> +  FileChangeEvent(nsIObserver *aObserver, nsILocalFile *aFile, const char *aTypeString) :
> +    mObs(aObserver) , mFile(aFile), mType(aTypeString) {};

space before the comma


you'll have to make a copy of aTypeString, or use nsCString.  nsCString is better.

@@ +50,5 @@
> +    NS_ASSERTION(NS_IsMainThread(), "Not main thread!");
> +    return mObs->Observe(mFile, mType, nsnull);
> +  }
> +};
> +

extra white space

@@ +59,5 @@
> +  int mWatchDescriptor;
> +  nsCOMPtr<nsIObserver> mObserver;
> +  FdObserverPair(){
> +    mWatchDescriptor = 0;
> +    mObserver = NULL;

nsnull is preferred over NULL.

@@ +73,5 @@
> +    if ((*list)[j]->mWatchDescriptor == wd)
> +      return (*list)[j]->mObserver;
> +  }
> +  return NULL;
> +}

add a white space line.

@@ +152,5 @@
> +}
> +
> +nsFileWatcherService::nsFileWatcherService()
> +{
> +  Init();

I wouldn't do this here.  Instead, have your nsFileWatcherService::nsFileWatcherServiceConstructor method call Init().  nsFileWatcherService::nsFileWatcherServiceConstructor can return a nsresult.  Just pass back whatever Init() returns.

@@ +154,5 @@
> +nsFileWatcherService::nsFileWatcherService()
> +{
> +  Init();
> +}
> +NS_METHOD nsFileWatcherService::Init(){

nsresult nsFileWatcherService::Init() {

@@ +155,5 @@
> +{
> +  Init();
> +}
> +NS_METHOD nsFileWatcherService::Init(){
> +  mInotifyFd = mReadFd = mShutdownFd = -1;

Set these in your constructor

@@ +169,5 @@
> +    close(mInotifyFd);
> +    return NS_ERROR_FAILURE;
> +  }
> +  mReadFd=arr[0];
> +  mShutdownFd=arr[1];

add spaces around =

@@ +179,5 @@
> +    close(mShutdownFd);
> +    close(mInotifyFd);
> +    return NS_ERROR_FAILURE;
> +  }
> +  NS_NewThread(getter_AddRefs(mWorkerThread),r);

test for result here.

@@ +186,5 @@
> +  if(!observerService){
> +    write(mShutdownFd,"shutdown",9);
> +    close(mShutdownFd);
> +    close(mReadFd);
> +    close(mInotifyFd);

so, instead of doing all of this clean up stuff on error -- you could just defer it to the destructor.  ~() will have to close any open fd anyway, right..

@@ +195,5 @@
> +}
> +
> +nsFileWatcherService::~nsFileWatcherService()
> +{
> +  if(mShutdownFd!=-1)

crazy style rule is:

if () {
  stmt;
}

@@ +218,5 @@
> +  nsCString path;
> +  aFile->GetNativePath(path);
> +  const char *tmpbuf = path.get();
> +  char *buf = (char *)NS_Alloc(strlen(tmpbuf)+1);
> +  strncpy(buf,tmpbuf,strlen(tmpbuf)+1);

I do not understand why you need a |buf| here?  Can't you use tmpbuf directly?

@@ +226,5 @@
> +                             IN_MODIFY | IN_MOVE | IN_MOVE_SELF);
> +  FdObserverPair *fopair = new FdObserverPair(wd, aObserver);
> +  nsTArray<FdObserverPair *> *arr = mTable.Get(path);
> +  if (!arr){
> +    arr = new nsTArray<FdObserverPair *>();

On shutdown, do we have to release all of these objects in the nsTArray?

@@ +231,5 @@
> +    mTable.Put(path,arr);
> +  }
> +  arr->AppendElement(fopair);
> +  PR_Lock(mLookupLock);
> +  PR_Unlock(mTableLock);

I do not understand the locking pattern.  Could you explain what your doing?

@@ +239,5 @@
> +  aFile->IsDirectory(&isdir);
> +  if (!isdir)
> +    return NS_OK;
> +
> +//inotify only watches the given directory, not the subdirectories, so we add those watches manually.  

DO you know if this is an expensive operation?  Should this be done on the worker thread?

@@ +315,5 @@
> +  NS_ENSURE_ARG_POINTER(result);
> +  NS_ENSURE_NO_AGGREGATION(outer);
> +  
> +  nsRefPtr<nsFileWatcherService> inst = new nsFileWatcherService();
> +  

As I mentioned above, after you create the object, call inst->Init() and return its value on failure.

@@ +321,5 @@
> +}
> +
> +NS_IMETHODIMP nsFileWatcherService::Observe(nsISupports *support,  const char *topic, const PRUnichar *aData)
> +{
> +  if (!nsCRT::strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)){

oh... hmm. 

instead of duplicating what should be in the destructor... maybe have a Shutdown() method?  Have your ~() call that.

::: xpcom/io/nsFileWatcherService.h
@@ +31,5 @@
> +  NS_DECL_NSIOBSERVER
> +  NS_DECL_NSIFILEWATCHERSERVICE
> +  
> +  nsFileWatcherService();
> +  NS_METHOD Init();

nsresult Init();

@@ +38,5 @@
> +
> +  private:
> +  ~nsFileWatcherService();
> +
> +  protected:

So, this still is really linux/inotify specific.  We are going to have to wrap it with #ifdef's or move it into a new file so that it will compile on other platforms.

@@ +48,5 @@
> +  int mReadFd;
> +  nsClassHashtable<nsCStringHashKey, nsTArray<FdObserverPair *> >mTable;
> +  nsDataHashtable<nsUint64HashKey,char *>mWdLookup;
> +  nsIObserver *TableLookup(int, nsCString *);
> +  PRLock *mTableLock, *mLookupLock;

Use mozilla::Mutex

@@ +54,5 @@
> +  nsCOMPtr<nsIThread> mWorkerThread;
> +  
> +};
> +
> +

kill the extra line.

::: xpcom/io/nsIFileWatcherService.idl
@@ +21,5 @@
> +     *  "created", "deleted", and "modified".  If the nsIFile is a
> +     *  directory, all files will be watched recursively, not
> +     *  following symlinks.
> +     */
> +    void addFileWatcher(in nsIFile aFile, in nsIObserver aObserver);

new line after this method.

::: xpcom/tests/unit/test_fwsadd.js
@@ +8,5 @@
> +
> +function cleanup() {
> +  var file = do_get_cwd();
> +  var fws = Cc["@mozilla.org/filewatch;1"].getService(Ci.nsIFileWatcherService);
> +  fws.removeFileWatcher(file,observer);

space after comma.  other places too.

@@ +13,5 @@
> +  do_get_file('fwstestfile').remove(false);
> +}
> +
> +var observer = {
> +  observe: function observe(subject, topic, data){

space after closing )

@@ +30,5 @@
> +function run_test(){
> +  do_register_cleanup(cleanup);
> +  var file = do_get_cwd();
> +  var fws = Cc["@mozilla.org/filewatch;1"].getService(Ci.nsIFileWatcherService);
> +  print(file.path);

remove print()

::: xpcom/tests/unit/test_fwsdel.js
@@ +10,5 @@
> +  observe: function(subject, topic, data){
> +    subject = subject.QueryInterface(Ci.nsIFile);
> +    do_check_eq(subject.leafName, "fwstestfile");
> +    do_check_eq(topic, "deleted");
> +    var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);

you have to make sure you don't call createInstance.  (same in a few other places)

@@ +11,5 @@
> +    subject = subject.QueryInterface(Ci.nsIFile);
> +    do_check_eq(subject.leafName, "fwstestfile");
> +    do_check_eq(topic, "deleted");
> +    var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
> +    fws.removeFileWatcher(do_get_file('fwdtestfile', true),observer);

instead of calling do_get_file() here, can you just use subject?

::: xpcom/tests/unit/test_fwsmod.js
@@ +5,5 @@
> +
> +Components.utils.import("resource://gre/modules/NetUtil.jsm");  
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +
> +

one white space line.

@@ +29,5 @@
> +function deletes(){
> +  var file = do_get_file('fwstestfile');
> +  var foStream = Cc["@mozilla.org/network/file-output-stream;1"].  
> +    createInstance(Ci.nsIFileOutputStream);
> +  foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0);

probably a good idea to say what these flags are.

@@ +39,5 @@
> +  do_register_cleanup(cleanup);
> +  var file = do_get_file('fwstestfile', true);
> +  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0777);
> +  var fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);
> +  fws = Cc["@mozilla.org/filewatch;1"].createInstance(Ci.nsIFileWatcherService);

why twice?  Use .getService()

Comment 9

[Paul Dagnelie]

Attachment: Patch after second revision

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.3

bsmedberg, could you provide feedback on this patch?  Linux and Mac are nearly done.  Windows still needs to be implemented.

Comment 11

[Benjamin Smedberg]

Comment on attachment 632272 [details] [diff] [review]
patch v.3

In terms of API:

* Why is this a new service instead of just a method on nsIFile? e.g. nsIFile.watch(observer) and nsIFile.unwatch(observer)
* What does "modified" mean? Does it mean that the file metadata changed, or the file contents have changed?
* If some systems (mac) don't know the type of change, why bother having different notification types, since all the clients are going to have to do all their checking via modified anyway?
* I think you'd be better off inventing a new observer specifically for this rather than reusing the too-generic nsIObserver.
* When watching a directory, will any notifications be sent for the directory itself or only for file children of the directory?
* What kinds of notifications are sent for symlinks?

In terms of impl:

I don't understand nsFileWatcherServiceWin: it's not implemented, right? Why does it exist at all?

I've got a bunch of style nits, but I want to understand the API and core first. Also I'm not the right person to do the actual review of either the *nix or mac implementation details, do you have a suggested reviewer for that code?

Comment 12

[Doug Turner (:dougt)]

> Why is this a new service instead of just a method on nsIFile? e.g. nsIFile.watch(observer) and nsIFile.unwatch(observer)

We could add it to the nsIFile, but figured that is a frozen enough api that I didn't want to mess with it.

> What does "modified" mean? Does it mean that the file metadata changed, or the file contents have changed?

As it turns out, we might not be able to determine what changed with a given file.  It might be created, deleted, or modified (its content, or meta data).  This is true for cocoa in my initial testing.   I am considering dropping these types - and instead, you'll be notified that "something" changed.

> If some systems (mac) don't know the type of change, why bother having different notification types, since all the clients are going to have to do all their checking via modified anyway?

Yup.  See above. :)

> I think you'd be better off inventing a new observer specifically for this rather than reusing the too-generic nsIObserver.

The initial consumer already implemented nsIObserver, so I thought that was going the way to go.  Given that we are going to probably drop 'topic', you're probably more right now. ;) 

> When watching a directory, will any notifications be sent for the directory itself or only for file children of the directory?

I think we want to say only for the children of that directory.  Things get fun when the directory being watched moves.  We'll add that to the IDL comment.

> nsFileWatcherServiceWin

Yeah, paul is still working on it.

> *nix and mac reviews

Yeah, we'll have additional reviews on it.  Not sure who yet.

Comment 13

[Benjamin Smedberg]

Comment on attachment 632272 [details] [diff] [review]
patch v.3

I'm going to provide some code/style nits, but per IRC discussion I'd like the API changes made:

* Don't use an XPCOM service, just add nsIFile.watch/unwatch
* Use a custom observer class instead of nsIObserver
* provide only a "something changed" API for cross-platform sanity

>+void
>+nsFileWatcherService::FileStreamCallback(ConstFSEventStreamRef streamRef,
>+                                         void *clientCallBackInfo,
>+                                         size_t numEvents,
>+                                         void *eventPaths,
>+                                         const FSEventStreamEventFlags eventFlags[],
>+                                         const FSEventStreamEventId eventIds[])
>+{
>+  int i;
>+  char **paths = (char**) eventPaths;
>+
>+  nsFileWatcherService* service = (nsFileWatcherService*) clientCallBackInfo;

In both lines, please use static_cast<char**>

>+
>+  for (i=0; i<numEvents; i++) {

Please add whitespace around the "<" operator.


>+void
>+nsFileWatcherService::PostNotification(const char* path, const char* topic) {

please put the opening brace in col 0 of the next line

>+  printf("%s in path %s\n", topic, path);

Remove debugging printfs before final patch ;-)

> +  mStreamContext = (FSEventStreamContext *) malloc(sizeof(FSEventStreamContext));

This seems odd, why can't use just use "new FSEventStreamContext"?

>+void
>+nsFileWatcherService::Shutdown()
>+{
>+  if (mStream) {
>+    FSEventStreamStop(mStream);
>+    FSEventStreamInvalidate(mStream);
>+    FSEventStreamRelease(mStream);
>+  }
>+
>+  if (mStream) {
>+    free((void*)mStreamContext);

With the comment above this just becomes "delete mStreamContext", but you don't need or want the cast here even with "free".

>diff --git a/xpcom/io/nsFileWatcherServiceCocoa.h b/xpcom/io/nsFileWatcherServiceCocoa.h

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In this and other modelines, please use "tab-width: 8" instead of 2, because we want to notice hard tabs that sneak into these files.

>+#ifndef _NS_FILE_WATCHER_SERVICE_COCOA_h_
>+#define _NS_FILE_WATCHER_SERVICE_COCOA_h_

nit, use "#ifndef nsFileWatcherServiceCocoa_h"

>diff --git a/xpcom/io/nsFileWatcherServiceUnix.cpp b/xpcom/io/nsFileWatcherServiceUnix.cpp


>+#ifdef MOZ_ENABLE_INOTIFY
>+// this is what we can use to ensure that we can compile on
>+// non supporting operating systems.
>+#endif

??

>+void
>+nsFileWatcherService::EventLoop()
>+{
>+  MOZ_ASSERT(!NS_IsMainThread());
>+  struct pollfd fds[2];
>+  fds[0] = {mInotifyFd, POLLIN, 0};
>+  fds[1] = {mShutdownReadFd, POLLIN, 0};
>+
>+  while (1) {

Make this "while (true)"

>+    int ret = poll(fds, 2, -1);
>+
>+    if (ret < 1 || fds[1].revents != 0) {
>+      return;
>+    }

Is this the code which joins the thread? I think a comment is probably in order here.

>+    char *events = (char *)NS_Alloc(queue_len);

Just use malloc/free here, NS_Alloc is just extra overhead. Or even better, nsAutoTArray<char> and "new char[queue_len]";

>+static PLDHashOperator
>+TableDestroyEnumerator(const nsACString_internal &aKey, nsAutoPtr<nsTArray<nsFileWatcherService::FdObserverPair *>> &aArray, void *aUserArg) {

you'll probably need to separate the close brackets into "> >" to satisfy C++ parsers.

>+nsresult
>+nsFileWatcherService::Init() {

brace position

>+  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread), r);

If I understand correctly what this thread is doing, I really don't think you need or want an XPCOM thread here, which are meant for threads with an event loop. Instead you should probably just be using PR_NewNamedThread.

Comment 14

[Paul Dagnelie]

Attachment: patch v.4

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> >+  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread), r);
> 
> If I understand correctly what this thread is doing, I really don't think
> you need or want an XPCOM thread here, which are meant for threads with an
> event loop. Instead you should probably just be using PR_NewNamedThread.

This is the thread running the event loop.  Also, there doesn't appear to be a PR_NewNamedThread in the codebase, and NS_NewNamedThread seems strictly more complicated than what is currently being used.

Comment 15

[Benjamin Smedberg]

The point was that it was the NSPR thread API not the XPCOM API. There is no need for that thread to run an XPCOM event loop when it's only ever going to run a single method. So instead it should be using PR_CreateThread and (it turns out that I was wrong about the method names) PR_SetCurrentThreadName.

Comment 16

[Paul Dagnelie]

Attachment: patch v.5

Comment 17

[Benjamin Smedberg]

Comment on attachment 635150 [details] [diff] [review]
patch v.5

>diff --git a/xpcom/io/nsFileWatcherService.h b/xpcom/io/nsFileWatcherService.h

>+#ifndef _NS_FILE_WATCHER_SERVICE_h_
>+#define _NS_FILE_WATCHER_SERVICE_h_

nit, please make this #ifndef nsFileWatcherService_h

>+#include "nsIFileWatcherService.h"

I don't think there is any reason to have an nsIFileWatcherService XPCOM interface, because it just confuses. Just declare functions in a namespace:

namespace mozilla {

nsresult WatchFile(nsIFile* aFile, nsIFileUpdateListener* aObserver);
nsresult UnwatchFile(nsIFile* aFile, nsIFileUpdateListener* aObserver);

Then the implementation can be entirely contained in the platform-specific implementation file (and this header should be renamed mozilla/FileWatcher.h).

Or if you're dedicated to the idea of a *class*, just declare an abstract class here and implement it in the platform-specific files.

>+// move to common header.
>+static const char *const created = "created";
>+static const char *const deleted = "deleted";
>+static const char *const modified = "modified";

Here and elsewhere there are remnants of the old observer topics, please remove.

As already noted, somebody who knows the mac APIs (probably smichaud) and *nix APIs (maybe karlt?) needs to do a detailed review of that impl.

>diff --git a/xpcom/io/nsFileWatcherServiceCocoa.h b/xpcom/io/nsFileWatcherServiceCocoa.h

>+#ifndef nsFIleWatcherServiceCocoa_h
>+#define nsFIleWatcherServiceCocoa_h

nit, weird casing

>+  nsInterfaceHashtable<nsStringHashKey, nsIMutableArray> mFileListeners;

nsIMutableArray is really not a good data structure for this. The code would be a lot cleaner if you just used a nsCOMArray<nsIFileUpdateListener> because you can just loop over the entries, avoiding all the QIs. So I think you want this to be a nsClassHashtable<nsStringHashKey, nsCOMArray<nsIFileUpdateListener> >

Also I wonder why for the mac and *nix impls this would be using a nsStringHashKey instead of a nsCStringHashKey, since file paths on those platforms are stored natively as narrow strings. It seems to just require extra conversions.

>diff --git a/xpcom/io/nsFileWatcherServiceUnix.cpp b/xpcom/io/nsFileWatcherServiceUnix.cpp


>+class FileChangeEvent : public nsRunnable
>+{
>+  nsCOMPtr<nsIFileUpdateListener> mListener;
>+  nsCOMPtr<nsIFile> mFile;
>+
>+public:
>+  FileChangeEvent(nsIFileUpdateListener *aListener, nsIFile *aFile) :
>+    mListener(aListener) , mFile(aFile) {
>+  };
>+  
>+  NS_IMETHOD Run(){
>+    NS_ASSERTION(NS_IsMainThread(), "Not main thread!");
>+    return mListener->Update(mFile);
>+  }
>+};

nits:

Here and in the other platform files, please put the public members first, then the privates

Please use this style for the constructor:
  Constructor(params)
    : mFoo(aFoo)
    , mBar(aBar)
  { }

>+NS_IMETHODIMP
>+nsFileWatcherService::AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool alert)

>+  char *buf = ToNewCString(path);

Why do you need "buf" here instead of just using path.get() ? And it doesn't look like you ever free it anyway.

For all three platforms, the Shutdown method needs to call PR_JoinThread.

>+nsresult
>+nsFileWatcherService::Init() {

nit, brace in column 0 of next line

>+  mTable.Init();
>+  mTableLock = new mozilla::Mutex("nsFileWatcherService::mTableLock");
>+
>+  
>+  mRunning = 1;
>+  mThread = (HANDLE)_beginthreadex(NULL, 0, EventLoop2,
>+                                   &mRunning, 0, NULL);

Why not continue to use PR_CreateThread here to be consistent?

>diff --git a/xpcom/io/nsFileWatcherServiceWin.h b/xpcom/io/nsFileWatcherServiceWin.h
>new file mode 100644
>--- /dev/null
>+++ b/xpcom/io/nsFileWatcherServiceWin.h
>@@ -0,0 +1,40 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef _NS_FILE_WATCHER_SERVICE_WIN_h_
>+#define _NS_FILE_WATCHER_SERVICE_WIN_h_
>+
>+#include "nsFileWatcherService.h"
>+#include "nsISupportsImpl.h"
>+#include "nsClassHashtable.h"
>+#include "nsHashKeys.h"
>+#include "nsTArray.h"
>+#include "mozilla/Mutex.h"
>+#include "nsIThread.h"
>+#include "windows.h"
>+
>+class nsFileWatcherService : public nsIFileWatcherService
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIFILEWATCHERSERVICE
>+  
>+  nsFileWatcherService();
>+  nsresult Init();
>+
>+  class CallbackData;
>+
>+  static nsFileWatcherService *GetService();
>+
>+private:
>+  virtual ~nsFileWatcherService();
>+protected:
>+  nsClassHashtable<nsStringHashKey, nsTArray<CallbackData *> >mTable;
>+  mozilla::Mutex *mTableLock;

I just noticed that in all these classes this is a pointer. Why wouldn't this just be a member mozilla::Mutex?

>+    /**
>+     *  watch
>+     *
>+     *  Watches this file for changes, or if this file is a directory,
>+     *  watch for changes in its children recursively, not
>+     *  dereferencing symlinks.  Multiple listeners can be installed
>+     *  at once, and all will be called when any appropriate changes
>+     *  are made.
>+     *
>+     *  @param listener
>+     *      the listener to call out to when the file updates
>+     *
>+     *  @return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST if the file
>+     *          doesn't exist, NS_NOT_AVAILABLE if there is an
>+     *          out-of-memory or other resource failure, NS_OK
>+     *          otherwise.
>+     */
>+    void watch(in nsIFileUpdateListener listener);

Potential issues to solve or document:

* If the path is a symlink to a file, what gets watched, the symlink or the target?
* If the path is a symlink to a directory, what gets watched, the symlink or the target directory?
* If a new directory is created under the target directory after the watch is installed, is that directory also recursively watched?

>+[scriptable, uuid(8968aaba-0f95-436c-8baf-7092ccaa814c)]
>+interface nsIFileUpdateListener : nsISupports

This interface should be [function] also, so that from JS you can just pass in a JS function and it will be automatically converted.

>diff --git a/xpcom/io/nsIFileWatcherService.idl b/xpcom/io/nsIFileWatcherService.idl

as noted above, this interface should be removed

Comment 18

[Paul Dagnelie]

Attachment: patch v.6

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> Comment on attachment 635150 [details] [diff] [review]
> patch v.5
> 
> >+NS_IMETHODIMP
> >+nsFileWatcherService::AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool alert)
> 
> >+  char *buf = ToNewCString(path);
> 
> Why do you need "buf" here instead of just using path.get() ? And it doesn't
> look like you ever free it anyway.

First question's answer: buf is stored into a long-living datastructure, and so needs to remain alive.  My understanding is basically that the string for this file's name will be freed when the file is, which could happen at any time.  Since get() simply returns the string's mData, when the string is freed, this buffer will be freed and that pointer will point to dead memory.  As such, we need to make a copy of it to use it.
Second: It is freed (albeit much later), in RemoveFileWatcher.

> 
> >+  mTable.Init();
> >+  mTableLock = new mozilla::Mutex("nsFileWatcherService::mTableLock");
> >+
> >+  
> >+  mRunning = 1;
> >+  mThread = (HANDLE)_beginthreadex(NULL, 0, EventLoop2,
> >+                                   &mRunning, 0, NULL);
> 
> Why not continue to use PR_CreateThread here to be consistent?

Because the method used to recieve notifications without polling requires that the thread in question sleep in an alertable state.  By dint of experimentation, I discovered PR_CreateThread doesn't create a thread that does that.

Comment 19

[Benjamin Smedberg]

Is the long-living datastructure just the mWdLookup hash? If so, why not just fix the hashtable to store a strong reference, such as:

nsDataHashtable<nsUint64HashKey, nsCString>.

That way you don't have to worry about manually freeing this string in all the right places and can just .Remove things from the hash.

As for _beginthreadex, what is actually different? It sure looks like NSPR uses the same _beginthreadex API to create its thread (although it does create the thread with CREATE_SUSPENDED and then call ResumeThread).

Comment 20

[Paul Dagnelie]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> 
> As for _beginthreadex, what is actually different? It sure looks like NSPR
> uses the same _beginthreadex API to create its thread (although it does
> create the thread with CREATE_SUSPENDED and then call ResumeThread).

I was trying to transition to it to test it again when i discovered the real issue: I need to have the thread's HANDLE to use QueueUserAPC, which is how I have to add files for the thread to watch.  Using nsIThread's Dispatch method won't wake a thread out of SleepEx, which is what the thread spins on, because only certain functions can be woken out of by the actual watch notifications.  I can't figure out a way using PRThread or nsIThread to get both of those to work, and both have to.

Comment 21

[Doug Turner (:dougt)]

bsmedberg, got any review time here?  Otherwise, can we move it to someone that has more time?

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 636954 [details] [diff] [review]
patch v.6

over to kyle.

Comment 23

[Paul Dagnelie]

Attachment: patch v.7

This is the most up-to-date version, having fixed a crash-on-close issue under windows and linux.

Comment 24

[Benjamin Smedberg]

I'm happy for khuey to complete the review if needed, but I'm still waiting for the reviews from the platform-specific reviewers; probably bbondy or jimm for Windows, smichaud for mac, and ... I don't know who the expert is for for linux, maybe karlt?

Comment 25

[Paul Dagnelie]

Attachment: patch v.7.1

A couple style fixes and removal of debug prints.

Comment 26

[Paul Dagnelie]

Attachment: patch v.7.2

Fixed cocoa crash bug, added reviewers for each platform.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 640346 [details] [diff] [review]
patch v.7.2

Review of attachment 640346 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should discuss the design of this a bit before anyone else spends time reviewing this.  There's a lot of stuff here that's scary (passing raw char*s around, manual AddRef/Release) which I think is unnecessary.  Also the threading model isn't very clear here.

Please add threadsafety assertions to relevant methods here.  It's very hard to tell what is supposed to run on what thread.  Use "NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");" for things that run on the main thread, and "NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");" for things that don't run on the main thread.  You did this in a couple places, but I'd like to see it everywhere.

You also seem to have some crazy indentation.  Please fix that.

::: xpcom/io/nsFileWatcherServiceUnix.cpp
@@ +51,5 @@
> +    { }
> +  
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Not main thread!");

Nit: Use "Wrong thread!" for the assertion message.  That seems to be what we've standardized on.

@@ +76,5 @@
> +  
> +  FdListenerPair(int a, nsIFileUpdateListener *b)
> +    : mWatchDescriptor(a)
> +    , mListener(b)
> +  { }

You should use MOZ_COUNT_CTOR/MOZ_COUNT_DTOR macros in the constructor/destructor of this class for leak tracking.

@@ +79,5 @@
> +    , mListener(b)
> +  { }
> +};
> +  
> +void

Add a '// static' comment so it's clear what's going on without referring back to the header.

@@ +82,5 @@
> +  
> +void
> +nsFileWatcherService::EventLoop(void *arg)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());

NS_ASSERTION please.

@@ +353,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +// move into class

Maybe you should do that ;-)

@@ +356,5 @@
> +
> +// move into class
> +nsFileWatcherService *gFileWatcher = nsnull;
> +
> +bool gShutdown = false;

Why do you need this?

@@ +363,5 @@
> +{
> +  if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    gShutdown = true;
> +    Shutdown();
> +    NS_IF_RELEASE(gFileWatcher);

How can this ever be null?

@@ +369,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +nsFileWatcherService * nsFileWatcherService::GetService()

// static

@@ +372,5 @@
> +
> +nsFileWatcherService * nsFileWatcherService::GetService()
> +{
> +
> +  if (gShutdown) {

Extra newline.

::: xpcom/io/nsFileWatcherServiceUnix.h
@@ +29,5 @@
> +  nsresult Init();
> +  void Shutdown();
> +  
> +  class FdListenerPair;
> +//  class FileChangeEvent;

No need for commented out forward decls.

@@ +38,5 @@
> +private:
> +  ~nsFileWatcherService();
> +  nsresult AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool alert);
> +
> +

Extra newline.

@@ +49,5 @@
> +          //This table goes from a path to a list of Observer/inotify watch pairs
> +  nsClassHashtable<nsCStringHashKey, nsTArray<FdListenerPair *> >mTable;
> +          //This table goes from an inotify watch to a file name
> +  nsClassHashtable<nsUint64HashKey, nsCString>mWdLookup;
> +  PRThread *mWorkerThread;

This should be an nsThread IMO.

::: xpcom/io/nsFileWatcherServiceWin.cpp
@@ +36,5 @@
> +      mLeafName(aLeafName),
> +      mDirPath(aDirPath),
> +      mOverlap(aOverlap)
> +    {
> +      mRefCnt = 1;

Nope!  Whoever is doing 'new CallbackData(...);' should addref, not the ctor.

@@ +41,5 @@
> +    }
> +  
> +  ~CallbackData()
> +  {
> +    NS_Free(mUpdateBuffer);

You should use an nsAutoPtr or something for mUpdateBuffer.

@@ +47,5 @@
> +
> +  void AddRef()
> +  {
> +    PR_AtomicIncrement(&mRefCnt);
> +  }

Please use NS_DECL_INLINE_THREADSAFE_REFCOUNTING(CallbackData) rather than these custom implementations.

Also, does this really need to be threadsafe?

@@ +66,5 @@
> +  FileChangeEvent(nsFileWatcherService::CallbackData *aData, char *aBuffer, DWORD aLen)
> +    : mData(aData),
> +      mLen(aLen)
> +    {
> +      mBuffer = aBuffer;

Stick this with the rest of the initializers?

@@ +67,5 @@
> +    : mData(aData),
> +      mLen(aLen)
> +    {
> +      mBuffer = aBuffer;
> +    };

Line up the braces with all the other functions.

@@ +73,5 @@
> +  ~FileChangeEvent()
> +  {
> +    NS_Free(mBuffer);
> +    mData->Release();
> +  }

You should use RAII classes here.  nsAutoPtr or similar for mBuffer, and an nsRefPtr for mData.

@@ +181,5 @@
> +  mTable.Init();
> +  mRunning = new int;
> +  *mRunning = 1;
> +  mThread = (HANDLE)_beginthreadex(NULL, 0, EventLoop2,
> +                                   mRunning, 0, NULL);

I may be wrong, but I don't see where you ever shut this thread down.

@@ +282,5 @@
> +}
> +
> +nsFileWatcherService *gFileWatcher;
> +
> +nsFileWatcherService * nsFileWatcherService::GetService()

// static

@@ +297,5 @@
> +nsFileWatcherService::Observe(nsISupports* aSupport, const char *aTopic, const PRUnichar *aData)
> +{
> +  if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    Shutdown();
> +    NS_IF_RELEASE(gFileWatcher);

How can gFileWatcher be null here?

::: xpcom/io/nsFileWatcherServiceWin.h
@@ +37,5 @@
> +private:
> +  virtual ~nsFileWatcherService();
> +protected:
> +  nsClassHashtable<nsStringHashKey, nsTArray<CallbackData *> >mTable;
> +  HANDLE mThread;

I really thing you should use an nsThread here if at all possible.

@@ +38,5 @@
> +  virtual ~nsFileWatcherService();
> +protected:
> +  nsClassHashtable<nsStringHashKey, nsTArray<CallbackData *> >mTable;
> +  HANDLE mThread;
> +  int *mRunning;

Why do you need a separately allocated int?

::: xpcom/io/nsIFile.idl
@@ +458,5 @@
> +     *  at once, and all will be called when any appropriate changes
> +     *  are made.  If a child directory is created, that directory
> +     *  will automatically be watched. If the file is a symlink to a
> +     *  directory or another file, the target will be watched for
> +     *  changes, not the link.

Please add a comment about what thread the notifications are delivered on.

@@ +486,5 @@
> +     *          watched with the given listener, NS_OK otherwise.
> +     */
> +    void unwatch(in nsIFileUpdateListener listener);
> +
> +};

Nit: Extraneous newline between unwatch and the end of the interface.

::: xpcom/io/nsLocalFileCommon.cpp
@@ +298,5 @@
> +    bool exist;
> +    this->Exists(&exist);
> +    if (!exist) {
> +      return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> +    }

I'm not thrilled by this.  This means every call to Watch triggers a stat (presumably on the main thread) right?

Comment 28

[Karl Tomlinson (:karlt)]

Something to think about:

It would be preferable if we could add fds to watch to the main thread's poll loop, rather than creating new threads just because there are more fds we want to watch.

The awkward thing is that the way fds are added to the main loop depends on the toolkit.  Bug 604039 also needed to watch fds.  It would be nice if we had a generic way to do this, but I recognize that is not what this bug is about.

Comment 29

[Paul Dagnelie]

Attachment: Linux + core patch 1.0

As of this patch, the code has been split into 3 pieces.  This one, a windows patch, and a mac patch.

Comment 30

[Paul Dagnelie]

Attachment: Windows patch 1.0

Comment 31

[Paul Dagnelie]

Attachment: Mac patch 1.0

Comment 32

[Doug Turner (:dougt)]

Comment on attachment 642060 [details] [diff] [review]
Linux + core patch 1.0

Review of attachment 642060 [details] [diff] [review]:
-----------------------------------------------------------------

karl can you continue to review the linux pieces?

::: xpcom/io/nsFileWatcherService.h
@@ +6,5 @@
> +
> +#ifndef nsFileWatcherService_h
> +#define nsFileWatcherService_h
> +
> +class nsFileWatcherServiceAbstract {

nsFileWatcherServiceAbstract -> nsFileWatcherService

::: xpcom/io/nsFileWatcherServiceStock.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsFileWatcherServiceStock_h

Stock -> Default

@@ +11,5 @@
> +class nsFileWatcherService : public nsFileWatcherServiceAbstract
> +{
> +public:
> +
> +  NS_SCRIPTABLE NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)

Why NS_SCRIPTABLE ?

@@ +14,5 @@
> +
> +  NS_SCRIPTABLE NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> +  {
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +  }

Space between methods.

@@ +20,5 @@
> +  {
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +  }
> +
> +  static nsFileWatcherService *GetService(){

{ on its own line.

::: xpcom/io/nsFileWatcherServiceUnix.cpp
@@ +39,5 @@
> +  FileChangeEvent(nsFileWatcherService *aFileWatcherService, int aWd, char *aName) :
> +    mFileWatcherService(aFileWatcherService) , mWd(aWd)
> +    {
> +      if (aName != nsnull) {
> +        mName = (char *)calloc(strlen(aName)+1,1);

why calloc here?  Can't you just use nsCString and friends?

@@ +52,5 @@
> +  
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsresult rv = mFileWatcherService->OnFileChange(mWd, mName);

mName.get() -- if you used a nsCString.

@@ +53,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsresult rv = mFileWatcherService->OnFileChange(mWd, mName);
> +    free(mName);

then you wouldn't have to free.

@@ +80,5 @@
> +    : mWatchDescriptor(a)
> +    , mListener(b)
> +  {
> +    MOZ_COUNT_CTOR(FdListenerPair);
> +  }

new line after here.

@@ +104,5 @@
> +  while (true) {
> +    int ret = poll(fds, 2, -1);
> +
> +    if (ret < 1 || fds[1].revents != 0) {
> +              //when we get a write on the pipe, shut down the thread

two space indent, even for comments.  space after //

@@ +123,5 @@
> +
> +      NS_ASSERTION(!(event->mask & IN_Q_OVERFLOW), "Overflow of inotify event queue");
> +      
> +      //if the watch has been removed, the data is no
> +      //longer in the mTables, so ignore

space after //

@@ +131,5 @@
> +        
> +      }
> +
> +      if (event->mask & IN_Q_OVERFLOW) {
> +        //TODO: handle this

what do you propose we do here?  Just drop the event, right? maybe just a NS_WARNING()?

@@ +213,5 @@
> +  if (observerService) {
> +    observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +  }
> +
> +  write(mShutdownWriteFd,"shutdown",9);

space after comma

@@ +332,5 @@
> +  if (arr->Length() == 0) {
> +    mTable.Remove(path);
> +  }
> +
> +  mWdLookup.Remove(wd); //TODO

what do you have to do here?

::: xpcom/io/nsFileWatcherServiceUnix.h
@@ +43,5 @@
> +
> +  int mInotifyFd;
> +  int mShutdownReadFd;
> +  int mShutdownWriteFd;
> +          //This table goes from a path to a list of Observer/inotify watch pairs

line up // with int.  add a space after the //

::: xpcom/io/nsIFile.idl
@@ +451,5 @@
> +
> +    /**
> +     *  watch
> +     *
> +     *  Watches this file for changes, or if this file is a directory,

this file is a directory,

this nsIFile is a directory,

::: xpcom/io/nsLocalFileCommon.cpp
@@ +291,5 @@
>  
>      return InitWithFile(targetFile);
>  }
> +
> +/* void watch (in nsIFileUpdateListener listener); */

remove the comment

@@ +292,5 @@
>      return InitWithFile(targetFile);
>  }
> +
> +/* void watch (in nsIFileUpdateListener listener); */
> +NS_IMETHODIMP nsLocalFile::Watch(nsIFileUpdateListener *listener)

NS_IMETHODIMP on its own line.

@@ +301,5 @@
> +    if (!fws) {
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +
> +    return fws->AddFileWatcher(this, listener);

if file doesn't exists, are we sure that nothing bad happens?

@@ +304,5 @@
> +
> +    return fws->AddFileWatcher(this, listener);
> +}
> +
> +/* void unwatch (in nsIFileUpdateListener listener); */

same

@@ +305,5 @@
> +    return fws->AddFileWatcher(this, listener);
> +}
> +
> +/* void unwatch (in nsIFileUpdateListener listener); */
> +NS_IMETHODIMP nsLocalFile::Unwatch(nsIFileUpdateListener *listener)

same.

Comment 33

[Doug Turner (:dougt)]

you're also leaking:

   TEST-PASS | automationutils.processLeakLog() | leaked 18446744073709551615 instances of FdListenerPair with size 16 bytes each (0 bytes total)

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 642062 [details] [diff] [review]
Windows patch 1.0

windows crashes on try:
    https://tbpl.mozilla.org/php/getParsedLog.php?id=13567485&tree=Try&full=1#error3

Comment 35

[Brian R. Bondy [:bbondy]]

Comment on attachment 642062 [details] [diff] [review]
Windows patch 1.0

Review of attachment 642062 [details] [diff] [review]:
-----------------------------------------------------------------

Started reviewing but am going to stop until the Windows crash is fixed. Please re-request after that fix.

::: xpcom/io/nsFileWatcherServiceWin.cpp
@@ +25,5 @@
> +  nsCOMPtr<nsIFileUpdateListener> mListener;
> +  HANDLE mDir;
> +  nsString mLeafName; //empty if watching directory
> +  nsString mDirPath;
> +  LPOVERLAPPED mOverlap;

nsAutoPtr, or delete mOverlap in the destructor

@@ +41,5 @@
> +  }
> +  
> +  ~CallbackData()
> +  {
> +    MOZ_COUNT_DTOR(CallbackData);

if (mDir != INVALID_HANDLE_VALUE) {
  CloseHandle(mDir);
}

::: xpcom/io/nsFileWatcherServiceWin.h
@@ +10,5 @@
> +#include "nsISupportsImpl.h"
> +#include "nsClassHashtable.h"
> +#include "nsHashKeys.h"
> +#include "nsTArray.h"
> +#include "mozilla/Mutex.h"

I don't think this is needed in the header file.

@@ +11,5 @@
> +#include "nsClassHashtable.h"
> +#include "nsHashKeys.h"
> +#include "nsTArray.h"
> +#include "mozilla/Mutex.h"
> +#include "nsIThread.h"

I don't think this is needed in the header file.

Comment 36

[Paul Dagnelie]

Attachment: Linux + core patch 2.0

Comment 37

[Karl Tomlinson (:karlt)]

Comment on attachment 642691 [details] [diff] [review]
Linux + core patch 2.0

I don't think you'll need my review as well as khuey's, but khuey let me know
if you want me to look at anything specific.

Some comments below from a quick look I took.

Would it be feasible to shutdown the reading thread and close the inotify fd
while there are no watches?
That could reduce the effect of the resources required for the extra thread.

>+  // inotify only watches the given directory, not the
>+  // subdirectories, so we add those watches manually
>+
>+  nsCOMPtr<nsISimpleEnumerator> enumerator;
>+  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

Are you sure you really want to monitor the whole hierarchy?
GetDirectoryEntries will block the (main) thread on disk read and there could
be many such calls?

>+    int ret = poll(fds, 2, -1);
>+
>+    if (ret < 1 || fds[1].revents != 0) {
>+      // when we get a write on the pipe, shut down the thread
>+      return;
>+    }

Need to handle poll returning -1 on EINTR.

>+    ssize_t len = read(self->mInotifyFd, events,queue_len);
>+    NS_ASSERTION(len, "Read from inotify fd error!");
>+
>+    while (i < len) {
>+
>+      struct inotify_event *event = (struct inotify_event *) (events+i);
>+
>+      NS_ASSERTION(!(event->mask & IN_Q_OVERFLOW), "Overflow of inotify event queue");

Use NS_ASSERTION only for things that should not happen unless the code is
broken.  I assume overflow can happen, so use NS_WARNING until this is
implemented.  What are the consequences of not handling overflow here?
Would it be simple to notify all watchers?

Not sure, given the read won't block, but may need to handle EINTR if the read
fails.

Add a space at ",queue_len".

Comment 38

[Paul Dagnelie]

Attachment: Linux + core patch 3.0

(In reply to Karl Tomlinson (:karlt) from comment #37)
> Comment on attachment 642691 [details] [diff] [review]
> Linux + core patch 2.0
 
> >+    ssize_t len = read(self->mInotifyFd, events,queue_len);
> >+    NS_ASSERTION(len, "Read from inotify fd error!");
> >+
> >+    while (i < len) {
> >+
> >+      struct inotify_event *event = (struct inotify_event *) (events+i);
> >+
> >+      NS_ASSERTION(!(event->mask & IN_Q_OVERFLOW), "Overflow of inotify event queue");
> 
> Use NS_ASSERTION only for things that should not happen unless the code is
> broken.  I assume overflow can happen, so use NS_WARNING until this is
> implemented.  What are the consequences of not handling overflow here?
> Would it be simple to notify all watchers?
We could notify all watchers, but we wouldn't be able to tell them what happened, since the interface is just "this file changed".  Since the queue overflowed, we couldn't know what file updated, so we wouldn't have a file to tell them.  Also, on a sample linux install, the maximum number of events that could be queued is 16384.  It seems unlikely that you'd generate that many notifications quickly enough that they wouldn't get processed.

> 
> Not sure, given the read won't block, but may need to handle EINTR if the
> read fails.
Non-blocking calls can't fail with EINTR, so that's okay.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 642796 [details] [diff] [review]
Linux + core patch 3.0

Review of attachment 642796 [details] [diff] [review]:
-----------------------------------------------------------------

Getting closer.

::: xpcom/io/Makefile.in
@@ +63,5 @@
>  endif # OS2
>  
> +ifeq ($(OS_ARCH),Linux)
> +CPPSRCS		+= nsFileWatcherServiceUnix.cpp
> +endif

Please reuse one of the existing ifdef blocks.

@@ +79,5 @@
>  		nsStorageStream.h \
>  		nsStringStream.h \
>  		nsStreamUtils.h \
>  		nsWildCard.h \
> +		nsFileWatcherService.h \

Does this really need to be exported?

::: xpcom/io/nsFileWatcherServiceUnix.cpp
@@ +17,5 @@
> +#include "nsIObserver.h"
> +#include "nsCRT.h"
> +#include "mozilla/Services.h"
> +#include "mozilla/Util.h"
> +#include "nsDebug.h"

I don't think you use all these headers.

@@ +44,5 @@
> +      }
> +      else {
> +        mName.Assign("");
> +      }
> +    };

These braces should only be indented two spaces, and there's a spurious semicolon here.

@@ +99,5 @@
> +  fds[1] = {self->mShutdownReadFd, POLLIN, 0};
> +  PR_SetCurrentThreadName("FileWatcherService Loop");
> +  
> +
> +  while (true) {

Spurious newline.

@@ +104,5 @@
> +    int ret = poll(fds, 2, -1);
> +
> +    if (ret < 1 || fds[1].revents != 0) {
> +      if (errno == EINTR)
> +        continue;

if (cond) {
  // single line
}

@@ +112,5 @@
> +
> +    unsigned int queue_len;
> +    ret = ioctl(self->mInotifyFd, FIONREAD, &queue_len);
> +    
> +    char *events = new char[queue_len];

nsAutoArrayPtr<char> events = ...

@@ +116,5 @@
> +    char *events = new char[queue_len];
> +    ssize_t i = 0;
> +    ssize_t len = read(self->mInotifyFd, events, queue_len);
> +    if(!len)
> +      NS_WARNING("Read from inotify fd error!");

here too.

@@ +120,5 @@
> +      NS_WARNING("Read from inotify fd error!");
> +
> +    while (i < len) {
> +
> +      struct inotify_event *event = (struct inotify_event *) (events+i);

Extra newline, and please use a C++ cast here.

@@ +123,5 @@
> +
> +      struct inotify_event *event = (struct inotify_event *) (events+i);
> +
> +      if(event->mask & IN_Q_OVERFLOW)
> +        NS_WARNING("Overflow of inotify event queue");

and here.

@@ +128,5 @@
> +      
> +      // if the watch has been removed, the data is no
> +      // longer in the mTables, so ignore
> +      if (event->mask & IN_IGNORED) {
> +        i+=sizeof(struct inotify_event) + event->len;

i += sizeof ...

@@ +131,5 @@
> +      if (event->mask & IN_IGNORED) {
> +        i+=sizeof(struct inotify_event) + event->len;
> +        continue; 
> +        
> +      }

Another spurious newline.

@@ +134,5 @@
> +        
> +      }
> +
> +      NS_DispatchToMainThread(new FileChangeEvent(self, event->wd, (event->len ? event->name : nsnull)));
> +      i+=sizeof(struct inotify_event) + event->len;

i += sizeof ...

@@ +136,5 @@
> +
> +      NS_DispatchToMainThread(new FileChangeEvent(self, event->wd, (event->len ? event->name : nsnull)));
> +      i+=sizeof(struct inotify_event) + event->len;
> +    }
> +    delete [] events;

And then this is not necessary.

@@ +141,5 @@
> +  }
> +}
> +
> +static PLDHashOperator
> +TableDestroyEnumerator(const nsACString_internal &aKey, nsAutoPtr<nsTArray<nsFileWatcherService::FdListenerPair *> > &aArray, void *aUserArg) {

should just be const nsACString&

@@ +142,5 @@
> +}
> +
> +static PLDHashOperator
> +TableDestroyEnumerator(const nsACString_internal &aKey, nsAutoPtr<nsTArray<nsFileWatcherService::FdListenerPair *> > &aArray, void *aUserArg) {
> +  for(unsigned int i=0;i<aArray->Length();) {

Please stick more spaces in there.

@@ +145,5 @@
> +TableDestroyEnumerator(const nsACString_internal &aKey, nsAutoPtr<nsTArray<nsFileWatcherService::FdListenerPair *> > &aArray, void *aUserArg) {
> +  for(unsigned int i=0;i<aArray->Length();) {
> +    delete (*aArray)[i];
> +    aArray->RemoveElementAt(i);
> +  }

This is extremely inefficient.  You end up shifting the array down (for no reason) on every element removal.

@@ +173,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  mNumWatches = 0;
> +
> +

Spurious newline.

@@ +215,5 @@
> +  if (result < 0) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  mShutdownReadFd = arr[0];
> +  mShutdownWriteFd = arr[1];

Why are you assigning uninitialized values to mShutdownRead/WriteFd?

@@ +252,5 @@
> +  return AddFileWatcherAlertable(aFile,aListener,false);
> +}
> +
> +NS_IMETHODIMP
> +nsFileWatcherService::AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool alert)

I really don't think that we should be running this function (and all the statting/IO it will do) on the main thread.

@@ +264,5 @@
> +    nsresult rv = SpawnWorker();
> +    NS_ENSURE_SUCCESS(rv,rv);
> +  }
> +
> +  nsCString *path = new nsCString();

You don't need to allocate this on the heap.

@@ +345,5 @@
> +      if (arr->Length() == 1) {
> +        inotify_rm_watch(mInotifyFd, wd);
> +      }
> +      delete (*arr)[i];
> +      arr->RemoveElementAt(i);

You just use an nsTArray<nsAutoPtr<FdListenerPair> >, and then when you RemoveElementAt it'll delete for you.

@@ +380,5 @@
> +      RemoveFileWatcher(subfile, aListener);
> +    }
> +    enumerator->HasMoreElements(&hasMore);
> +  }
> +  return NS_OK;

Again, I don't think we want to do all this statting/IO on the main thread.

@@ +383,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +nsRefPtr<nsFileWatcherService> gFileWatcher = nsnull;

Use a raw (but strong) pointer.

@@ +391,5 @@
> +nsFileWatcherService::Observe(nsISupports* aSupport, const char *aTopic, const PRUnichar *aData)
> +{
> +  if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    gShutdown = true;
> +    Shutdown();

Shouldn't Shutdown() set gShutdown?

::: xpcom/io/nsFileWatcherServiceUnix.h
@@ +22,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +
> +  NS_SCRIPTABLE NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener);
> +  NS_SCRIPTABLE NS_IMETHOD RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener);

Do these really need to be scriptable (or even virtual)?

::: xpcom/io/nsLocalFileCommon.cpp
@@ +306,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsLocalFile::Unwatch(nsIFileUpdateListener *listener)
> +{

Please assert that you're on the main thread here too.

Comment 40

[Karl Tomlinson (:karlt)]

>+  // inotify only watches the given directory, not the
>+  // subdirectories, so we add those watches manually
>+
>+  nsCOMPtr<nsISimpleEnumerator> enumerator;
>+  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

The other issue here is that the subdirectory may get moved out of the hierarchy.
In that situation, the watch never gets removed.

Comment 41

[Paul Dagnelie]

Attachment: Linux + core patch 4.0

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #39)
> Comment on attachment 642796 [details] [diff] [review]
> Linux + core patch 3.0
> 
> Review of attachment 642796 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: xpcom/io/Makefile.in
> @@ +63,5 @@
> >  endif # OS2
> >  
> > +ifeq ($(OS_ARCH),Linux)
> > +CPPSRCS		+= nsFileWatcherServiceUnix.cpp
> > +endif
> 
> Please reuse one of the existing ifdef blocks.
None of the others actually only gets included on linux.  Since inotify only works on linux, I need to use a new one.

> @@ +215,5 @@
> > +  if (result < 0) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  mShutdownReadFd = arr[0];
> > +  mShutdownWriteFd = arr[1];
> 
> Why are you assigning uninitialized values to mShutdownRead/WriteFd?
I'm not.  They're set by pipe, and if pipe fails, i return a failure code.  They should never be uninitialized.

> 
> @@ +252,5 @@
> > +  return AddFileWatcherAlertable(aFile,aListener,false);
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsFileWatcherService::AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool alert)
> 
> I really don't think that we should be running this function (and all the
> statting/IO it will do) on the main thread.

That's true,  but that fix would probably delay the code for a while due to debugging and reapproval.  Would filing a follow-up be okay?

> 
> @@ +264,5 @@
> > +    nsresult rv = SpawnWorker();
> > +    NS_ENSURE_SUCCESS(rv,rv);
> > +  }
> > +
> > +  nsCString *path = new nsCString();
> 
> You don't need to allocate this on the heap.
It ends up getting stored in mWdLookup, so I think it does need to be allocated on the heap?  Unless it copies it when you insert it.

Comment 42

[Paul Dagnelie]

(In reply to Paul Dagnelie from comment #41)
> Created attachment 643200 [details] [diff] [review]
> Linux + core patch 4.0
> 
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #39)
> > Comment on attachment 642796 [details] [diff] [review]
> > Linux + core patch 3.0
> > 
> > Review of attachment 642796 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > ::: xpcom/io/Makefile.in
> > @@ +63,5 @@
> > >  endif # OS2
> > >  
> > > +ifeq ($(OS_ARCH),Linux)
> > > +CPPSRCS		+= nsFileWatcherServiceUnix.cpp
> > > +endif
> > 
> > Please reuse one of the existing ifdef blocks.
> None of the others actually only gets included on linux.  Since inotify only
> works on linux, I need to use a new one.
> 
What i should have said was "gets included *only* on linux".

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 643200 [details] [diff] [review]
Linux + core patch 4.0

Review of attachment 643200 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/Makefile.in
@@ +79,5 @@
>  		nsStorageStream.h \
>  		nsStringStream.h \
>  		nsStreamUtils.h \
>  		nsWildCard.h \
> +    nsFileWatcherService.h \

I still don't think that this needs to be exported.

::: xpcom/io/nsFileWatcherServiceUnix.cpp
@@ +44,5 @@
> +      mName.Assign("");
> +    }
> +  }
> +  ~FileChangeEvent()
> +    { }

Line the indenting here up with everything else too.

@@ +50,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +    nsresult rv = mFileWatcherService->OnFileChange(mWd, mName);
> +    return rv;

Just return mFileWatcherService->OnFileChange(mWd, mName);

@@ +110,5 @@
> +    unsigned int queue_len;
> +    ret = ioctl(self->mInotifyFd, FIONREAD, &queue_len);
> +    
> +    nsAutoPtr<char> events;
> +    events = new char[queue_len]; //for some reason, this has to be on a separate line

No.  This should be:

nsAutoArrayPtr<char> events(new char[queue_len]);

@@ +141,5 @@
> +static PLDHashOperator
> +TableDestroyEnumerator(const nsACString &aKey, nsAutoPtr<nsTArray<nsAutoPtr<nsFileWatcherService::FdListenerPair> > > &aArray, void *aUserArg) {
> +  for(int i=aArray->Length()-1; i >=0; i--) {
> +    aArray->RemoveElementAt(i);
> +  }

Just do aArray->Clear();

@@ +226,5 @@
> +
> +void nsFileWatcherService::DestroyWorker()
> +{
> +  if(!mRunning)
> +    return;

if (!mRunning) {
  return;
}

@@ +240,5 @@
> +
> +  mTable.Enumerate(TableDestroyEnumerator,nsnull);
> +  mWdLookup.Enumerate(LookupDestroyEnumerator,nsnull);
> +
> +

Spurious newline.

@@ +255,5 @@
> + */
> +NS_IMETHODIMP
> +nsFileWatcherService::AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> +{
> +  return AddFileWatcherAlertable(aFile,aListener,false);

Spaces between arguments please.

@@ +271,5 @@
> +    nsresult rv = SpawnWorker();
> +    NS_ENSURE_SUCCESS(rv,rv);
> +  }
> +
> +  nsCString *path = new nsCString();

This still should just be nsCString path;

@@ +296,5 @@
> +  // inotify only watches the given directory, not the
> +  // subdirectories, so we add those watches manually
> +
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

You need to check the retval here.

@@ +299,5 @@
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));
> +
> +  bool hasMore;
> +  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {

Because if it fails enumerator will be null.

@@ +302,5 @@
> +  bool hasMore;
> +  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {
> +
> +    nsCOMPtr<nsIFile> subfile;
> +    enumerator->GetNext(getter_AddRefs(subfile));

Same here.

@@ +306,5 @@
> +    enumerator->GetNext(getter_AddRefs(subfile));
> +
> +    bool issym;
> +    subfile->IsDirectory(&isdir);
> +    subfile->IsSymlink(&issym);

And here, since you haven't initialized issym or isdir.

@@ +340,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  int wd = -1;
> +  for (unsigned int i=0; i < arr->Length(); i++) {

PRUint32 i

@@ +362,5 @@
> +    return NS_OK;
> +  }
> +
> +  bool isdir;
> +  aFile->IsDirectory(&isdir);

You need to check the return value here.  This should be:

nsresult rv;
rv = aFile->IsDirectory(&isDir);
if (NS_FAILED(rv) || !isdir) {
  return NS_OK;
}

@@ +368,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));

You need to check the return value here.

@@ +371,5 @@
> +  nsCOMPtr<nsISimpleEnumerator> enumerator;
> +  aFile->GetDirectoryEntries(getter_AddRefs(enumerator));
> +  bool hasMore;
> +
> +  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {

Because if GetDirectoryEntries fails enumerator will be null here.

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also please get the followup filed for the main thread stats.

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 643200 [details] [diff] [review]
Linux + core patch 4.0

Karl, could you give the inotify and ioctl usage a once-over?

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 643200 [details] [diff] [review]
Linux + core patch 4.0

>+    else {
>+      mName.Assign("");
>+    }

mName should already be the empty string, so no need to assign("") 

>+    unsigned int queue_len;
>+    ret = ioctl(self->mInotifyFd, FIONREAD, &queue_len);

man ioctl_list says FIONREAD is (signed) int *.

It's a bit odd to record the return value, but not act on it.
I guess we can assume this won't fail and so there is no need to record the
return value.

>+      struct inotify_event *event = reinterpret_cast<struct inotify_event *>(events+i);

Use spaces for binary operators (events + i).

>+      if (event->mask & IN_Q_OVERFLOW) {
>+        NS_WARNING("Overflow of inotify event queue");
>+      }
>+      
>+      // if the watch has been removed, the data is no
>+      // longer in the mTables, so ignore
>+      if (event->mask & IN_IGNORED) {
>+        i += sizeof(struct inotify_event) + event->len;
>+        continue; 
>+      }
>+
>+      NS_DispatchToMainThread(new FileChangeEvent(self, event->wd, (event->len ? event->name : nsnull)));
>+      i += sizeof(struct inotify_event) + event->len;

continue on IN_Q_OVERFLOW as OnFileChange won't handle this.

Move the update of i to the top of the block so that only one line is
needed.

Isn't it possible that other bits (e.g. IN_DELETE) are set as well as IN_IGNORED, in which case we should notify observers?

>+      NS_DispatchToMainThread(new FileChangeEvent(self, event->wd, (event->len ? event->name : nsnull)));

Wrap to keep code within 80 columns.

>+  if (!storedName) { //The watch has been removed, but we haven't been told yet

I don't understand "but we haven't been told yet".
There is no action on IN_IGNORED events.

>+  for (unsigned int i=0; i < arr->Length(); i++) {
>+    arr2.AppendElement((*arr)[i]);
>+  }
>+  for (unsigned int i=0; i < arr2.Length(); i++) {

Add spaces around initializer '='.

>+    bool isdir;
>+    file->IsDirectory(&isdir);
>+    if (isdir) {
>+      AddFileWatcherAlertable(file,arr2[i]->mListener, true);
>+    }

Add a space in ",arr2".

I assume AddFileWatcherAlertable should be called only for create events.
I assume watchers should be removed for IN_MOVE_SELF events.

Watches are automatically removed on file deletion, so I hope the watch descriptor is not reused in the near future.  Seems we should remove immediately so we are less likely to remove a subsequent watch with the same descriptor.

Can the |aLert| parameter be renamed to aNotifyDescendants or
aUpdateDescendents?

>+  int wd = inotify_add_watch(mInotifyFd, path->get(), mask);

Check for success here.

Comment 47

[(dormant account)]

Comment on attachment 643200 [details] [diff] [review]
Linux + core patch 4.0

+    ssize_t len = read(self->mInotifyFd, events, queue_len);
+    if (!len) {
+      NS_WARNING("Read from inotify fd error!");
+    }
+

len = -1 is not caught here.

nit:
+      // if the watch has been removed, the data is no
+      // longer in the mTables, so ignore
+      if (event->mask & IN_IGNORED) {
+        i += sizeof(struct inotify_event) + event->len;
+        continue; 
+      }
+
+      NS_DispatchToMainThread(new FileChangeEvent(self, event->wd, (event->len ? event->name : nsnull)));
+      i += sizeof(struct inotify_event) + event->len;
+    }

duplicating pointer arith is messy.

use for(;i<len; i += sizeof(struct inotify_event) + event->len;)
instead of a while loop

Comment 48

[Paul Dagnelie]

Attachment: Linux + core patch 5.0

(In reply to Karl Tomlinson (:karlt) from comment #46)
> Comment on attachment 643200 [details] [diff] [review]
> Linux + core patch 4.0
> 
> >+    unsigned int queue_len;
> >+    ret = ioctl(self->mInotifyFd, FIONREAD, &queue_len);
> 
> man ioctl_list says FIONREAD is (signed) int *.
> 
> It's a bit odd to record the return value, but not act on it.
> I guess we can assume this won't fail and so there is no need to record the
> return value.
> 
Well, I don't know whether FIONREAD is one of the ioctl's that can fail.  Most can't.  I'll just remove the return check.

> I assume watchers should be removed for IN_MOVE_SELF events.
I'm tempted to leave this as an open question for two reasons: one, this needs to be done ASAP, and two, there are a lot of cases to think about.  If the file is a directory, and it's the top level being watched, and all that's happening is it is being renamed, should it stop being watched?  On the other hand, if ~/Music/iTunes is being moved to to /tmp/garbage, unwatching it is probable desirable.

Now, removing on IN_MOVE_FROM events makes sense, to me, at least.  And adding for IN_MOVE_TO.  However, if it's a file that gets moved out, it is hard to know whether that file is itself being watched, and whether it, therefore, needs to have its watch removed.  So, i'd support removing watches from directories that leave a watched directory, but other than that, I think other edge cases are going to require more time and thought.

Also, khuey: Turns out that if you don't export nsFileWatcherService.h in Makefile.in, the build fails on mac for some reason.  That may be because there's some other problem, but that's why it's still in there in this version.

Comment 49

[Paul Dagnelie]

Attachment: Linux + core patch 5.1

I didn't qref before uploading... fixed.

Comment 50

[Karl Tomlinson (:karlt)]

Comment on attachment 643647 [details] [diff] [review]
Linux + core patch 5.1

>+      // if the watch has been removed, the data is no
>+      // longer in the mTables, so ignore
>+      if (event->mask & IN_IGNORED && !(event->mask & (IN_UNMOUNT | IN_DELETE |
>+                                                       IN_DELETE_SELF)) ) {

if (event->mask & IN_IGNORED && !(event->mask & IN_ALL_EVENTS)) {

or

if (event->mask & IN_IGNORED && !(event->mask & (IN_UNMOUNT | IN_DELETE_SELF)) {

>+    bool isdir;
>+    file->IsDirectory(&isdir);
>+    if (isdir && aEvent & (IN_CREATE | IN_MOVED_TO)) {
>+      AddFileWatcherAlertable(file, arr2[i]->mListener, true);
>+    }
>+    if (isdir && aEvent & IN_MOVED_FROM) {
>+      RemoveFileWatcher(file, arr2[i]->mListener);
>+    }

For the Remove test, isdir is false where it matters because the dir no longer
exists.  Use aEvent & IN_ISDIR.
Do the same for the Add to save the IsDirectory stat.

The documentation implies that IN_MOVED_TO/FROM are only for files
(which would make this very difficult).

If you have tested and can verify that the documentation is misleading, the
TO/FROM sounds good.

If the documentation is correct, then remove IN_MOVED_TO/FROM and I want
something that won't leak watches.  Anything else can be sorted out in bug
775347.

  Replacing IN_MOVED_FROM with IN_MOVE_SELF is sufficient to remove the leak.
  It still has issues you refer to in comment 42, but they can be sorted out
  in the follow-up.

  Another option is to remove all subdir handling for now, and add that in bug
  775347, if necessary.

Marking r- because I want someone to verify that this issue is resolved, but
that can be someone else if they can get to it before I do.

Comment 51

[Karl Tomlinson (:karlt)]

The watches also need to be removed "depth first" to avoid leaking watches.
i.e. handle subdirs before removing the watch from their parent.

Comment 52

[Karl Tomlinson (:karlt)]

Bah, avoiding leaks with subdirs is hard.  If we get an event about a moved subdir (or even if an ancestor is moved), we can no longer read the dir at its old path, and so watches on subdirs of the subdir won't be removed.

I think subdir hierarchy handling needs to be removed and handled in the follow-up when there is a robust plan.

Comment 53

[Doug Turner (:dougt)]

what does 'subdir hierarchy handling needs' mean?

if you have a nsIFile pointing at /a/, will the listener see changes in /a/b/c ?

Comment 54

[Karl Tomlinson (:karlt)]

Without "subdir hierarchy handling", the listener will see changes to /a/file but not /a/b/c.

It looks unlikely that we'll get subdir hierarchy support that doesn't leak before tomorrow.

If this is no good without hierarchy support, then perhaps a |mChildren| array could be added to FdListenerPair to keep track of a tree of FdListenerPairs, but I haven't really thought that through.

Comment 55

[Doug Turner (:dougt)]

We need hierarchy support.

Comment 56

[Paul Dagnelie]

> The documentation implies that IN_MOVED_TO/FROM are only for files
> (which would make this very difficult).
> 
> If you have tested and can verify that the documentation is misleading, the
> TO/FROM sounds good.

I tested it on my machine, and it works on directories too.  Given that, what needs to be done to get this landed soon, if that is at all possible?

Comment 57

[Paul Dagnelie]

My idea is as follows:

class Directory {
  nsTArray<nsRefPtr<Directory> > mChildren;
  nsRefPtr<Directory> mParent;
  int mWd;
  bool mTopLevel;
  nsTArray<nsCOMPtr<nsIFileUpdateListener> > mListener;
};

nsClassHashtable<nsCStringHashKey, nsRefPtr<Directory> >mTable;
nsDataHashtable<nsUint64HashKey, nsCString>mWdLookup;

When adding a listener to a file, step 1 is look up the path in mTable.  If the target is found, the add the listener to the Directory's list, along with the list for each subdir, and mark mTopLevel to true.  This requires no file io.  If the target isn't found, then create the directory tree with this as the root, and recursively add listeners.  We could have it do lookups into mTable at each step to ensure that we don't duplicate trees.

When removing a listener, you have to remove the same file that you added the listener to.  Even if you know the file moved from, say, ~/Music/iTunes to ~/Music/iTunes_music, you have to use ~/Music/iTunes, because there's no way for us to know that that's the new name.  Alternatively, we could simply automatically remove those watches that involve moving the top level of a watch heirarchy.  Either way works.  In the end, you recurse through, using removing from the list of listeners, and, if it's empty, deleting the directory entry and calling inotify_rm_watch.  This should also require no file io.

When an update comes in, there are a few cases:
* There's the easy case, where new files are created, or deleted, or modified, etc.
* There's the case where new folders are created, possibly with contents.  We add those to the directory tree under the watch archive, and add every listener that is listening to the new parent to every subdirectory, and then update all of those listeners with each new directory and file.
* There's the cases where subdirectories are moved: If a directory gets a IN_MOVED_FROM event, delete it and the tree below it (also do this if a subdirectory is just deleted).  If it gets an IN_MOVED_TO event, add it and the tree below it.  This does mean doing wasteful work if a subdirectory is renamed, but I don't know how to solve it.  You don't know if you're going to get an IN_MOVED_TO event immediately, since a cookie is always generated on either of these events.
* Finally, there's the case where the top level of a heirarchy is moved.  I'm not sure what to do in this case.  The options are "leave it alone, and make them remove it with the old name", or "delete it and make them re-add it"

Are there cases is missed?  What do people think?

Comment 58

[Karl Tomlinson (:karlt)]

Thanks.
There are still some tricky issues but this may work out.

Bear in mind that there may be a FileWatcher on a directory with an ancestor
directory also having a FileWatcher.  That means mTopLevel can't be on the
shared Directory node.

The FileWatcher listeners don't know that the event indicates a file removal, so I think we have to let the client call RemoveFileWatcher even after the
file has been removed.  But I assume we still want to remove the inotify watch descriptors.

I'm not yet clear we have a solution to these issues.

More minor things to think about:

I expect Directory doesn't need the strong references to mChildren and
mParent as there will be a reference in mTable.  Perhaps call mTable
mDirectoryTable.

You may not need to add to the mListener array for the whole subtree (when
adding subtrees), as OnFileChange may be able to walk up the tree to build an
observer list for the event.  With this approach, on RemoveFileWatcher,
subtrees would be removed only when they have no mParent and the only
mListener matches aListener.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

Looks like I shouldn't start my review of the Mac patch yet -- not until the "Linux + Core" patch is finished, and the Mac patch has been revised accordingly.

I'll be out next week.  So I won't be able to start on this until the week after next.

Also, since I'm not familiar with Apple's File System Events API (https://developer.apple.com/library/mac/#documentation/Darwin/Conceptual/FSEvents_ProgGuide/Introduction/Introduction.html), I'll need to build this patch and test it.  For that it'd be nice to have some kind of testcase for manual testing.

Comment 60

[Doug Turner (:dougt)]

Steven, the cocoa patch is very different and doesn't have these same recursive issues.  There are already tests included in the patch.  Thanks for looking at this.

Comment 61

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #60)

Thanks, Doug.  Then I'll start my review as soon as I get back.

I saw the automated tests.  But I'd like some kind of manual testcase to play with -- just a web page that exercises the new API.

Comment 62

[Paul Dagnelie]

> Bear in mind that there may be a FileWatcher on a directory with an ancestor
> directory also having a FileWatcher.  That means mTopLevel can't be on the
> shared Directory node.
I thought about that.  I don't see why not.  it's still the top level of a heirarchy.

> The FileWatcher listeners don't know that the event indicates a file
> removal, so I think we have to let the client call RemoveFileWatcher even
> after the
> file has been removed.  But I assume we still want to remove the inotify
> watch descriptors.
This is true.  I'm also not sure what to do here.  maybe keep a record of everything we automatically removed and then just quickly return when the user manually removes it?

> More minor things to think about:
> 
> I expect Directory doesn't need the strong references to mChildren and
> mParent as there will be a reference in mTable.  Perhaps call mTable
> mDirectoryTable.
I don't really understand the first part of that.  Yes, the directory entries will be stored in the table, but they also need to be in the tree structure.  The name change is fine.

> 
> You may not need to add to the mListener array for the whole subtree (when
> adding subtrees), as OnFileChange may be able to walk up the tree to build an
> observer list for the event.  With this approach, on RemoveFileWatcher,
> subtrees would be removed only when they have no mParent and the only
> mListener matches aListener.

That was another thing i considered.  I wasn't sure whether we wanted to take the time to walk the tree.  That could be timeconsuming in a large directory structure.  But i guess it does save a lot of memory.

Comment 63

[Karl Tomlinson (:karlt)]

Consider the following situations that might happen if an editor changes a file..

1. There is a watch in file "a",
then either
A. 2. "a" is renamed to "a.bak", then
   3. A new file "a" is created,
or
B.
   2. "a.tmp" is moved to "a".

In each situation the nsIFileUpdateListener will receive notification of the
update to "a".  The listener may then look at "a" again.  It may notice the
file has changed but it won't know that the listener is not watching the new
"a".  It is either A watching "a.bak" or B not watching anything.

I think we need to treat the FileWatchers as watches on paths.
The watch is performed on an nsIFile and the update method provides an
nsIFile.  nsIFile doesn't represent a file but a path.

Treating FileWatchers as watches on paths requires watching each ancestor
directory.  That's a bit of a pain but not as much as already suffered
by watching a complete subtree.  It may simplify some things.

BTW, should nsIFileUpdateListener::update() be passed the same nsIFile object
as was passed to AddFileWatcher?
And should the nsIFile automatically remove its watches when the nsIFile goes
out of scope?

(In reply to Paul Dagnelie from comment #62)
> > Bear in mind that there may be a FileWatcher on a directory with an ancestor
> > directory also having a FileWatcher.  That means mTopLevel can't be on the
> > shared Directory node.

> I thought about that.  I don't see why not.  it's still the top level of a
> heirarchy.

If it just means "the top level of *a* hierarchy" then that is fine.  I don't
know what the intended purpose is.  Perhaps it won't be needed with the path
based approach.  Perhaps this information is also available through
mListeners.  I guess mListeners would at least need to be checked to determine
when mIsATopLevel could be set to false.

> > The FileWatcher listeners don't know that the event indicates a file
> > removal, so I think we have to let the client call RemoveFileWatcher even
> > after the
> > file has been removed.  But I assume we still want to remove the inotify
> > watch descriptors.

> This is true.  I'm also not sure what to do here.  maybe keep a record of
> everything we automatically removed and then just quickly return when the
> user manually removes it?

I'm guessing path-based FileWatchers will make this easier as mListeners
doesn't change.  The inotify watch descriptors can be set to -1 to indicate
that there is currently no watch on the path.

(Without path based watches, it would get complicated when a file being
watched is deleted, and then another watch is added with the same path.  When
RemoveFileWatcher is called there could be two matches for the same path and
observer, but we wouldn't know whether to remove from the list of already
automatically removed watches or from the active watch.)

> > I expect Directory doesn't need the strong references to mChildren and
> > mParent as there will be a reference in mTable.  [...]

> I don't really understand the first part of that.  Yes, the directory
> entries will be stored in the table, but they also need to be in the tree
> structure.

When separate objects each add a reference to the other, there is a risk of
each keeping the other alive, unless the references are carefully removed.

Yes there need to be pointers in the tree, but it may be sufficient to have

  nsTArray<Directory*> mChildren;
  Directory* mParent;

Comment 64

[Paul Dagnelie]

(In reply to Karl Tomlinson (:karlt) from comment #63)
> Consider the following situations that might happen if an editor changes a
> file..
> 
> 1. There is a watch in file "a",
> then either
> A. 2. "a" is renamed to "a.bak", then
>    3. A new file "a" is created,
> or
> B.
>    2. "a.tmp" is moved to "a".
> 
> In each situation the nsIFileUpdateListener will receive notification of the
> update to "a".  The listener may then look at "a" again.  It may notice the
> file has changed but it won't know that the listener is not watching the new
> "a".  It is either A watching "a.bak" or B not watching anything.
I think this is a very good point.
 
> I think we need to treat the FileWatchers as watches on paths.
> The watch is performed on an nsIFile and the update method provides an
> nsIFile.  nsIFile doesn't represent a file but a path.
> 
> Treating FileWatchers as watches on paths requires watching each ancestor
> directory.  That's a bit of a pain but not as much as already suffered
> by watching a complete subtree.  It may simplify some things.
This sounds like we need to set up a pattern where there can be a formal inotify watch on a directory, but we only care about updates to some of its elements.  This also gives us a decent way to do watches on a particular file instead of on a directory: we watch the directory and only call a particular listener when a particular file changes.  I guess this means we should change "Directory" as follows:

class Directory {
  nsCString name; // the name for this directory so we can reconstruct paths as we travel up and down the tree
  nsTArray<Directory* > mChildren;
  Directory * mParent;
  int mWd;
  bool mTopLevel;
  nsTArray<PathListenerPair> mListeners; //this has any listeners introduced for this directory, and for files within it.
}

The idea behind the "path listener pair" is that we would iterate through it any time a file in that directory gets updated, and if it's in the list of paths, we update, otherwise, we don't.  We could have "" or something represent "listeners for the whole directory and its subchildren", and they could always be first to make thing easier.  Then we iterate upwards through the directory listing looking for more listeners.  I guess my two questions are "how far upwards should the tree extend" and "is there a good way to short circuit iterating backwards through the tree if there are no more listeners to get".  that last one could just be a flag, I guess.
 
> BTW, should nsIFileUpdateListener::update() be passed the same nsIFile object
> as was passed to AddFileWatcher?
> And should the nsIFile automatically remove its watches when the nsIFile goes
> out of scope?
We could do that... do we want to force them to keep their nsIFile around?  And what is gained by this?

> 
> (In reply to Paul Dagnelie from comment #62)
> > > Bear in mind that there may be a FileWatcher on a directory with an ancestor
> > > directory also having a FileWatcher.  That means mTopLevel can't be on the
> > > shared Directory node.
> 
> > I thought about that.  I don't see why not.  it's still the top level of a
> > heirarchy.
> 
> If it just means "the top level of *a* hierarchy" then that is fine.  I don't
> know what the intended purpose is.  Perhaps it won't be needed with the path
> based approach.  Perhaps this information is also available through
> mListeners.  I guess mListeners would at least need to be checked to
> determine
> when mIsATopLevel could be set to false.
The idea was that if we got a notice of "IN_MOVE_SELF" and it was a top level directory of a heirarchy, we could decide to do something special, whereas if it was a lower level, we'd just let our parents take care of it.  It's less important if we use the idea of the upwards recursion.  Also, how bad is the watching of parents going to be for performance?  I feel like it could be pretty bad.  If we're not actually putting inotify_watch'es on them, what would the point be?

> 
> > > The FileWatcher listeners don't know that the event indicates a file
> > > removal, so I think we have to let the client call RemoveFileWatcher even
> > > after the
> > > file has been removed.  But I assume we still want to remove the inotify
> > > watch descriptors.
> 
> > This is true.  I'm also not sure what to do here.  maybe keep a record of
> > everything we automatically removed and then just quickly return when the
> > user manually removes it?
> 
> I'm guessing path-based FileWatchers will make this easier as mListeners
> doesn't change.  The inotify watch descriptors can be set to -1 to indicate
> that there is currently no watch on the path.
> 
> (Without path based watches, it would get complicated when a file being
> watched is deleted, and then another watch is added with the same path.  When
> RemoveFileWatcher is called there could be two matches for the same path and
> observer, but we wouldn't know whether to remove from the list of already
> automatically removed watches or from the active watch.)
Well, they'd probably have different listeners.  If they didn't, we could just discard the old one when they add a new one.

Comment 65

[Karl Tomlinson (:karlt)]

> This sounds like we need to set up a pattern where there can be a formal
> inotify watch on a directory, but we only care about updates to some of its
> elements.  This also gives us a decent way to do watches on a particular
> file instead of on a directory: we watch the directory and only call a
> particular listener when a particular file changes.

I didn't notice any problem with the previous way of watching a particular
file.  Watching the parent instead of the file might help us reduce the number
of watches we need in some situations, but I doubt these are situations we need
to optimize.  In some situations (e.g. /tmp/), watches on particular files are
going to be much more efficient than a watch on all files under the parent
directory.

> I guess my two questions are "how far upwards should the tree extend"

Ideally to one level below /.

But I don't really mind if an initial implementation doesn't behave as
expected with parent path changes, so long as there are no leaks and changes
in the last link of the Watcher path are handled as expected (to deal with
editor changes).

> and "is there a good way to
> short circuit iterating backwards through the tree if there are no more
> listeners to get".  that last one could just be a flag, I guess.

Iterating up is going to be much less time consuming than iterating down, so
aim for minimal/simple code rather than speed.

> > BTW, should nsIFileUpdateListener::update() be passed the same nsIFile object
> > as was passed to AddFileWatcher?
> > And should the nsIFile automatically remove its watches when the nsIFile goes
> > out of scope?
> We could do that... do we want to force them to keep their nsIFile around? 
> And what is gained by this?

The gain I'm looking for is a way to make it easier for clients to avoid
leaking (hard for clients to leak). 

To avoid leaking, the client needs to keep a record of Watchers it has added.
Keeping the nsIFile is one way to do this.  The watch() method is called on
the nsIFile, so calling unwatch() on the same nsIFile feels appropriate.

> Also, how bad is the watching of parents going to be for performance?  I
> feel like it could be pretty bad.  If we're not actually putting
> inotify_watch'es on them, what would the point be?

They'd need inotify watches to be useful.
Only IN_MOVE_SELF/IN_DELETE_SELF are needed on these directories, so it
shouldn't be a performance issue.

Comment 66

[Paul Dagnelie]

Attachment: Just the interface changes and a stub impl

This just changes the nsIFile idl and adds a stub implementation to Common so we can get this out the door.  Each platform's impementation will be added later.

Comment 67

[Karl Tomlinson (:karlt)]

(In reply to Paul Dagnelie from comment #64)
> This sounds like we need to set up a pattern where there can be a formal
> inotify watch on a directory, but we only care about updates to some of its
> elements.  This also gives us a decent way to do watches on a particular
> file instead of on a directory: we watch the directory and only call a
> particular listener when a particular file changes.

Oh, I think see your point now.

(In reply to Karl Tomlinson (:karlt) from comment #65)
> I didn't notice any problem with the previous way of watching a particular
> file.  Watching the parent instead of the file might help us reduce the number
> of watches we need in some situations, but I doubt these are situations we
> need to optimize.  In some situations (e.g. /tmp/), watches on particular files
> are going to be much more efficient than a watch on all files under the parent
> directory.

> Only IN_MOVE_SELF/IN_DELETE_SELF are needed on these directories, so it
> shouldn't be a performance issue.

I guess we are also going to need to watch for changes on the parent directory to detect when the file with Watcher exists again (situation A in comment 63).
And I guess its not worth managing changing the events watched on a directory based on whether a sublink exists or not.

Watching IN_MODIFY only on particular files would still help make this more efficient.

(In reply to Paul Dagnelie from comment #64)
> The idea behind the "path listener pair" is that we would iterate through it
> any time a file in that directory gets updated, and if it's in the list of
> paths, we update, otherwise, we don't.

Consider as an alternative keeping the Directory entry when there is an observer on a descendant, but the link doesn't exist, and marking the Directory as not existing / not having an active inotify watch.

Comment 68

[Paul Dagnelie]

(In reply to Karl Tomlinson (:karlt) from comment #67)
> (In reply to Karl Tomlinson (:karlt) from comment #65)
> > I didn't notice any problem with the previous way of watching a particular
> > file.  Watching the parent instead of the file might help us reduce the number
> > of watches we need in some situations, but I doubt these are situations we
> > need to optimize.  In some situations (e.g. /tmp/), watches on particular files
> > are going to be much more efficient than a watch on all files under the parent
> > directory.
> 
> > Only IN_MOVE_SELF/IN_DELETE_SELF are needed on these directories, so it
> > shouldn't be a performance issue.
> 
> I guess we are also going to need to watch for changes on the parent
> directory to detect when the file with Watcher exists again (situation A in
> comment 63).
> And I guess its not worth managing changing the events watched on a
> directory based on whether a sublink exists or not.
> 
> Watching IN_MODIFY only on particular files would still help make this more
> efficient.
For the parent directories, where we are only seeing "does our child directory move or get deleted", we could do IN_MOVE | IN_DELETE | IN_CREATE.  We could also IN_EXCL_UNLINK, which would help, especially for things like /tmp.
 
> (In reply to Paul Dagnelie from comment #64)
> > The idea behind the "path listener pair" is that we would iterate through it
> > any time a file in that directory gets updated, and if it's in the list of
> > paths, we update, otherwise, we don't.
> 
> Consider as an alternative keeping the Directory entry when there is an
> observer on a descendant, but the link doesn't exist, and marking the
> Directory as not existing / not having an active inotify watch.
as an alternative to what?  the idea at present is basically a B-tree where when a watch fires on a directory, we iterate through the list of listeners in that directory and call the relevant ones, and then iterate upwards and call any relevant listeners in the parents.  When you call AddFileListener, you add a listener to the directory you passed in, and it is only stored there.

> > > BTW, should nsIFileUpdateListener::update() be passed the same nsIFile object
> > > as was passed to AddFileWatcher?
> > > And should the nsIFile automatically remove its watches when the nsIFile goes
> > > out of scope?
> > We could do that... do we want to force them to keep their nsIFile around? 
> > And what is gained by this?
> 
> The gain I'm looking for is a way to make it easier for clients to avoid
> leaking (hard for clients to leak). 
> 
> To avoid leaking, the client needs to keep a record of Watchers it has added.
> Keeping the nsIFile is one way to do this.  The watch() method is called on
> the nsIFile, so calling unwatch() on the same nsIFile feels appropriate.

this would let us use pointers instead of strings for some things, which would be cleaner.

Comment 69

[Karl Tomlinson (:karlt)]

(In reply to Paul Dagnelie from comment #68)
> (In reply to Karl Tomlinson (:karlt) from comment #67)
> > (In reply to Paul Dagnelie from comment #64)
> > > The idea behind the "path listener pair" is that we would iterate through it
> > > any time a file in that directory gets updated, and if it's in the list of
> > > paths, we update, otherwise, we don't.
> > 
> > Consider as an alternative keeping the Directory entry when there is an
> > observer on a descendant, but the link doesn't exist, and marking the
> > Directory as not existing / not having an active inotify watch.

> as an alternative to what?  the idea at present is basically a B-tree where
> when a watch fires on a directory, we iterate through the list of listeners
> in that directory and call the relevant ones, and then iterate upwards and
> call any relevant listeners in the parents.  When you call AddFileListener,
> you add a listener to the directory you passed in, and it is only stored
> there.

Sounds good.  I'm questioning whether PathListenerPair is necessary, or nsIFileUpdateListener is sufficient because the path information is already in the Directory tree.

Comment 70

[Paul Dagnelie]

The idea behind including the path was that you could use this to watch individual files more easily.  For example, if someone wanted to watch /home/person/.program/config, inotify_add_watch would be called on /home/person/.program and then if an event occured and the event's name field was equal to 'config', the event would trigger.  This lets you handle things like mv config config.bak; mv newconf config; and it'll watch the new config file.  That path information isn't stored in the tree as yet, and since each file being watched could be paired with a different listener, that connection has to be reflected somewhere.

Comment 71

[Paul Dagnelie]

Attachment: Linux patch 6.0

This version incorporates the preceeding comments (there's still file I/O on the main thread, but less), and shouldn't leak watches.  Also, users are no longer allowed to remove a listener from the Update function.

Comment 72

[Paul Dagnelie]

Attachment: Linux patch 6.1

This patch adds a change type back into the system, so people being updated know what kind of change occured.  It also adds some documentation and removes spurious debug printfs.

Comment 73

[Paul Dagnelie]

Attachment: interface changes 2

This is the change that actually changes the interface and adds the type parameter to Update

Comment 74

[Paul Dagnelie]

Attachment: Linux patch 6.2

fixing bugs and adding tests

Comment 75

[Doug Turner (:dougt)]

Interface only change landed on m-i:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/482dd6331e42


Please leave bug open for linux implementation which should land shortly.

Paul, please create separate/followups for mac and windows.

Comment 76

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/482dd6331e42

Comment 77

[Doug Turner (:dougt)]

per comment 74

Comment 78

[Paul Dagnelie]

Attachment: Linux patch 6.3

This version of the linux patch adds some documentation and a couple bug fixes pointed out by dougt.

Comment 79

[Paul Dagnelie]

Attachment: Mac patch 1.1

Fixes the mac patch to pass in a change type to Update.

Comment 80

[Paul Dagnelie]

Attachment: Windows patch 1.1

Modifies windows to pass in a change type to Update.

Comment 81

[Paul Dagnelie]

Attachment: Linux patch 6.4

forgot to hg add a file

Comment 82

[Karl Tomlinson (:karlt)]

Comment on attachment 647801 [details] [diff] [review]
interface changes 2

The interface change to nsIFile needs superreview.
http://www.mozilla.org/hacking/reviewers.html

Some things to consider:

Something resembling an enum would be more pleasant in C++ than strings
"created" etc.

Is it reasonable to rely on javascript to explicitly unwatch the paths that it
watches?

Is it clear to the client that the listener won't be destroyed until it breaks
the watch, even when there are no external references?

Similarly, is it clear that the nsIFile won't be destroyed until something
explicitly breaks the watch, even when there are no external references?

Comment 83

[Karl Tomlinson (:karlt)]

Comment on attachment 648549 [details] [diff] [review]
Linux patch 6.4

In trying to provide feedback as soon as I can, here are some review comments,
even though I haven't completed review.

What is being implemented is complex, and more helpful comments would help
make it easier to follow.  We don't need comments on individual operations,
unless there is a particular reason for something being done in a particular
way.  However, overview comments, describing the structures in detail and
describing what each state means, are helpful.

If watches can't be removed during update, then that needs to be documented in
the API rather than failing when the remove method is called.  That failure
could happen in a nested event loop and may not be detected by the client.
The previous version of this patch built a list of updates to issue before
dispatch, so there were no problems with removal during update.  That may also
be necessary for handling inotify events during nested event loops.

Perhaps the API can specify that nested event loops are not permitted, but I
don't know whether we could allow script to run in that case.  Being able to
remove a watch during update sounds like a useful feature.

>* * *
>Adding new tests for onchange notifications in devicestorage
>* * *
>B# HG changeset patch
># Parent 3985d9f5e5102af9428ea302a063e7dbc2113fee
>* * *
>try: -b do -p all -u all -t all
>* * *
>try: -b do -p macosx64,macosx -u all -t all

I'm not familiar with "* * *", but looks like this is accumulating remants of something else in the comment.

> ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>-CMMSRCS		+= \
>-		CocoaFileUtils.mm \
>-		$(NULL)
>+CMMSRCS		+= CocoaFileUtils.mm
> endif

Normally we don't make whitespace changes to code that is not being modified,
so leave this change out of this patch.

> EXPORTS		= \
> 		nsAppDirectoryServiceDefs.h \
> 		nsDirectoryService.h \

> 		nsWildCard.h \
>+		nsFileWatcherService.h \

nsFileWatcherService.h shouldn't be exported because it contains APIs not
intended to be used elsewhere.

I can't think how exporting the file helps the build as the file includes
other files that are not exported.

> nsLocalFile::Watch(nsIFileUpdateListener *listener)
> {
>     NS_ASSERTION(NS_IsMainThread(), "Watch must be called from main thread!");
>-    return NS_ERROR_NOT_IMPLEMENTED;
>+
>+    nsFileWatcherService* fws = nsFileWatcherService::GetService();
>+    if (!fws) {
>+      return NS_ERROR_NOT_AVAILABLE;
>+    }

Looks like these should still return NS_ERROR_NOT_IMPLEMENTED.

>+++ b/xpcom/io/nsFileWatcherServiceDefault.h

>+  NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
>+  {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }
>+
>+  NS_IMETHOD RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
>+  {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }

Can these methods be removed, given there is no concrete nsFileWatcherService
ever instantiated in this build?

>+nsFileWatcherService *gFileWatcher = nsnull;
>+bool gShutdown = false;

These should have internal linkage.

>+  nsCString mName; //name of the file being watched; only set if we're watching a specific file isntead of a directory
>+  nsCOMPtr<nsIFileUpdateListener> mListener; //the listener to update when something chases; always set
>+  nsCOMPtr<nsIFile> mFile; //the file corresponding to the thing that is ultimately being watched down the parent chain; only set if this listener represents a parent watching for a descendent
>+
>+  PathListenerPair()
>+  {
>+    mListener = nsnull;
>+    mFile = nsnull;
>+    mName.Assign("",0);

Stay within 80 columns.

isntead
chases -> changes.

mListener is "always set" but not in this constructor.

No need to reassign initial values to member variables with default
constructors as these are already set.  Similarly in Directory.

It seems that a PathListenerPair is used to represent two quite different
things: 1, a file explicitly watched for any changes; and 2, descendant
listeners that want to know when this directory is moved.

An overview comment for the PathListenerPair distinguishing the two cases
would be helpful.

mName is getting set even for parent directories via the 3 arg constructor
from AddFileWatcherAlertable (contrary to the comment above).

I wonder why the nsIFileUpdateListener is recorded for purpose 2, given that
the tree needs to be descended anyway to remove watches.  (Maybe this will
become clearer as I look further.)

>+  PathListenerPair(PathListenerPair *aPair)
>+  {
>+    mName = aPair->mName;
>+    mListener = aPair->mListener;
>+    mFile = aPair->mFile;
>+    MOZ_COUNT_CTOR(PathListenerPair);

Use constructors rather than assignment for initializing member variables.
Similarly in Directory.

>+  bool mTopLevel; //set to true if this directory has any listeners introduced for it or its files
>+  nsTArray<PathListenerPair> mListeners; //this has any listeners introduced for this directory, and for files within it, or for any of its descendents 

"descendants"

Can you expand the comment to clarify the distinction between "and" and "or",
please?

>+ *  Note: this code makes many linux-specific assumptions.

The code uses inotify, which is linux-specific.  That would suggest the file
should be called nsFileWatcherServiceLinux or nsFileWatcherServiceInotify, and
there's no need to include this comment, unless there are specific issues you
want to alert people to.

>+ *  it.  We also need to ensure the directory's tree is in the table,
>+ *  and that all of its subdirectories and parents have watches.  Much

The text here makes a distinction here between trees in the table and watches
for links in the tree.  Each link is in the table, right?
Please adjust the comment to clarify.

>+  aFile->IsDirectory(&isdir);
>+
>+  if (isdir) {
>+    aFile->GetNativePath(dirPath);
>+    aFile->GetNativeLeafName(dirName);
>+  } else {
>+    parent->GetNativePath(dirPath);
>+    parent->GetNativeLeafName(dirName);
>+    nsCOMPtr<nsIFile> partemp;
>+    parent->GetParent(getter_AddRefs(partemp));
>+    parent = partemp;
>+  }
>+
>+  Directory *dir = mDirectoryTable.Get(dirPath);

A single path may point a directory and a file at different times.  To catch
these changes, an inotify watch needs to be on the parent directory before the
IsDirectory() test.  By induction this means starting with the root directory
(which won't be a file and won't get renamed).

>+    if (!aTop)
>+      return NS_OK;
>+    if (!isdir) {

Usually leave a blank line after jump statements in single-line blocks.

>+      PathListenerPair pair (leafname, aListener);
>+      dir->mListeners.AppendElement(&pair);
>+    } else {
>+      PathListenerPair pair (aListener);
>+      dir->mListeners.InsertElementAt(0, &pair);

Mozilla style has no space before the '(' for the constructor.

Why the distinction between prepending and appending pairs?
This needs to be documented.

>+  uint32_t mask = IN_CREATE | IN_DELETE | IN_MODIFY | IN_MOVE | (isdir ? 0 : IN_DELETE_SELF | IN_MOVE_SELF);
>+  int wd = inotify_add_watch(mInotifyFd, dirPath.get(), mask);

Are IN_DELETE_SELF | IN_MOVE_SELF ever needed?
I wonder why there is a distinction in the kind of watch on a directory based
on whether the directory or one of its files is being watched.

Watches in different parts of the tree, ancestors and descendants, have
different flags, but I don't see any watches being changed when new listeners
are added.  At least add comments to explain the differences and how they are managed.

>+  aDir->mWd = inotify_add_watch(mInotifyFd, path.get(), IN_CREATE | IN_DELETE | IN_MODIFY | IN_MOVE | IN_DELETE_SELF | IN_MOVE_SELF); //this should probably change...

I guess so?

>+public:

>+  nsFileWatcherService();
>+  nsresult Init();
>+  void Shutdown();
>+  
>+  class FdListenerPair;
>+  class PathListenerPair;
>+  class Directory;

How many of these need to be public?

>+private:
>+  ~nsFileWatcherService();
>+  nsresult AddFileWatcherAlertable(nsIFile *aFile, nsIFileUpdateListener *aListener, bool aUpdateDescendents, bool aTop);
>+  nsresult SpawnWorker();
>+  void DestroyWorker();
>+  void RecursivelyRemove(nsFileWatcherService::Directory *aDir, nsCOMPtr<nsIFile> *aFile);
>+  void RecursivelyRemoveWatches(nsFileWatcherService::Directory *aDir);
>+  void RecursivelyAddWatches(nsFileWatcherService::Directory *aDir, nsCOMPtr<nsIFile> *aFile);
>+protected:
>+  static void EventLoop(void *ignored);

I assume there is no distinction intended between private/public?
Can "protected" be removed?

>+  int mLevels;

Document this.
Can there be an update within an update?
Nested event loops, I guess.

>+  // This table goes from a path to a list of Observer/inotify watch pairs
>+  nsClassHashtable<nsCStringHashKey,Directory>mDirectoryTable;
>+  // This table goes from an inotify watch to a file name
>+  nsDataHashtable<nsUint64HashKey, nsCString>mWdLookup;

... "inotify watch to a path name"

Have a space between the type and the variable name.

Comment 84

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 648537 [details] [diff] [review]
Mac patch 1.1

This patch won't apply to current trunk.

Are there other patches which haven't landed yet that it depends on?

Comment 85

[Paul Dagnelie]

Attachment: Linux patch 7.0

(In reply to Karl Tomlinson (:karlt) from comment #82)
> Comment on attachment 647801 [details] [diff] [review]
> interface changes 2
> 
> The interface change to nsIFile needs superreview.
> http://www.mozilla.org/hacking/reviewers.html
The interface changes have already landed.

> Something resembling an enum would be more pleasant in C++ than strings
> "created" etc.
I think that strings were preferred for javascript's sake?

> 
> Is it reasonable to rely on javascript to explicitly unwatch the paths that
> it
> watches?
What other option is there?  If they don't unwatch them, the watches will be released on shutdown.  Other than that, there isn't much we can do.

> Is it clear to the client that the listener won't be destroyed until it
> breaks
> the watch, even when there are no external references?
> 
> Similarly, is it clear that the nsIFile won't be destroyed until something
> explicitly breaks the watch, even when there are no external references?
Does it especially matter?  Will javascript users care when exactly the nsIFile goes away, or when the listener is gc'd?

> If watches can't be removed during update, then that needs to be documented
> in
> the API rather than failing when the remove method is called.  That failure
> could happen in a nested event loop and may not be detected by the client.
> The previous version of this patch built a list of updates to issue before
> dispatch, so there were no problems with removal during update.  That may
> also
> be necessary for handling inotify events during nested event loops.
> 
> Perhaps the API can specify that nested event loops are not permitted, but I
> don't know whether we could allow script to run in that case.  Being able to
> remove a watch during update sounds like a useful feature.
Unfortunately, doing it involves some ugly hacks, like manually checking for gc canary values.  In the end, it's not that hard to just call unwatch later.


> 
> > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> >-CMMSRCS		+= \
> >-		CocoaFileUtils.mm \
> >-		$(NULL)
> >+CMMSRCS		+= CocoaFileUtils.mm
> > endif
> 
> Normally we don't make whitespace changes to code that is not being modified,
> so leave this change out of this patch.


> 
> > EXPORTS		= \
> > 		nsAppDirectoryServiceDefs.h \
> > 		nsDirectoryService.h \
> 
> > 		nsWildCard.h \
> >+		nsFileWatcherService.h \
> 
> nsFileWatcherService.h shouldn't be exported because it contains APIs not
> intended to be used elsewhere.
> 
> I can't think how exporting the file helps the build as the file includes
> other files that are not exported.
> 
> > nsLocalFile::Watch(nsIFileUpdateListener *listener)
> > {
> >     NS_ASSERTION(NS_IsMainThread(), "Watch must be called from main thread!");
> >-    return NS_ERROR_NOT_IMPLEMENTED;
> >+
> >+    nsFileWatcherService* fws = nsFileWatcherService::GetService();
> >+    if (!fws) {
> >+      return NS_ERROR_NOT_AVAILABLE;
> >+    }
> 
> Looks like these should still return NS_ERROR_NOT_IMPLEMENTED.
They should return NS_ERROR_NOT_AVAILABLE if it's on a platform that's been implemented.  This should maybe changed so that GetService returns an error code and has an outparam.

> 
> >+++ b/xpcom/io/nsFileWatcherServiceDefault.h
> 
> >+  NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> >+  {
> >+    return NS_ERROR_NOT_IMPLEMENTED;
> >+  }
> >+
> >+  NS_IMETHOD RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> >+  {
> >+    return NS_ERROR_NOT_IMPLEMENTED;
> >+  }
> 
> Can these methods be removed, given there is no concrete nsFileWatcherService
> ever instantiated in this build?
To remove them, we'd have to remove them from the abstract interface that defines this class, and I think it's useful to keep these defined so that the set of functions associated with this class is clear.  Also, in case someone does something silly like directly calling the constructor and creating one directly.

> I wonder why the nsIFileUpdateListener is recorded for purpose 2, given that
> the tree needs to be descended anyway to remove watches.  (Maybe this will
> become clearer as I look further.)
That could and probably should be changed.

> >+  aFile->IsDirectory(&isdir);
> >+
> >+  if (isdir) {
> >+    aFile->GetNativePath(dirPath);
> >+    aFile->GetNativeLeafName(dirName);
> >+  } else {
> >+    parent->GetNativePath(dirPath);
> >+    parent->GetNativeLeafName(dirName);
> >+    nsCOMPtr<nsIFile> partemp;
> >+    parent->GetParent(getter_AddRefs(partemp));
> >+    parent = partemp;
> >+  }
> >+
> >+  Directory *dir = mDirectoryTable.Get(dirPath);
> 
> A single path may point a directory and a file at different times.  To catch
> these changes, an inotify watch needs to be on the parent directory before
> the
> IsDirectory() test.  By induction this means starting with the root directory
> (which won't be a file and won't get renamed).
I don't think that this will help... having watch on the parent directories won't interrupt this operation, and won't enable us to change the process.  As soon as it starts, whenever it starts, the path could change from a directory to a file or vice versa.  There's no way to fix this short of having a lock on the file system somehow.

> Watches in different parts of the tree, ancestors and descendants, have
> different flags, but I don't see any watches being changed when new listeners
> are added.  At least add comments to explain the differences and how they
> are managed.
I'm not sure what you mean here.  Why would watches on existing things be changed when a new listener is added?

> >+public:
> 
> >+  nsFileWatcherService();
> >+  nsresult Init();
> >+  void Shutdown();
> >+  
> >+  class FdListenerPair;
> >+  class PathListenerPair;
> >+  class Directory;
> 
> How many of these need to be public?
Directory does, since it is used in non-class functions (the table destructors).  The others have moved.

There isn't much more new documentation in this version, but that's ongoing, and the next version should have better high-level documentation explaining the system as a whole.

Comment 86

[Paul Dagnelie]

Attachment: Mac patch 1.2

Fixed the patch queue so it pushes.

Comment 87

[Paul Dagnelie]

Attachment: Windows patch 1.2

Same as for mac.

Comment 88

[Karl Tomlinson (:karlt)]

(In reply to Paul Dagnelie from comment #85)
> (In reply to Karl Tomlinson (:karlt) from comment #82)
> > The interface change to nsIFile needs superreview.
> > http://www.mozilla.org/hacking/reviewers.html
> The interface changes have already landed.

They can either be retrospectively reviewed or backed out. 

> > Is it reasonable to rely on javascript to explicitly unwatch the paths
> > that it watches?
> What other option is there?  If they don't unwatch them, the watches will be
> released on shutdown.  Other than that, there isn't much we can do.

The watch can be removed when the nsIFile goes out of scope (comment 63).

Or the methods can be hidden from javascript.

> > Is it clear to the client that the listener won't be destroyed until it
> > breaks the watch, even when there are no external references?
> > 
> > Similarly, is it clear that the nsIFile won't be destroyed until something
> > explicitly breaks the watch, even when there are no external references?
> Does it especially matter?  Will javascript users care when exactly the
> nsIFile goes away, or when the listener is gc'd?

I'm concerned that javascript clients won't care, or at least will expect
things to be cleaned up automatically, because that is what usually happens.

The nsIFile won't go away and the listener won't be GCed while the
nsFileWatcherService keeps a reference.

> > If watches can't be removed during update, then that needs to be
> > documented in the API rather than failing when the remove method is
> > called.  That failure could happen in a nested event loop and may not be
> > detected by the client.  The previous version of this patch built a list
> > of updates to issue before dispatch, so there were no problems with
> > removal during update.  That may also be necessary for handling inotify
> > events during nested event loops.
> > 
> > Perhaps the API can specify that nested event loops are not permitted, but I
> > don't know whether we could allow script to run in that case.  Being able to
> > remove a watch during update sounds like a useful feature.
> Unfortunately, doing it involves some ugly hacks, like manually checking for
> gc canary values.

I don't know what you had in mind here, but if a reference is added to the listeners and nsIFiles in the list of objects for event dispatch, then no GC will happen.

> > 
> > >+++ b/xpcom/io/nsFileWatcherServiceDefault.h
> > 
> > >+  NS_IMETHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> > >+  {
> > >+    return NS_ERROR_NOT_IMPLEMENTED;
> > >+  }
> > >+
> > >+  NS_IMETHOD RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> > >+  {
> > >+    return NS_ERROR_NOT_IMPLEMENTED;
> > >+  }
> > 
> > Can these methods be removed, given there is no concrete nsFileWatcherService
> > ever instantiated in this build?
> To remove them, we'd have to remove them from the abstract interface that
> defines this class, and I think it's useful to keep these defined so that
> the set of functions associated with this class is clear.  Also, in case
> someone does something silly like directly calling the constructor and
> creating one directly.

That's not something we need to support, but there is no
nsFileWatcherService constructor on these platforms.
The abstract interface shouldn't change or need to change.

> > A single path may point a directory and a file at different times.  To
> > catch these changes, an inotify watch needs to be on the parent directory
> > before the IsDirectory() test.  By induction this means starting with the
> > root directory (which won't be a file and won't get renamed).
> I don't think that this will help... having watch on the parent directories
> won't interrupt this operation, and won't enable us to change the process. 
> As soon as it starts, whenever it starts, the path could change from a
> directory to a file or vice versa.  There's no way to fix this short of
> having a lock on the file system somehow.

When there is already a watch before the IsDirectory() test, there will be
notifications of any changes made to the hierarchy and so adjustments will subsequently be made for those changes.

> > Watches in different parts of the tree, ancestors and descendants, have
> > different flags, but I don't see any watches being changed when new listeners
> > are added.  At least add comments to explain the differences and how they
> > are managed.
> I'm not sure what you mean here.  Why would watches on existing things be
> changed when a new listener is added?

Descendant directories need IN_MODIFY if they have files.  Ancestor
directories don't unless they are direct parents of watched files.  When a new
listener is added/removed, existing things may change to/from
descendant/ancestor state.

This is one level of complexity that can be left out of this patch I assume.
Let's try to keep it as simple as possible first to get something landed.

Comment 89

[Karl Tomlinson (:karlt)]

Comment on attachment 649491 [details] [diff] [review]
Linux patch 7.0

Forgot to hg mv or add, I assume.

Comment 90

[Brian R. Bondy [:bbondy]]

Comment on attachment 649491 [details] [diff] [review]
Linux patch 7.0

Review of attachment 649491 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsFileWatcherService.h
@@ +6,5 @@
> +
> +#ifndef nsFileWatcherService_h
> +#define nsFileWatcherService_h
> +
> +class nsFileWatcherServiceBase {

Please add a virtual destructor to this class

Comment 91

[Brian R. Bondy [:bbondy]]

Comment on attachment 648540 [details] [diff] [review]
Windows patch 1.1

Review of attachment 648540 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsFileWatcherServiceWin.cpp
@@ +47,5 @@
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CallbackData);
> +};
> +
> +class FileChangeEvent : public nsRunnable

Please add a comment that describes what the class is for

@@ +50,5 @@
> +
> +class FileChangeEvent : public nsRunnable
> +{
> +public:
> +  FileChangeEvent(nsFileWatcherService::CallbackData *aData, char *aBuffer, DWORD aLen)

Please add a javasdoc style comment with a description of what each parameter does. 
For aBuffer, please indicate that the class takes ownership of the passed in raw memory

@@ +74,5 @@
> +      nsString name;
> +      name.Assign(info->FileName, info->FileNameLength/sizeof(wchar_t));
> +      
> +      if ( (mData->mLeafName.Length() > 0 && !name.Equals(mData->mLeafName)) || (info->Action & FILE_ACTION_RENAMED_OLD_NAME)) {
> +        i+=info->NextEntryOffset;

i += info->NextEntryOffset;
nit: here and elsewhere please put a space before and after all operators, assignments, and comparators.

@@ +116,5 @@
> +  nsAutoPtr<char> mBuffer;
> +  DWORD mLen;
> +};  
> +
> +VOID CALLBACK CallbackHandler(DWORD aErrorCode, DWORD aNumberOfBytesTransferred, LPOVERLAPPED aOverlap)

nit: Here and elsewhere, please put the type on the return type and calling convention on the previous line:
VOID CALLBACK
CallbackHandler(DWORD aErrorCode, DWORD aNumberOfBytesTransferred, LPOVERLAPPED aOverlap)

@@ +124,5 @@
> +    NS_Free(aOverlap);
> +    return;
> +  }
> +
> +  char *temp_buffer =(char *) NS_Alloc(aNumberOfBytesTransferred); //we copy the data to a new buffer so that we can start listening again ASAP

nit: Move this comment to the previous line

@@ +128,5 @@
> +  char *temp_buffer =(char *) NS_Alloc(aNumberOfBytesTransferred); //we copy the data to a new buffer so that we can start listening again ASAP
> +  memcpy(temp_buffer, data->mUpdateBuffer, aNumberOfBytesTransferred);
> +  bool success = ReadDirectoryChangesW(data->mDir, data->mUpdateBuffer, 1024 , TRUE, 
> +                                       FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_CREATION | 
> +                                       FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME, nsnull, 

nit: Use NULL instead of nsnull in this file
nit2: if (!success) { 
  NS_WARNING("The ReadDirectoryChangesW operation could not be queued");
}

@@ +133,5 @@
> +                                       aOverlap, CallbackHandler);
> +  NS_DispatchToMainThread(new FileChangeEvent(data, temp_buffer, aNumberOfBytesTransferred));
> +}
> +
> +void __stdcall AddDir(ULONG_PTR aParam)

nit: Please put return type and calling convention on the previous line.

@@ +140,5 @@
> +  NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> +  bool success = ReadDirectoryChangesW(aData->mDir, aData->mUpdateBuffer, 1024 , TRUE, 
> +                                       FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_CREATION | 
> +                                       FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME,
> +                                       NULL, aData->mOverlap, CallbackHandler);

nit: if (!success) { 
  NS_WARNING("....
}

@@ +143,5 @@
> +                                       FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME,
> +                                       NULL, aData->mOverlap, CallbackHandler);
> +}
> +
> +void __stdcall Wakeup(ULONG_PTR aParam)

nit: Please put return type and calling convention on the previous line.

@@ +148,5 @@
> +{
> +  return;
> +}
> +
> +void __stdcall Cancel(ULONG_PTR aParam)

nit: Please put return type and calling convention on the previous line.

@@ +159,5 @@
> +{
> +  for(unsigned int i=0;i<aArray->Length();) {
> +    QueueUserAPC(Cancel,(HANDLE)aUserArg, (ULONG_PTR)(*aArray)[i]->mDir);
> +    aArray->RemoveElementAt(i);
> +  }

I think this equivalence would be easier to read, or just add a comment explaining that you are clearing the whole list:
while(mArray->Length()) {
  QueueUserAPC(Cancel, (HANDLE)aUserArg, (ULONG_PTR)(*aArray)[0]->mDir);
  aArray->RemoveElementAt(0);
}

@@ +171,5 @@
> +  }
> +  return 0;
> +}
> +
> +nsFileWatcherService::nsFileWatcherService()

Init mThread to INVALID_HANDLE_VALUE here and mRunning to 0.

@@ +177,5 @@
> +}
> +
> +nsresult
> +nsFileWatcherService::Init()
> +{

Check to make sure Init wasn't already called.

@@ +181,5 @@
> +{
> +  mTable.Init();
> +  mRunning = 1;
> +  mThread = (HANDLE)_beginthreadex(NULL, 0, EventLoop2,
> +                                   &mRunning, 0, NULL);

Check to make sure we don't get a return of INVALID_HANDE_VALUE

@@ +198,5 @@
> +{
> +}
> +
> +void nsFileWatcherService::Shutdown()
> +{

Check to make sure Init was called.

@@ +201,5 @@
> +void nsFileWatcherService::Shutdown()
> +{
> +  mRunning--;
> +  mTable.Enumerate(TableDestroyEnumerator, nsnull);
> +  QueueUserAPC(Wakeup, mThread, nsnull);

nit: NULL for these 2 lines

@@ +206,5 @@
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  if (observerService) {
> +    observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +  }
> +  WaitForSingleObject(mThread,INFINITE);

nit: Space before each argument
WaitForSingleObject(mThread, INFINITE);

@@ +211,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsFileWatcherService::AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> +{

Check to make sure init was called

@@ +224,5 @@
> +  }
> +  else{
> +    aFile->GetPath(path);
> +  }
> +  HANDLE dir = CreateFile(path.get(), FILE_LIST_DIRECTORY, // needed for ReadDirectoryChangesW

You never close this handle, you should close it from within the CallbackData destructor
nit: Use CreateFileW

@@ +227,5 @@
> +  }
> +  HANDLE dir = CreateFile(path.get(), FILE_LIST_DIRECTORY, // needed for ReadDirectoryChangesW
> +                          FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, //allow all these operations
> +                          NULL, OPEN_EXISTING, //we're not actually trying to create it
> +                          FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, //again for ReadRiectoryChangesW

nit: For comments here and elsewhere please add a space after the // and before the comment text.

@@ +235,5 @@
> +  }
> +
> +  char *buffer = (char *)NS_Alloc(1024);
> +
> +  OVERLAPPED *overlap = (OVERLAPPED *)NS_Alloc(sizeof(OVERLAPPED));

This is never freed, it's probably best for CallbackData to take ownership.

@@ +243,5 @@
> +  
> +  aFile->GetPath(path);
> +  nsTArray<CallbackData *> *arr = mTable.Get(path);
> +  if (!arr) {
> +    arr = new nsTArray<CallbackData *>();

This memory is never freed

@@ +249,5 @@
> +  }
> +  arr->AppendElement((CallbackData *)overlap->hEvent);
> +  mRunning++;
> +  
> +  QueueUserAPC(AddDir, mThread, (ULONG_PTR)overlap->hEvent);

if (!QueueUserAPC(...)) {
 Error handling here, return an error code too.
}

@@ +256,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsFileWatcherService::RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener)
> +{

Check to make sure Init was called

@@ +272,5 @@
> +
> +  if (!data) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  QueueUserAPC(Cancel, mThread, (ULONG_PTR)data->mDir);

if (!QueueUserAPC(...)) {
 Error handling here, return an error code too.
}

@@ +287,5 @@
> +
> +//static
> +nsFileWatcherService * nsFileWatcherService::GetService()
> +{
> +  if (gFileWatcher == nsnull) {

nit: if(!gFileWatcher) {

@@ +299,5 @@
> +nsFileWatcherService::Observe(nsISupports* aSupport, const char *aTopic, const PRUnichar *aData)
> +{
> +  if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
> +    Shutdown();
> +    gFileWatcher = nsnull;

nit: NULL

::: xpcom/io/nsFileWatcherServiceWin.h
@@ +22,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +
> +  NS_SCRIPTABLE NS_METHOD AddFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener);
> +  NS_SCRIPTABLE NS_METHOD RemoveFileWatcher(nsIFile *aFile, nsIFileUpdateListener *aListener);

Remove NS_SCRITABLE on the above 2 lines.

Comment 92

[Benjamin Smedberg]

Comment on attachment 647801 [details] [diff] [review]
interface changes 2

From IRC discussion: on non-mac we can give reliable "created" "deleted" "modified" markers for the type of change (on nsIFileUpdateListener.update). On mac we don't have that information directly, but we could synthesize that information by statting the file and comparing it with previous information.

If that is the plan, then we should remove the "unknown" modification type and simply do the synthesizing in the mac impl (and make sure the tests cover all the relevant cases).

Comment 93

[Paul Dagnelie]

Comment on attachment 647801 [details] [diff] [review]
interface changes 2

bsmedberg, you've already looked at the interface changes, can you provide a retroactive superreview+ ?

Comment 94

[Paul Dagnelie]

I see I was late to the party, my apologies :).  I'll get on that change as well as others.

Comment 95

[Paul Dagnelie]

Attachment: Interface changes patch

Here it is.  I'll change the mac implementation to obey this soon.

Comment 96

[Doug Turner (:dougt)]

 Interface changes patch :
    https://hg.mozilla.org/integration/mozilla-inbound/rev/095da8941d4c

Comment 97

[Karl Tomlinson (:karlt)]

One key thing to sort out here is whether it is appropriate for the nsFileWatcherService to own nsIFiles and nsIFileUpdateListeners with no external references.

nsIFile's could be managed by keeping only bare (effectively weak) pointers to nsIFiles from nsFileWatcherService.  The destructor of the nsIFile implementation could notify the nsFileWatcherService that the nsIFile will be destroyed so that nsFileWatcherService can clear the nsIFile from the watch/directory structures.

nsIFileUpdateListeners are more difficult.  One option might be that nsIFile::watch require the nsIFileUpdateListener to support nsIWeakReference, and watch would fail if not.

Compare nsIObserverService::addObserver, which makes it clear whether the observer service holds a strong or a weak reference.

Given that existing model permitting strong references, I guess we can let nsIFileUpdateListeners be owned by the nsFileWatcherService.
If taking that approach, then the documentation of nsIFile::watch needs to make it clear that the nsIFileUpdateListener will live until the nsIFile is explicitly unwatched.  Note that the nsIFile client doesn't know that there is such a thing as an nsFileWatcherService.

The same extends to nsIFile.  If it will live on after its last external reference is removed, then the watch documentation needs to make that clear.

Comment 98

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/095da8941d4c

Comment 99

[Doug Turner (:dougt)]

we really should have factored this bug into sub-bugs.

Comment 100

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 101

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 102

[David Flanagan [:djf]]

Without some kind of notification, gaia media apps must rescan the entire /sdcard each time they become visible.  When the user takes a picture with the camera and then switches to the gallery, the gallery finds that picture by scanning for new files. The current code does not scale well and we need some kind of notification of new files.

FileWatcherService may be more than we need for B2G devices, however.  It would probably be good enough for v1 if device storage itself sent out notifications to all clients.  That is, if the camera writes a file while the gallery app is running, the gallery app should get a notification of that. On the desktop we have to deal with the case of other unrelated programs adding files to media directories and probably need FileWatcherService for that.

But some form of device storage notifications should block basecamp.

Comment 103

[Paul Dagnelie]

Attachment: Linux patch 7.1

This patch adds removal of watches when the relevant nsIFile falls out of scope, and adds a roadmap and documentation of the system, since I'll be leaving tomorrow.

Comment 104

[Paul Dagnelie]

Attachment: Windows patch 1.2

This patch again adds documentation and a roadmap.

Comment 105

[Paul Dagnelie]

Attachment: Mac patch 1.3

This patch adds documentation and a roadmap.

Comment 106

[David Flanagan [:djf]]

:dougt if this bug is no longer blocking, are you working on notifications that just work for B2G? What's the bug number, and should we nominate that one for blocking?

Comment 107

[Doug Turner (:dougt)]

- minusing. 

so, what we are going to do instead is to track changes caused by device storage internally, and send onchange notifications for those.  I'll file/nom that bug shortly.

Comment 108

[Doug Turner (:dougt)]

Comment on attachment 649718 [details] [diff] [review]
Interface changes patch

When bug 782352, I plan to back out these interface changes.  Gaia needs something now, and we can do something a bit easier for them.  When the implementations are more fully functional, we can reland the interface changes.

In retrospect, we probably shouldn't have landed these interface changes without implementations behind them.  My bad.

Comment 109

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edc8f91fd747
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d5892177f44

Comment 110

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edc8f91fd747
https://hg.mozilla.org/mozilla-central/rev/6d5892177f44

Comment 111

[Doug Turner (:dougt)]

To be clear, no changes from this bug landed on any mozilla branch.

Comment 112

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 650732 [details] [diff] [review]
Mac patch 1.3

Do I still need to review this?

Comment 113

[Andrew McCreight [:mccr8]]

This got backed out a decade ago, so I'll mark it WONTFIX. If somebody wants to revive this, it would probably be better to file a new bug.