Splitting bug 958280 in per-platform implementations.

As you requested, just to see if we are on the same wavelength :D

Comment on attachment 8405488 [details] nsINativeOSFileWatcher.idl Looks good, I believe that we are on the same wavelength. A few nits, while I'm here. === General remark: it's probably going to be independent from OS.File, so let's remove any reference to OS.File. > A service providing native implementations of path changes notification. If I understand your code correctly, you can have many instances of the watcher running simultaneously, so it's a "component" rather than a "service". > Watches the passed path for changes. Could you specify further? What happens if this is a directory? > The constructor implementation. This sentence looks out of place. > @param pathToUnwatch The path to un-watch. Could you specify further? What happens if the path isn't watched?

This allows user to watch a folder for changes in its containing files on Windows. I just have a few doubts: - Should I report the errors using the Error Callback in places like NativeWatcherWin.cpp:319? Instead of using NS_ERROR? - Is it right to return NS_ERROR_FAILURE when there's an error like file is not found (see the same location of the previous point)?

Comment on attachment 8413330 [details] [diff] [review] bug958280.patch Review of attachment 8413330 [details] [diff] [review]: ----------------------------------------------------------------- It's a good start. I'd like a number of fixes and clarification before we proceed, though. Nit: You should probably replace "watcher" with "file watcher" just about everywhere in this patch. ::: toolkit/components/watcher/NativeWatcherModule.cpp @@ +5,5 @@ > + > +#include "mozilla/ModuleUtils.h" > +#include "nsIClassInfoImpl.h" > + > +// TODO: load different headers based on the architecture Side-note: We're not going to land something with such a big TODO. I believe that you should provide a placeholder for other platforms that simply returns NS_ERROR_NOT_IMPLEMENTED from each method. @@ +7,5 @@ > +#include "nsIClassInfoImpl.h" > + > +// TODO: load different headers based on the architecture > +#include "NativeWatcherWin.h" > + Most (all?) of the contents of this file should be moved to toolkit/components/build/nsToolkitCompsModule.cpp . ::: toolkit/components/watcher/NativeWatcherWin.cpp @@ +12,5 @@ > +#include "NativeWatcherWin.h" > + > +// the number of notifications stored within the buffer inside > +// WatchedResourceDescriptor:notificationBuffer > +#define WATCHED_RES_MAXIMUM_NOTIFICATIONS 25 This is C++, let's make it a const. @@ +16,5 @@ > +#define WATCHED_RES_MAXIMUM_NOTIFICATIONS 25 > + > +/** > + * A structure to keep track of the information related to a > + * watched resource. Can you document the fields? @@ +19,5 @@ > + * A structure to keep track of the information related to a > + * watched resource. > + */ > +struct WatchedResourceDescriptor { > + nsAutoString path; nsAutoString is on the stack. Let's make it nsString. @@ +20,5 @@ > + * watched resource. > + */ > +struct WatchedResourceDescriptor { > + nsAutoString path; > + nsTArray<unsigned char> notificationBuffer; Looks like this should be a nsCString. @@ +28,5 @@ > + > +/** > + * An event used to notify the main thread when an error happens. > + */ > +class WatchedErrorEvent MOZ_FINAL : public nsRunnable { Side-note: we cannot use NS_RunnableMethodWithArg as we need to make sure that nsINativeWatcherErrorCallback is released on the main thread. Filed bug 1003061 to provide a better solution. @@ +60,5 @@ > +/** > + * An event used to notify the main thread of a change in a watched > + * resource. > + */ > +class WatchedChangeEvent MOZ_FINAL : public nsRunnable { Same here. @@ +89,5 @@ > +}; > + > +/** > + * A runnable to dispatch from the main thread to get the notifications > + * of the changes in the watched resources. I'm not completely satisfied with this description. Could you make it clearer that this defines the entire behavior of the IO thread? @@ +111,5 @@ > + > + /** > + * The watching thread logic. > + */ > + NS_IMETHOD Run() { For defensive purposes, I'd assert that we not on the main thread. Also, can you set the name of the thread, using NS_SetThreadName()? @@ +113,5 @@ > + * The watching thread logic. > + */ > + NS_IMETHOD Run() { > + OVERLAPPED* overlappedStructure; > + DWORD transferredBytes = 0; What's that for? @@ +121,5 @@ > + // check for changes in the resource status by querying the |mIOCompletionPort|. > + // GetQueuedCompletionStatus waits until there's a change or an error. > + if (!GetQueuedCompletionStatus(mIOCompletionPort, &transferredBytes, > + &changedResourceHandle, &overlappedStructure, > + INFINITE)) { Can you comment that this is a blocking call? @@ +129,5 @@ > + // some path was unwatched! that's not really an error, > + // just call GetQueuedCompletionStatus again. > + continue; > + } > + Nit: Please check your whitespace. Look at the review mode to see misplaced whitespace. @@ +131,5 @@ > + continue; > + } > + > + // notify the error back to the main thread > + nsAutoString errorMsg(L"There was an error while watching the requested resources."); Please use NS_LITERAL_STRING everywhere instead of L. Also, if I understand your code correctly, there is no way to recover from this error. Please make this clear in the doc and the string. @@ +135,5 @@ > + nsAutoString errorMsg(L"There was an error while watching the requested resources."); > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mOnError, errorMsg); > + NS_DispatchToMainThread(errorRunnable); > + break; What can cause errors here? @@ +142,5 @@ > + // be sure to lock the resources we need > + mozilla::MutexAutoLock lock(mResourcesLock); > + > + // check to see which resource is changedResourceHandle > + WatchedResourceDescriptor* changedRes = nullptr; I believe that you should make WatchedResourceDescriptor owned entirely by the IO thread. This will save you a mutex which could possibly block the main thread for a little time. @@ +144,5 @@ > + > + // check to see which resource is changedResourceHandle > + WatchedResourceDescriptor* changedRes = nullptr; > + WatchedResourceDescriptor** it = mWatchedResources.begin(); > + for(it; it != mWatchedResources.end(); ++it) { Why didn't you define |it| in the initializer of the for()? @@ +150,5 @@ > + // we've found our guy > + changedRes = *it; > + break; > + } > + } I understand that you use linear search because you assume that the number of files is small. I need to think whether it's the right strategy. Also, you need to document this choice. @@ +159,5 @@ > + nsAutoString errorMsg(L"Some resource changed, but we are not watching it anymore."); > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mOnError, errorMsg); > + NS_DispatchToMainThread(errorRunnable); > + continue; Why is this an error? @@ +164,5 @@ > + } > + > + // parse the changes and notify the main thread > + unsigned char* rawNotificationBuffer = > + (unsigned char*)(changedRes->notificationBuffer.Elements()); I don't understand why you use an unsigned char* rather than a string. @@ +202,5 @@ > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mOnError, errorMsg); > + NS_DispatchToMainThread(errorRunnable); > + } > + } while(1); We generally prefer `while(true)`. Also, just a matter of taste, but I'd use a `while` instead of a `do...while`, as it makes easier to spot the structure when starting the loop. (same thing for your other loop) @@ +252,5 @@ > + NS_ENSURE_TRUE(mIOCompletionPort, NS_ERROR_FAILURE); > + > + // to create a new thread, get the thread manager > + nsCOMPtr<nsIThreadManager> threadManager = do_GetService(NS_THREADMANAGER_CONTRACTID); > + nsresult rv = threadManager->NewThread(0, 0, getter_AddRefs(mIOThread)); NS_NewThread() is a bit more readable. @@ +292,5 @@ > + // retrieve a file handle to associate with the completion port. Makes > + // sure to request the appropriate rights (i.e. read files and list > + // files contained in a folder). Note: the nullptr security flag prevents > + // the HANDLE to be passed to child processes. > + HANDLE resHandle = CreateFileW(PromiseFlatString(pathToWatch).get(), CreateFileW (and a few other functions called in this method) requires file I/O. You should could call it on the IO thread. @@ +306,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + // initialise the resource descriptor > + WatchedResourceDescriptor* resourceDesc = new WatchedResourceDescriptor; Please use a ScopedPtr, to avoid any leaks. @@ +339,5 @@ > + // TODO: send back an error Event? > + return NS_ERROR_FAILURE; > + } > + > + mozilla::MutexAutoLock lock(mResourcesLock); As mentioned above, I don't like this lock. I'd prefer if you just sent the string to the IO thread and let the IO thread do everything from here. ::: toolkit/components/watcher/NativeWatcherWin.h @@ +24,5 @@ > + NS_DECL_NSINATIVEWATCHER > + > + NativeWatcher(); > + > +private: Could you document these fields? @@ +30,5 @@ > + > + mozilla::Vector<WatchedResourceDescriptor*> mWatchedResources; > + > + HANDLE mIOCompletionPort; > + mozilla::Mutex mResourcesLock; In particular, what does this mutex protect? ::: toolkit/components/watcher/nsINativeWatcher.idl @@ +14,5 @@ > +{ > + /** > + * @param errorMessage A message describing the error occurred. > + */ > + void complete(in AString errorMessage); A string is nice but a nsresult would be more useful. @@ +46,5 @@ > + * @param onError > + * The callback invoked whenever an error occurs. > + */ > + [implicit_jscontext] void init(in nsINativeWatcherCallback onChange, > + in nsINativeWatcherErrorCallback onError); I believe that you don't use the jscontext, so just get rid of it. @@ +50,5 @@ > + in nsINativeWatcherErrorCallback onError); > + > + /** > + * Watches the passed path for changes. If it's a folder, every file > + * it contains is watched. Does not recursively watch folders. Can you check whether this specification can be efficiently implemented under non-Windows platforms? Also, looking at your implementation, I believe that this cannot watch a single file, am I right? ::: toolkit/components/watcher/tests/xpcshell/head.js @@ +4,5 @@ > +"use strict"; > + > +let {utils: Cu, interfaces: Ci} = Components; > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); You don't seem to use it. ::: toolkit/components/watcher/tests/xpcshell/test_watcher.js @@ +18,5 @@ > + // space. > + do_get_profile(); > + > + let dir = OS.Constants.Path.profileDir; > + Well, missing test, obviously.

(In reply to Alessio Placitelli [:Dexter] from comment #3) > Created attachment 8413330 [details] [diff] [review] > bug958280.patch > > This allows user to watch a folder for changes in its containing files on > Windows. I just have a few doubts: > > - Should I report the errors using the Error Callback in places like > NativeWatcherWin.cpp:319? Instead of using NS_ERROR? By default, let's report all errors in the callback. > - Is it right to return NS_ERROR_FAILURE when there's an error like file is > not found (see the same location of the previous point)? We have more precise error numbers for this. See http://dxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h?from=ErrorList.h#1 (it's automatically included).

This should include all the changes you suggested about the patch I've previously included.

Comment on attachment 8417362 [details] [diff] [review] bug958280.patch Review of attachment 8417362 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +24,5 @@ > + // the path on the file system of the watched resource > + nsString path; > + > + // a buffer containing the latest notifications received for the resource > + nsTArray<unsigned char> notificationBuffer; I'm not really convinced by `notificationBuffer`. Do I understand correctly that you are always using it as a pointer? @@ +27,5 @@ > + // a buffer containing the latest notifications received for the resource > + nsTArray<unsigned char> notificationBuffer; > + > + // the OS handle to the watched resource > + HANDLE resourceHandle; We probably want to make this an instance of Scoped<> that auto-closes the resource. @@ +30,5 @@ > + // the OS handle to the watched resource > + HANDLE resourceHandle; > + > + // used to hold information for the asynchronous ReadDirectoryChangesW call > + OVERLAPPED overlappedInfo; Does this need to be auto-closed, too? @@ +159,5 @@ > + } > + > + // If we reach this point, mIOCompletionPort was probably closed > + // and we need to close this thread. This condition is identified > + // by catching the ERROR_INVALID_HANDLE error. Do I understand correctly that closing the completion port is something that happens legitimately during shutdown? If so, in case of ERROR_INVALID_HANDLE, you should probably return status NS_OK. @@ +177,5 @@ > + // closed (for network connections). This basically mean we just unwatched > + // a resource. We skip this and continue watching. > + // Note: if changedResourceHandle is nullptr as well, the wait on the Completion > + // I/O was stopped by a PostQueuedCompletionStatus call to allow addPath and > + // removePath to work on this thread. I don't understand how/when this can happen for a file HANDLE. @@ +180,5 @@ > + // I/O was stopped by a PostQueuedCompletionStatus call to allow addPath and > + // removePath to work on this thread. > + return NS_DispatchToCurrentThread(this); > + } > + What about the case in which !transferredBytes && !overlappedStructure, which I believe you reach when you call PostQueuedCompletionStatus? @@ +189,5 @@ > + WatchedResourceDescriptor* changedRes = > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > + if (!changedRes) { > + // We couldn't find the resource. This can also happen when calling > + // removePath from the main thread. I don't understand why should cause an error. @@ +208,5 @@ > + > + // FileNameLength is in bytes, but we need FileName length > + // in characters, so divide it by sizeof(WCHAR). > + nsAutoString test(notificationInfo->FileName, > + notificationInfo->FileNameLength / sizeof(WCHAR)); `test` is not a very good name @@ +211,5 @@ > + nsAutoString test(notificationInfo->FileName, > + notificationInfo->FileNameLength / sizeof(WCHAR)); > + nsRefPtr<WatchedChangeEvent> changeRunnable = > + new WatchedChangeEvent(mOnChange, test); > + NS_DispatchToMainThread(changeRunnable); rv = NS_DispatchToMainThread(changeRunnable); if (NS_FAILED(rv)) { return rv; } Same thing for other calls to NS_DispatchToMainThread. @@ +228,5 @@ > + mNotificationBufferSize, > + false, // watch subtree (recurse) > + FILE_NOTIFY_CHANGE_CREATION > + | FILE_NOTIFY_CHANGE_LAST_WRITE > + | FILE_NOTIFY_CHANGE_FILE_NAME, File removal would be useful, too. @@ +231,5 @@ > + | FILE_NOTIFY_CHANGE_LAST_WRITE > + | FILE_NOTIFY_CHANGE_FILE_NAME, > + &dwPlaceholder, > + &changedRes->overlappedInfo, > + nullptr)) { What can cause this kind of error? @@ +273,5 @@ > + * @return NS_OK if there was no error with thread creation and execution. > + */ > +NS_IMETHODIMP > +NativeFileWatcher::Init(nsINativeFileWatcherCallback* aOnChange, > + nsINativeFileWatcherErrorCallback* aOnError) { You need to fail if the watcher is already initialized. @@ +373,5 @@ > + */ > +NS_IMETHODIMP > +NativeFileWatcher::Destroy() { > + > + { // Begin locked section I'd prefer if you could dispatch a runnable to perform all of this off the main thread. This runnable will need to take ownership of mWatchedResourceByHandle, mWatchedResourcesByPath, etc. @@ +419,5 @@ > + * otherwise NS_OK. > + */ > +nsresult NativeFileWatcher::addPathRunnableMethod(const nsAString& pathToWatch) { > + > + mozilla::MutexAutoLock lock(mResourcesLock); Once you have moved all the destruction off the main thread, you'll be able to get rid of this lock. @@ +423,5 @@ > + mozilla::MutexAutoLock lock(mResourcesLock); > + > + // Is pathToWatch already being watched? > + if (mWatchedResourcesByPath.Get(pathToWatch)) { > + mErrorCallback->Complete(NS_ERROR_FILE_ALREADY_EXISTS); Coding convention: if you disregard the nsresult of a method, please cast the result to void. @@ +431,5 @@ > + // Retrieve a file handle to associate with the completion port. Makes > + // sure to request the appropriate rights (i.e. read files and list > + // files contained in a folder). Note: the nullptr security flag prevents > + // the |HANDLE| to be passed to child processes. > + HANDLE resHandle = CreateFileW(PromiseFlatString(pathToWatch).get(), To make the code more robust, you should use a subclass of Scoped<> to auto-close this HANDLE. @@ +442,5 @@ > + if (resHandle == INVALID_HANDLE_VALUE) { > + DWORD dwError = GetLastError(); > + nsresult rv = (dwError == ERROR_FILE_NOT_FOUND) > + ? NS_ERROR_FILE_NOT_FOUND > + : NS_ERROR_FAILURE; Function ConvertWinError can convert many errors to something more useful. Perhaps in a followup bug http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#468 @@ +444,5 @@ > + nsresult rv = (dwError == ERROR_FILE_NOT_FOUND) > + ? NS_ERROR_FILE_NOT_FOUND > + : NS_ERROR_FAILURE; > + mErrorCallback->Complete(rv); > + return rv; The error has already been reported. I'd return NS_OK. ::: toolkit/components/filewatcher/NativeFileWatcherWin.h @@ +34,5 @@ > + // Since |HANDLE| is basically a typedef to void*, we use nsVoidPtrHashKey to > + // compute the hashing key. We need 2 indexes in order to quickly look up the > + // changed resource in the Worker Thread. > + // The objects are not ref counted and get destroyed by mWatchedResourcesByPath > + // on NativeFileWatcher::Destroy or in NativeFileWatcher::RemovePath. That sounds like a possible race condition during shutdown. @@ +37,5 @@ > + // The objects are not ref counted and get destroyed by mWatchedResourcesByPath > + // on NativeFileWatcher::Destroy or in NativeFileWatcher::RemovePath. > + nsClassHashtable<nsStringHashKey, WatchedResourceDescriptor> mWatchedResourcesByPath; > + nsDataHashtable<nsVoidPtrHashKey, WatchedResourceDescriptor*> mWatchedResourcesByHandle; > + Please document for each field to which thread they belong. ::: toolkit/components/filewatcher/tests/xpcshell/test_watcher.js @@ +14,5 @@ > + do_test_pending(); > + > + // Set up profile. We will use profile path create some test files > + do_get_profile(); > + Since you will have a number of behavior to test, you should use add_task for each sub-test. @@ +19,5 @@ > + let watchedDir = OS.Constants.Path.profileDir; > + let tempFileName = 'test.txt'; > + > + // instantiate the native watcher > + var watcher = Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] let @@ +50,5 @@ > + // remove the watch and free the associated memory > + watcher.removePath(watchedDir); > + watcher.destroy(); > + watcher = null; > +} You should also test: 1/ what happens if we add a path that doesn't exist; 2/ that the component correctly informs us when we create/modify/remove a file/directory in the path; 3/ that we can monitor several paths at once.

This new patch implements all the modification you suggested. I've decided to split the xpcshell tests in different files: I find it easier to test it this way but I'm open to other suggestions!

My bad, I forgot to add the test files to the patch.

Comment on attachment 8422639 [details] [diff] [review] bug958280.patch Review of attachment 8422639 [details] [diff] [review]: ----------------------------------------------------------------- In the next version of your patch, could you make one patch for the tests and one for the implementation? This will make it easier to review by chunks. Note: I have reviewed a fest tests, not all of them. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +11,5 @@ > +#include "nsServiceManagerUtils.h" > +#include "nsTArray.h" > +#include "nsVariant.h" > +#include "nsXPCOMCIDInternal.h" > + Oh, I forgot to tell you about namespaces. Everything that is not exported should go in an anonymous namespace. Everything that is exported should go in namespace `mozilla`. @@ +13,5 @@ > +#include "nsVariant.h" > +#include "nsXPCOMCIDInternal.h" > + > +// the number of notifications stored within the buffer inside > +// WatchedResourceDescriptor:notificationBuffer Nit: Please make this a full sentence (well, at least something with uppercase and dot). Same thing for your other comments/docs. @@ +43,5 @@ > + // the path on the file system of the watched resource > + nsString path; > + > + // a buffer containing the latest notifications received for the resource > + mozilla::ScopedDeleteArray<unsigned char> notificationBuffer; Can you document why this isn't a ScopedDeleteArray<FILE_NOTIFY_INFORMATION>? @@ +62,5 @@ > + /** > + * @param aOnError The passed error callback. > + * @param aError The |nsresult| error value. > + */ > + WatchedErrorEvent(const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError, I believe that this shouldn't be a reference. Note that this will need a second review from someone more familiar than me of `nsMainThreadPtr*`. @@ +64,5 @@ > + * @param aError The |nsresult| error value. > + */ > + WatchedErrorEvent(const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError, > + const nsresult& anError) > + : mOnError(aOnError) I believe that this should be mOnError = new nsMainThreadPtrHolder<...>(aOnError) Same thing for other uses of nsMainThreadPtr*. @@ +88,5 @@ > +/** > + * An event used to notify the main thread of a change in a watched > + * resource. > + */ > +class WatchedChangeEvent MOZ_FINAL : public nsRunnable { As above. @@ +139,5 @@ > + const nsMainThreadPtrHandle<nsINativeFileWatcherCallback>& aOnChange, > + const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError) > + : mIOCompletionPort(aIOCompletionPort) > + , mWatchedResourcesByPath(aWatchedResourcesByPath) > + , mWatchedResourcesByHandle(aWatchedResourcesByHandle) I suspect that we could make this runner own `mWatchedResourceBy*` and [de]allocate them entirely off the main thread, too, as follows: 1. add a field `mInitialized` to `NativeFileWatcherIOTask`, initialized to `false`, and owned entirely by the worker thread; 2. move the current code of `Run()` into a method `Loop()`; 3. create a new method `Init()` to a. initialize the stuff above; b. set `mInitialized` to `true`; 4. when you start `Run()`, if `mInitialized` is `false`, execute method `Init()` before executing method `Loop()`. This will be imperceptibly slower (one `if` off the main thread per loop iteration), but this should also help keep the cleanup off the main thread. @@ +149,5 @@ > + /** > + * The watching thread logic. > + */ > + NS_IMETHOD Run() { > + // Assure that we're not on the main thread! Nit: That comment is probably not necessary. @@ +162,5 @@ > + DWORD transferredBytes = 0; > + > + // Will hold the |HANDLE| to the watched resource returned by GetQueuedCompletionStatus > + // which generated the change events > + ULONG_PTR changedResourceHandle = 0; Why a `ULONG_PTR` and not a `HANDLE` set to `INVALID_HANDLE_VALUE`? @@ +164,5 @@ > + // Will hold the |HANDLE| to the watched resource returned by GetQueuedCompletionStatus > + // which generated the change events > + ULONG_PTR changedResourceHandle = 0; > + > + // Check for changes in the resource status by querying the |mIOCompletionPort|. Nit: // Check for changes in the resource status by querying the |mIOCompletionPort| (blocking). The rest of the comment is not very useful. @@ +172,5 @@ > + &changedResourceHandle, &overlappedStructure, > + INFINITE)) { > + // Ok, there was some error > + DWORD errCode = GetLastError(); > + switch (errCode) { I believe that you should also handle `ERROR_NOTIFY_ENUM_DIR` and `STATUS_NOTIFY_ENUM_DIR`, both of which specify that there have been too many changes. In this case, I'd suggest dispatching a success with a special value "*" (once we have arrays, an empty array) – and documenting the behavior. @@ +174,5 @@ > + // Ok, there was some error > + DWORD errCode = GetLastError(); > + switch (errCode) { > + case ERROR_ABANDONED_WAIT_0: > + case ERROR_INVALID_HANDLE: { Mmmmhh... according to the documentation, under Windows XP, ERROR_ABANDONED_WAIT_0 doesn't work nicely and will wait until timeout. We should probably make sure that the timeout is not infinite at least on Windows XP. @@ +179,5 @@ > + // If we reach this point, mIOCompletionPort was probably closed > + // and we need to close this thread. This condition is identified > + // by catching the ERROR_INVALID_HANDLE error. > + return NS_OK; > + break; Nit: No `break` after `return`. Same below. @@ +199,5 @@ > + > + return NS_DispatchToCurrentThread(this); > + break; > + } > + default: { Given the number of places in which we can have errors, I believe that you should add logging. I suspect that this will save everybody's life some day. See http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog.h?from=prlog.h . @@ +204,5 @@ > + // It should probably never get here, but it's better to be safe. > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mOnError, NS_ERROR_FAILURE); > + nsresult rv = NS_DispatchToMainThread(errorRunnable); > + NS_ENSURE_SUCCESS(rv, rv); This macro is deprecated. You should now write if (NS_FAILED(rv)) { return rv; } Same for other uses of `NS_ENSURE_SUCCESS`. @@ +206,5 @@ > + new WatchedErrorEvent(mOnError, NS_ERROR_FAILURE); > + nsresult rv = NS_DispatchToMainThread(errorRunnable); > + NS_ENSURE_SUCCESS(rv, rv); > + > + return NS_ERROR_FAILURE; Looks like a NS_OK. @@ +233,5 @@ > + WatchedResourceDescriptor* changedRes = > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > + if (!changedRes) { > + // We couldn't find the resource. This can also happen when calling > + // removePath from the main thread. Can this still happen or should this be replaced with an assertion failure? @@ +273,5 @@ > + DWORD dwPlaceholder; > + if (!ReadDirectoryChangesW(changedRes->resourceHandle, > + changedRes->notificationBuffer.rwget(), > + mNotificationBufferSize, > + false, // watch subtree (recurse) I believe that we decided to recurse. @@ +303,5 @@ > + nsString::const_iterator delim_begin(begin), > + delim_end(end); > + > + // Is the path using '\\' or '/'? Search for last '\' at the end of the string > + if (!RFindInReadable(NS_LITERAL_STRING("\\"), delim_begin, delim_end)) { You should be able to use `path.RFindChar('\\')`. @@ +305,5 @@ > + > + // Is the path using '\\' or '/'? Search for last '\' at the end of the string > + if (!RFindInReadable(NS_LITERAL_STRING("\\"), delim_begin, delim_end)) { > + // Okay, the path is using '/' > + return false; I'd go the other way and assume that the path uses '\\' unless there is at least one '/'. @@ +358,5 @@ > + mNotificationBufferSize = > + WATCHED_RES_MAXIMUM_NOTIFICATIONS * sizeof(FILE_NOTIFY_INFORMATION); > + > + // Creates an IO completion port and allows at most 2 thread to access it concurrently > + mIOCompletionPort = CreateIoCompletionPort((HANDLE)INVALID_HANDLE_VALUE, nullptr, 0, 2); Nit: Could you add a small comment for each non-trivial argument with the name of the argument? @@ +366,5 @@ > + > + // We need to store a nsCOMPtr as well for those callbacks in order to call them > + // from addPathRunnableMethod and removePathRunnableMethod > + mChangeCallback = aOnChange; > + mErrorCallback = aOnError; I don't see why you need these. @@ +481,5 @@ > +NativeFileWatcher::Destroy() { > + > + // Make sure the instance was initialised > + if (!mIOThread) { > + return NS_ERROR_NOT_INITIALIZED; Actually, we can just return NS_OK if `!mIOThread`. @@ +527,5 @@ > + * Returns NS_ERROR_UNEXPECTED if OS |HANDLE|s are unexpectedly closed. > + * If the ReadDirectoryChangesW call fails, returns NS_ERROR_FAILURE, > + * otherwise NS_OK. > + */ > +nsresult NativeFileWatcher::addPathRunnableMethod(const nsAString& pathToWatch) { For defensive purposes, can you assert that we're not on the main thread (for each of the RunnableMethods)? @@ +532,5 @@ > + > + // Is pathToWatch already being watched? > + if (mWatchedResourcesByPath.Get(pathToWatch)) { > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mErrorCallbackHandle, NS_ERROR_FILE_ALREADY_EXISTS); Let's just ignore the path if it is already watched. @@ +571,5 @@ > + mozilla::ScopedDeletePtr<WatchedResourceDescriptor> > + resourceDesc(new WatchedResourceDescriptor); > + resourceDesc->resourceHandle = resHandle; > + resourceDesc->path = pathToWatch; > + memset(&resourceDesc->overlappedInfo, 0, sizeof(OVERLAPPED)); This should be done automatically by C++ with the call to `new`. @@ +647,5 @@ > + WatchedResourceDescriptor* toRemove = mWatchedResourcesByPath.Get(pathToRemove); > + if (!toRemove) { > + // We are trying to remove a path which wasn't being watched > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mErrorCallbackHandle, NS_ERROR_FILE_NOT_FOUND); Let's just ignore it. @@ +657,5 @@ > + } > + > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle > + toRemove->resourceHandle.dispose(); > + Why don't you remove the data from mWatchedResourceBy* here? @@ +673,5 @@ > + mWatchedResourcesByHandle.Clear(); > + > + // Clear frees the memory associated with each element and clears the table. > + // Since we are using Scoped |HANDLE|s, they get automatically closed as well. > + mWatchedResourcesByPath.Clear(); If both `mWatchedResourcesBy*` belong to the runnable, you won't need this anymore. ::: toolkit/components/filewatcher/NativeFileWatcherWin.h @@ +28,5 @@ > + NativeFileWatcher(); > + ~NativeFileWatcher(); > + > +private: > + size_t mNotificationBufferSize; Please document. @@ +43,5 @@ > + // The |HANDLE| to the I/O Completion Port, owned by the main thread. > + HANDLE mIOCompletionPort; > + nsCOMPtr<nsIThread> mIOThread; > + > + nsCOMPtr<nsINativeFileWatcherCallback> mChangeCallback; Since you already have the `nsMainThreadPtrHandle` instances, get rid of the matching `nsCOMPtr`. ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +16,5 @@ > +{ > + /** > + * @param error A message describing the error occurred. > + */ > + void complete(in nsresult error); Let's make it: void complete(in nsresult xpcomError, in int osError) @@ +29,5 @@ > +{ > + /** > + * @param resourcePath The path of the changed resource. > + */ > + void changed(in AString resourcePath); Could you add an `int32 flags` arg, for the future? In the current implementation, this will always be 0. ::: toolkit/components/filewatcher/tests/xpcshell/test_already_init.js @@ +19,5 @@ > + */ > + add_task(function* test_already_init() { > + // Instantiate the native watcher > + let watcher = Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] > + .createInstance(Ci.nsINativeFileWatcher); Looks like you'll need this snippet quite often. You might wish to add a function `makeWatcher()` to your head.js. @@ +26,5 @@ > + let onChangeFunc = function(changed) { } > + let onErrorFunc = function(error) { > + // Stop the test and write the error in the log > + do_throw('Unexpected error: ' + error); > + } In most of your tests, you will probably want to use the pattern I showed you: let deferred = Promise.defer(); watcher.init(deferred.resolve, deferred.reject); yield deferred; @@ +41,5 @@ > + } > + > + // Free the resources > + watcher.destroy(); > + watcher = null; That last instruction is not useful. ::: toolkit/components/filewatcher/tests/xpcshell/test_re_init.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +Components.utils.import("resource://gre/modules/Services.jsm"); Not used.

Implements some of the suggestions from Yoric (and splits the patch in 2 patches).

This is the patch containing the xpcshell-tests. I've implemented the changes you suggested.

My comments are within the quoted part. (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on May 16th-23th) from comment #10) > @@ +62,5 @@ > > + /** > > + * @param aOnError The passed error callback. > > + * @param aError The |nsresult| error value. > > + */ > > + WatchedErrorEvent(const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError, > > I believe that this shouldn't be a reference. Note that this will need a > second review from someone more familiar than me of `nsMainThreadPtr*`. As :jdm mentioned yesterday in #introduction, this should be the correct use of this class. > @@ +139,5 @@ > > + const nsMainThreadPtrHandle<nsINativeFileWatcherCallback>& aOnChange, > > + const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError) > > + : mIOCompletionPort(aIOCompletionPort) > > + , mWatchedResourcesByPath(aWatchedResourcesByPath) > > + , mWatchedResourcesByHandle(aWatchedResourcesByHandle) > > I suspect that we could make this runner own `mWatchedResourceBy*` and > [de]allocate them entirely off the main thread, too, as follows: > > 1. add a field `mInitialized` to `NativeFileWatcherIOTask`, initialized to > `false`, and owned entirely by the worker thread; > 2. move the current code of `Run()` into a method `Loop()`; > 3. create a new method `Init()` to > a. initialize the stuff above; > b. set `mInitialized` to `true`; > 4. when you start `Run()`, if `mInitialized` is `false`, execute method > `Init()` before executing method `Loop()`. > > This will be imperceptibly slower (one `if` off the main thread per loop > iteration), but this should also help keep the cleanup off the main thread. Should I move the add*RunnableMethod to NativeFileWatcherIOTask as well then? > @@ +162,5 @@ > > + DWORD transferredBytes = 0; > > + > > + // Will hold the |HANDLE| to the watched resource returned by GetQueuedCompletionStatus > > + // which generated the change events > > + ULONG_PTR changedResourceHandle = 0; > > Why a `ULONG_PTR` and not a `HANDLE` set to `INVALID_HANDLE_VALUE`? This is a good point indeed: is basically followed the examples from Microsoft (http://msdn.microsoft.com/en-us/library/ms706972%28v=vs.85%29.aspx). It appears that the size of ULONG_PTR varies based on the underlying machine architecture (http://msdn.microsoft.com/en-us/library/cc230394.aspx). But since HANDLE is just a void*, it should be the same. Probably switching to HANDLE would result in less casts in the following functions and thus cleaner code. > @@ +174,5 @@ > > + // Ok, there was some error > > + DWORD errCode = GetLastError(); > > + switch (errCode) { > > + case ERROR_ABANDONED_WAIT_0: > > + case ERROR_INVALID_HANDLE: { > > Mmmmhh... according to the documentation, under Windows XP, > ERROR_ABANDONED_WAIT_0 doesn't work nicely and will wait until timeout. We > should probably make sure that the timeout is not infinite at least on > Windows XP. Yes, but the documentation states "Closing the completion port handle *while a call is outstanding* [...]", which is not the case here. Since the closing function runs on the same worker thread, when we close the handle GetQueuedCompletionStatus is not being executed. It gets executed afterwards so we should be safe (but I guess that should be tested)! > Given the number of places in which we can have errors, I believe that you > should add logging. I suspect that this will save everybody's life some day. > See > http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog. > h?from=prlog.h . Should I add logging when every error occurs? (i.e. whenever I dispatch a WatcherErrorEvent?) > @@ +273,5 @@ > > + DWORD dwPlaceholder; > > + if (!ReadDirectoryChangesW(changedRes->resourceHandle, > > + changedRes->notificationBuffer.rwget(), > > + mNotificationBufferSize, > > + false, // watch subtree (recurse) > > I believe that we decided to recurse. We decided to settle for a non recursive behaviour since it was easier for the user to add the subfolders to the watch list than to deal with recursive notifications on all platforms in a simple way. But we can re-discuss this if you wish! > @@ +571,5 @@ > > + mozilla::ScopedDeletePtr<WatchedResourceDescriptor> > > + resourceDesc(new WatchedResourceDescriptor); > > + resourceDesc->resourceHandle = resHandle; > > + resourceDesc->path = pathToWatch; > > + memset(&resourceDesc->overlappedInfo, 0, sizeof(OVERLAPPED)); > > This should be done automatically by C++ with the call to `new`. Apparently it's not on my system when doing debug builds: is that normal? If I comment that line I get random values inside the overlappedInfo structure. Am I missing something? > @@ +657,5 @@ > > + } > > + > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle > > + toRemove->resourceHandle.dispose(); > > + > > Why don't you remove the data from mWatchedResourceBy* here? Because the loop will still be needing it (unfortunately) for at least one cycle (just documented in the code). > @@ +26,5 @@ > > + let onChangeFunc = function(changed) { } > > + let onErrorFunc = function(error) { > > + // Stop the test and write the error in the log > > + do_throw('Unexpected error: ' + error); > > + } > > In most of your tests, you will probably want to use the pattern I showed > you: > > let deferred = Promise.defer(); > watcher.init(deferred.resolve, deferred.reject); > > yield deferred; Sure, I've done it this way except in tests which do not need to wait on a promise (i.e. double initialisation test, etc.).

(In reply to Alessio Placitelli [:Dexter] from comment #13) > As :jdm mentioned yesterday in #introduction, this should be the correct use > of this class. Fine with me, then. > > This will be imperceptibly slower (one `if` off the main thread per loop > > iteration), but this should also help keep the cleanup off the main thread. > > Should I move the add*RunnableMethod to NativeFileWatcherIOTask as well then? Sounds like a good idea. > Yes, but the documentation states "Closing the completion port handle *while > a call is outstanding* [...]", which is not the case here. Since the closing > function runs on the same worker thread, when we close the handle > GetQueuedCompletionStatus is not being executed. It gets executed afterwards > so we should be safe (but I guess that should be tested)! Right. > > Given the number of places in which we can have errors, I believe that you > > should add logging. I suspect that this will save everybody's life some day. > > See > > http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog. > > h?from=prlog.h . > > Should I add logging when every error occurs? (i.e. whenever I dispatch a > WatcherErrorEvent?) Probably, yes. > > @@ +273,5 @@ > > I believe that we decided to recurse. > > We decided to settle for a non recursive behaviour since it was easier for > the user to add the subfolders to the watch list than to deal with recursive > notifications on all platforms in a simple way. But we can re-discuss this > if you wish! I'm almost sure that the MacOS X equivalent doesn't allow non-recursive, so it's probably easier if it's recursive. > > @@ +571,5 @@ > > > + mozilla::ScopedDeletePtr<WatchedResourceDescriptor> > > > + resourceDesc(new WatchedResourceDescriptor); > > > + resourceDesc->resourceHandle = resHandle; > > > + resourceDesc->path = pathToWatch; > > > + memset(&resourceDesc->overlappedInfo, 0, sizeof(OVERLAPPED)); > > > > This should be done automatically by C++ with the call to `new`. > > Apparently it's not on my system when doing debug builds: is that normal? If > I comment that line I get random values inside the overlappedInfo structure. > Am I missing something? No, you are right. No need to add the comment. > > @@ +657,5 @@ > > > + } > > > + > > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle > > > + toRemove->resourceHandle.dispose(); > > > + > > > > Why don't you remove the data from mWatchedResourceBy* here? > > Because the loop will still be needing it (unfortunately) for at least one > cycle (just documented in the code). Mmmh... Ok. I'll take a look at the doc :)

Comment on attachment 8423741 [details] [diff] [review] bug958280_tests.patch Review of attachment 8423741 [details] [diff] [review]: ----------------------------------------------------------------- Plenty of good things. ::: toolkit/components/filewatcher/tests/xpcshell/head.js @@ +5,5 @@ > +"use strict"; > + > +let {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Please add a second argument `this` to all your imports. This is necessary on B2G, so it's generally a good habit. Also, since you seem to always use OS.File, you don't need to go through XPCOMUtils to make it a lazy getter. @@ +13,5 @@ > + > +function makeWatcher() { > + return Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] > + .createInstance(Ci.nsINativeFileWatcher); > +}; I believe that most of your tests would be simplified by putting something along the lines of: function makeSimpleWatcher() { let watcher = makeWatcher(); let deferred = Promise.defer(); watcher.init(deferred.resolve, deferred.reject); return deferred.promise; } ::: toolkit/components/filewatcher/tests/xpcshell/test_already_init.js @@ +24,5 @@ > + // Define the callback functions > + let onChangeFunc = function(changed) { } > + let onErrorFunc = function(error) { > + // Stop the test and write the error in the log > + do_throw('Unexpected error: ' + error); You want to make sure that the test doesn't stop before we hit this line. So please use a Promise as I suggested even for these simple tests. @@ +40,5 @@ > + } > + > + // Free the resources > + watcher.destroy(); > + watcher = null; You never need this `watcher = null`. Also, let's perform the tests without calling `watcher.destroy()`, this will tell us more about the robustness of the code. ::: toolkit/components/filewatcher/tests/xpcshell/test_re_init.js @@ +24,5 @@ > + // Define the callback functions > + let onChangeFunc = function(changed) { } > + let onErrorFunc = function(error) { > + // Stop the test and write the error in the log > + do_throw('Unexpected error: ' + error); Here, too, use a Promise. ::: toolkit/components/filewatcher/tests/xpcshell/test_remove_non_watched.js @@ +29,5 @@ > + // When removing a resource which wasn't being watched, it should silently > + // ignore the request. > + do_throw("Unexpected exception: " > + + xpcomError + " (XPCOM) " > + + osError + " (OS Error)"); Here, too, use a Promise. ::: toolkit/components/filewatcher/tests/xpcshell/test_use_no_init.js @@ +25,5 @@ > + > + // This second call should trigger an error, which is propagated > + // as an exception to xpcshell-test. > + try { > + watcher.addPath(OS.Constants.Path.profileDir); Call `do_throw()` after `watcher.addPath` to fail if we have reached this line. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_already_watched.js @@ +28,5 @@ > + function(xpcomError, osError) { > + // We should silently ignore the new addPath, so throw if we get an error. > + do_throw("Unexpected exception: " > + + xpcomError + " (XPCOM) " > + + osError + " (OS Error)"); Here, too, use a Promise. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_file_creation_single.js @@ +26,5 @@ > + > + let deferred = Promise.defer(); > + > + // Initialise the watcher by passing the Promise's resolve and reject > + // respectively as the changed and error callbacks. This comment is not really useful. Same thing for other instances of that comment. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_file_modification_single.js @@ +42,5 @@ > + let fileDest = yield OS.File.open(tmpFilePath, {write: true}); > + try { > + let writeLength = yield fileDest.write(new Uint8Array(10)); > + } catch(ex) { > + do_throw("Unexpected error: " + error); No need to catch the error. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js @@ +41,5 @@ > + let deferred = Promise.defer(); > + > + // Initialise the watcher by passing the change and error callbacks. > + watcher.init( > + function(changed) { Could you add a `do_print` here? This will help us in case we need to debug a regression on this test. @@ +44,5 @@ > + watcher.init( > + function(changed) { > + detectedChanges += 1; > + > + // Close previous do_test_pending() call. This comment looks stale. @@ +49,5 @@ > + if (detectedChanges == resourcesToWatch) { > + deferred.resolve(detectedChanges); > + } > + }, > + deferred.reject That line doesn't do anything. @@ +65,5 @@ > + let array = new Uint8Array(50); > + for (let i = 0; i < resourcesToWatch; i++) { > + let tmpSubDirPath = OS.Path.join(watchedDir, tempDirNameBase + i); > + let tmpFilePath = OS.Path.join(tmpSubDirPath, tempFileName); > + let bytes = yield OS.File.writeAtomic(tmpFilePath, array); You can also pass a string to `writeAtomic`, instead of an array, if you wish.

In the crash stack that Terrence sent me ( https://pastebin.mozilla.org/5273291 ), there's this (where the dtor is being triggered from deferred finalization): [bunch of JS stuff] xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x097d2638, bool mayWait=true) Line 263 C++ xul.dll!nsThread::Shutdown() Line 559 C++ xul.dll!mozilla::NativeFileWatcher::Destroy() Line 712 C++ xul.dll!mozilla::NativeFileWatcher::~NativeFileWatcher() Line 536 C++ I'm not an expert on this, but it looks like maybe you have a nested event loop in there? That's always bad news. There's probably a NS_ProcessNextEvent way way up the stack.

nsThread::Shutdown must not be called at random times. You need to know spinning event loop is fine when it is called, so you probably want to post a nsRunnable which then calls thread->Shutdown()

Thanks for your help, Andrew and Olli! So calling the nsThread::Shutdown() from the destructor is not a good idea (in general)? (In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #19) > nsThread::Shutdown must not be called at random times. You need to know > spinning event loop is fine > when it is called, so you probably want to post a nsRunnable which then calls > thread->Shutdown()

This patch takes in account your last suggestions. I've not yet removed the .destroy() from the tests since, removing it, causes all the tests to fail due to the issue discussed with :smaug and :mccr8.

Good catch. Yes, you should call shutdown() from the runnable.

(In reply to Alessio Placitelli [:Dexter] from comment #20) > Thanks for your help, Andrew and Olli! > > So calling the nsThread::Shutdown() from the destructor is not a good idea > (in general)? Well, it depends on the dtor. If you know the dtor won't be called at "unsafe" times, calling Shutdown is fine, but if you can't guarantee that, then some other setup is needed.

Comment on attachment 8431095 [details] [diff] [review] bug958280_tests.patch Review of attachment 8431095 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, with minor nits. ::: toolkit/components/filewatcher/tests/xpcshell/head.js @@ +12,5 @@ > + return Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] > + .createInstance(Ci.nsINativeFileWatcher); > +}; > + > +function makeSimpleWatcher() { Not: some doc? ::: toolkit/components/filewatcher/tests/xpcshell/test_already_init.js @@ +20,5 @@ > + let watcher = makeWatcher(); > + > + // Define the callback functions. In this test none of the callbacks > + // is expected to be called. > + let onChangeFunc = function(changed) { } Since neither callback should be called, could you add a `do_throw` here, too? Same thing for the other tests in which neither callback should be called. ::: toolkit/components/filewatcher/tests/xpcshell/test_re_init.js @@ +14,5 @@ > + > +/** > + * Test for multiple initialisation > + */ > +add_task(function* test_already_init() { Minor nit: I would merge test_already_init.js and test_re_init.js in a single file test_init.js. Keep it in two different tasks, of course. ::: toolkit/components/filewatcher/tests/xpcshell/test_remove_non_watched.js @@ +14,5 @@ > + > +/** > + * Test removing non watched path > + */ > +add_task(function* test_remove_not_watched() { Here too, you might wish to merge a few files. Your call. @@ +23,5 @@ > + let watcher = makeWatcher(); > + > + // Initialise the watcher by passing the change and error callbacks. > + watcher.init( > + function(changed) { }, Generally, if you expect a callback to be called but don't want to it to do anything, add a comment `// do nothing`. If you expect it to be not called, add a `do_throw`. ::: toolkit/components/filewatcher/tests/xpcshell/test_use_no_init.js @@ +18,5 @@ > +add_task(function* test_usage_no_init() { > + // Instantiate the native watcher > + let watcher = makeWatcher(); > + > + // watcher.init(...) That comment doesn't look useful. @@ +20,5 @@ > + let watcher = makeWatcher(); > + > + // watcher.init(...) > + > + // This second call should trigger an error, which is propagated It's not a second call, btw. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_already_watched.js @@ +23,5 @@ > + let watcher = makeWatcher(); > + > + // Initialise the watcher by passing the change and error callbacks. > + watcher.init( > + function(changed) { }, Here, too, please put something in the callback. I'll stop mentioning that. @@ +39,5 @@ > + // We expect to silently ignore this second watching request > + watcher.addPath(watchedDir); > + > + // Remove the watch and free the associated memory. > + watcher.removePath(watchedDir); I would try without the `removePath`, just to encourage memory leaks (if any) to appear in the tests.

Adds a check for null callbacks and moves nsIThread::Shutdown in a nsIRunnable.

Implements the suggestions from your last feedback and fixes the tests.

Comment on attachment 8431729 [details] [diff] [review] bug958280_tests.patch Review of attachment 8431729 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with minor nits. I'm basically satisfied with marking this r+ with these changes, although I'll wait until we have the watcher itself nailed down, in case we realize we need to change or add tests. Also, in the future, could you add version numbers to your patches? This makes it easier to check inter-diff between tests. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_file_modification_single.js @@ +30,5 @@ > + > + // Create a file within the directory to be watched. We do this > + // before watching the folder so we do not get the creation notification. > + let tmpFilePath = OS.Path.join(watchedDir, tempFileName); > + let bytes = yield OS.File.writeAtomic(tmpFilePath, "some data"); You don't seem to use `bytes`, you can get rid of it. @@ +41,5 @@ > + try { > + let writeLength = yield fileDest.write(new Uint8Array(10)); > + } finally { > + yield fileDest.close(); > + } A simple call to OS.File.writeAtomic() should be sufficient. ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js @@ +65,5 @@ > + > + // Create a file within each watched directory. > + for (let i = 0; i < resourcesToWatch; i++) { > + let tmpSubDirPath = OS.Path.join(watchedDir, tempDirNameBase + i); > + let tmpFilePath = OS.Path.join(tmpSubDirPath, tempFileName); Just use `OS.Path.join(watchDir, tempDirNameBase + i, tempFileName)`. @@ +66,5 @@ > + // Create a file within each watched directory. > + for (let i = 0; i < resourcesToWatch; i++) { > + let tmpSubDirPath = OS.Path.join(watchedDir, tempDirNameBase + i); > + let tmpFilePath = OS.Path.join(tmpSubDirPath, tempFileName); > + let bytes = yield OS.File.writeAtomic(tmpFilePath, "test content"); You don't need `bytes`.

Thanks for the feedback. This patch polishes the tests based on your last suggestions!

This patch contains a NativeFIleWatcher implementation using the Cycle Collector. It doesn't work properly, as it doesn't get collected.

I believe I have it (thanks Nical for the brainstorming session). Breaking the cycle across threads: * FileWatcher holds a strong reference to onChange and onError; * NativeFileWatcherIOTask holds unsafe C++ pointers to onChange and onError, so as to not create a cycle. This should ensure that the cycle is purely on the main thread. With this, we end up with unsafe C++ pointers off the main thread, so we need a shutdown protocol that takes this into account. Shutdown Protocol: 1. [MT] The destructor of NativeFileWatcher calls Destroy() (which may also be called by the CC); 2. [MT] Destroy sends destroyRunnableMethod(onChange, onError) to the IOT, where onChange and onError are instances of nsMainThreadHandle<> this ensures that the callbacks are not gc-ed yet, as long as the thread could still call them unsafely; 2. [IOT] destroyRunnableMethod(onChange, onError) 1/ stops the loop; 2/ schedules for the next tick a call to Shutdown() when leaving the scope, onChange and onError will be released on the MT All of this needs, of course, to be perfectly documented.

Well, if there is no solution with the current design, here's a design that should work better. With this design, the JS consumer needs to explicitly call the method that kills the thread, but there is less ambiguity. I also have the impression that we can design a JS module on top of the current C++ code that will make sure to call Destroy() upon garbage-collection. If push comes to shove, we can do that.

This version implements the shutdown protocol and correctly gets cycle collected (except in the xpcshell-test, where we need to append forceGC() and forceCC() to the end of the test in order to trigger the collection).

Comment on attachment 8439388 [details] Plan 9 Good plan, but we shouldn't need this anymore.. right? :)

Comment on attachment 8439965 [details] [diff] [review] bug958280_v2_(Cycle Collector).patch Review of attachment 8439965 [details] [diff] [review]: ----------------------------------------------------------------- Many remarks, but this looks good. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +34,5 @@ > + } > + > + NS_METHOD Run() { > + MOZ_ASSERT(NS_IsMainThread()); > + mOnError->Complete(mError, mOsError); Since you ignore the result of the call, please cast it to `void`. @@ +39,5 @@ > + return NS_OK; > + } > + > + private: > + nsINativeFileWatcherErrorCallback* mOnError; Please add a comment here explaining why you need to use a raw pointer and refer to the comments on the Shutdown protocol to explain why this is safe. @@ +63,5 @@ > + } > + > + NS_METHOD Run() { > + MOZ_ASSERT(NS_IsMainThread()); > + mOnChange->Changed(mChangedResource, 0); Here, too, please cast the result to `void`. Also, please add a comment for the `0`. @@ +68,5 @@ > + return NS_OK; > + } > + > +private: > + nsINativeFileWatcherCallback* mOnChange; As above. @@ +84,5 @@ > + return gNativeWatcherPRLog; > +} > +#endif > + > +#define FILEWATCHERLOG(args) PR_LOG(GetFileWatcherContextLog(), PR_LOG_DEBUG, (args)) Thanks. Since our suites cannot really test this, have you checked manually that this works? @@ +86,5 @@ > +#endif > + > +#define FILEWATCHERLOG(args) PR_LOG(GetFileWatcherContextLog(), PR_LOG_DEBUG, (args)) > + > +// The number of notifications to store within WatchedResourceDescriptor:notificationBuffer. Please add a comment explaining what happens if the buffer is exceeded. @@ +100,5 @@ > + static void release(type anHandle) { > + if (anHandle != INVALID_HANDLE_VALUE) { > + // If CancelIo is called on an |HANDLE| not yet associated to a Completion I/O > + // it simply does nothing. > + CancelIo(anHandle); Here, too, can you cast the results to `void`? @@ +146,5 @@ > +{ > +public: > + NativeWatcherIOCleanupTask(nsIThread* workerThread) > + : mWorkerThread(workerThread) > + { Let's add assertions to make sure we're calling the ctor and Run() on the right threads. @@ +185,5 @@ > + /** > + * The watching thread logic. > + */ > + NS_IMETHOD Run() { > + MOZ_ASSERT(!NS_IsMainThread()); At this stage, we should return immediately if `mOnChange` and `mOnError` are null pointers (see below for the shutdown protocol). @@ +245,5 @@ > + return NS_DispatchToCurrentThread(this); > + } > + default: { > + // It should probably never get here, but it's better to be safe. > + FILEWATCHERLOG("NativeFileWatcherIOTask::Run - Unknown error."); Can you also log the error number? @@ +261,5 @@ > + } > + > + // When an |HANDLE| associated to the completion I/O port is gracefully > + // closed, GetQueuedCompletionStatus still may return a status update. Moreover, > + // this can also be triggered when watching files on a network folder and loosing Nit: "losing". @@ +330,5 @@ > + // is greater than 64 KB and the application is monitoring a directory over the network. > + // It could return ERROR_NOACCESS if the buffer is not aligned on a DWORD boundary. > + DWORD dwError = GetLastError(); > + > + FILEWATCHERLOG("NativeFileWatcherIOTask::Run - Call to ReadDirectoryChangesW failed."); Can you also log the error number and the name of the directory? @@ +340,5 @@ > + return rv; > + } > + } > + > + // Reschedule the runnable to run, again, on this thread when possible. "Reschedule the runnable to run on this thread, as soon as we have handled any pending messages." @@ +360,5 @@ > + */ > + nsresult addPathRunnableMethod(const nsAString& pathToWatch) { > + > + MOZ_ASSERT(!NS_IsMainThread()); > + Here too (and in the other runnable methods), we should exit immediately if `mOnError` or `mOnChange` is `nullptr`. @@ +384,5 @@ > + ? NS_ERROR_FILE_NOT_FOUND > + : NS_ERROR_FAILURE; > + > + FILEWATCHERLOG("NativeFileWatcherIOTask::addPathRunnableMethod - \ > + Call to CreateFileW failed."); Here, too, please add details to the log. @@ +411,5 @@ > + (ULONG_PTR)resourceDesc->resourceHandle.get(), 0)) { > + DWORD dwError = GetLastError(); > + > + FILEWATCHERLOG("NativeFileWatcherIOTask::addPathRunnableMethod - \ > + Call to CreateIoCompletionPort failed."); Etc. @@ +452,5 @@ > + // is greater than 64 KB and the application is monitoring a directory over the network. > + // It could return ERROR_NOACCESS if the buffer is not aligned on a DWORD boundary. > + nsRefPtr<WatchedErrorEvent> errorRunnable = > + new WatchedErrorEvent(mOnError, NS_ERROR_FAILURE, dwError); > + nsresult rv = NS_DispatchToMainThread(errorRunnable); Could you take the opportunity to factor all the calls that post an error runnable to the main thread into a single method `ReportError(nsstatus, int)`? @@ +507,5 @@ > + /** > + * Removes all the watched resources from the watch list and stops the > + * watcher thread. Frees all the used resources. > + */ > + nsresult destroyRunnableMethod(DestroyRunnableParametersWrapper* wrappedCallbacks) { I was thinking of something along the lines of the following (I took the opportunity to change the name of the method): nsresult deactivateRunnableMethod(DestroyRunnableParametersWrapper* aWrappedCallbacks) { // We pass `wrappedCallbacks` to ensure that we still hold strong references // towards the callbacks when we start executing `destroyRunnableMethod`. // // Once we have entered this method, we are certain that the callbacks will never be called, // so we can release the strong reference, ensuring that the callbacks and the NativeFileWatcher // will eventually be garbage-collected. ScopedDeletePtr<DestroyRunnableParametersWrapper> callbacks = aWrappedCallbacks; // Make sure that we never call the callbacks again. // Also, removing these references effectively deactivates // all the non-shutdown methods of this object. mOnError = nullptr; mOnChange = nullptr; // ... // Here, the rest of the cleanup you perform. // ... // At this stage, we may still have pending calls to Run() and runnable methods. By nulling // mOnError and mOnChange, we have made sure that these methods do not do anything and the // call to Destroy() has made sure that we cannot receive any further request. // // Now we just need to reschedule a final call to Shutdown() back to the main thread. return NS_DispatchToMainThread(new ShutdownTask(do_GetCurrentThread())); } @@ +578,5 @@ > + > +NativeFileWatcher::~NativeFileWatcher() { > + // Prevent a possible race condition when freeing mWatchedResourcesByPath and > + // mWatchedResourcesByHandle by making sure the worker thread is always killed. > + Destroy(); Can you assert that we succeed? @@ +746,5 @@ > + */ > +NS_IMETHODIMP > +NativeFileWatcher::Destroy() { > + > + // Make sure the instance was initialised. (and not de-initialized yet) @@ +749,5 @@ > + > + // Make sure the instance was initialised. > + if (!mIOThread) { > + return NS_OK; > + } I would add the following to be 100% sure that nobody can place calls to `mIOThread` once we have entered `Destroy()`, even if we exit due to an error. nsCOMPtr<nsIThread> ioThread = nullptr; ioThread.swap(mIOThread); Pas this point, `mIOThread` will be `nullptr` and use the local variable `ioThread` to access that thread. @@ +792,5 @@ > + // The thread will automatically exit from its loop when mIOCompletionPort is closed. > + // We dispatch the Thread::Shutdown() in a runnable to avoid spinning the event loop > + // from here (since this might be called from NativeFileWatcher's dtor in unsafe > + // locations). > + if (mIOThread) { Now that we dispatch this from the IO thread, let's get rid of the dispatch on the main thread. ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +74,5 @@ > + > + /** > + * Un-watches all the paths and releases the used resources. > + */ > + void destroy(); Can you rename this method to `uninit`?

Each test file contains a new task at the end which forces the garbage and cycle collection. It didn't work by using a tail file, so I've settled with this one.

I've added a MOZ_ASSERT in the dtor to make sure Uninit() succeeds and fixed the resource path not printing correctly in the NPSR log.

Comment on attachment 8442333 [details] [diff] [review] bug958280_v4.patch Review of attachment 8442333 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but please fix the errors below before requesting review from froydnj ::: toolkit/components/filewatcher/NativeFileWatcherNotSupported.h @@ +25,5 @@ > + NS_IMETHODIMP RemovePath(const nsAString& pathToRemove) { > + return NS_ERROR_NOT_IMPLEMENTED; > + }; > + > + NS_IMETHODIMP Destroy() { That should be Uninit() ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +538,5 @@ > + // > + // Once we have entered this method, we are certain that the callbacks will never > + // be called, so we can release the strong reference when exiting (through the passed > + // |nsAutoPtr|), ensuring that the callbacks and the NativeFileWatcher will eventually > + // be garbage-collected. You need to grab `aWrappedCallbacks` with the nsAutoPtr hereL nsAutoPtr<DestroyRunnableParametersWrapper> callbacks(aWrappedCallbacks); @@ +807,5 @@ > + */ > +NS_IMETHODIMP > +NativeFileWatcher::Uninit() { > + > + // Make sure the instance was initialised (and not de-initialized yet). Nit: pick a spelling for initialized/initialised and keep it :) @@ +840,5 @@ > + ioThread->Dispatch( > + NS_NewRunnableMethodWithArg<nsAutoPtr<DestroyRunnableParametersWrapper> >( > + static_cast<NativeFileWatcherIOTask*>(mWorkerIORunnable.get()), > + &NativeFileWatcherIOTask::deactivateRunnableMethod, > + nsAutoPtr<DestroyRunnableParametersWrapper>(wrappedCallbacks)), No, if you put the `nsAutoPtr` here, the cleanup logic will be called immediately, which is too early. Here, you need to pass a `DestroyRunnableParametersWrapper*`.

This patch takes care of the nits from Yoric in the previous patch!

Comment on attachment 8443489 [details] [diff] [review] bug958280_v5.patch Review of attachment 8443489 [details] [diff] [review]: ----------------------------------------------------------------- First things first: this is a well-written patch. The comments are good, particularly the explanations of how inter-thread stuff works. I have concerns about the API; I'll list them here, but I've also noted them below where applicable: - It looks like we're spawning a thread for each one of these nsINativeFileWatcher objects. That seems very heavyweight; I could easily imagine 10-20 different subsystems holding on to their own watcher, which means we would have 10-20 threads running, just for filesystem changes. We ought to be able to get by with one global thread that all the watchers dispatch to. - The callback API is very sparse. Consider the callback API in bug 759146, where we provide the callback with information about what exactly changed. (I realize that the 'flags' argument could provide this information, but such a "future enhancement" really seems like it ought to be a "present enhancement".) Particular comments about this implementation: - Some of the error-handling behavior for the I/O thread's main task is a little odd, insofar as we just stop watching for changes. - The Windows implementation in bug 759146 also appears to be somewhat simpler, though I can't say right away whether that's because of scope goals or something else. - I think it's a tad strange to do one concrete implementation without nailing down what the generic interface looks like. I see from skimming comments that it looks like the needs of the Mac implementation have been considered, but I don't see much else. Again, bug 759146 has trod this path before, it might be worth looking there for ideas. ::: toolkit/components/filewatcher/NativeFileWatcherNotSupported.h @@ +6,5 @@ > +#define mozilla_nativefilewatcher_h__ > + > +#include "nsINativeFileWatcher.h" > + > +class NativeFileWatcher MOZ_FINAL : public nsINativeFileWatcher { Style nit: for non-control structures (class definitions, enum definitions, function definitions, etc.), the opening brace belongs on the next line. There's quite a number of them in this patch; please fix them all. @@ +30,5 @@ > + return NS_ERROR_NOT_IMPLEMENTED; > + }; > + > + // Avoid accidental use of built-in operator= > + void operator=(const NativeFileWatcher& other) MOZ_DELETE; You should also include the copy constructor here. Both the copy constructor and operator= should be private. @@ +33,5 @@ > + // Avoid accidental use of built-in operator= > + void operator=(const NativeFileWatcher& other) MOZ_DELETE; > +}; > + > +NS_IMPL_ISUPPORTS(NativeFileWatcher, nsINativeFileWatcher); I think this should go in a separate file. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +41,5 @@ > + > + private: > + // |mOnError| needs to be a raw pointer to prevent cycles. This is safe as our > + // shutdown protocol accounts for that (see Uninit). > + nsINativeFileWatcherErrorCallback* mOnError; This comment seems inaccurate. It's not so much to prevent cycles as it is to make sure that you're not AddRef'ing/Release'ing on the wrong thread. It is probably better to use nsCOMPtr rather than raw pointers, pass around already_AddRefed<>, and use .forget() judiciously to make it clear that the lifetimes of these things are being managed appropriately. Serves as a good reminder for somebody who comes along later, too. @@ +74,5 @@ > + > +private: > + // |mOnChange| needs to be a raw pointer to prevent cycles. This is safe as our > + // shutdown protocol accounts for that (see Uninit). > + nsINativeFileWatcherCallback* mOnChange; Likewise for this. @@ +85,5 @@ > +#if defined(PR_LOGGING) > +static PRLogModuleInfo* GetFileWatcherContextLog() { > + static PRLogModuleInfo *gNativeWatcherPRLog; > + if (!gNativeWatcherPRLog) > + gNativeWatcherPRLog = PR_NewLogModule("NativeFileWatcher"); Nit: ifs are braced. @@ +113,5 @@ > + (void)CloseHandle(anHandle); > + } > + } > +}; > +typedef mozilla::Scoped<AutoCloseHandleTraits> AutoCloseHandle; No need for |mozilla::| prefixes inside |namespace mozilla {|. @@ +121,5 @@ > + * watched resource. > + */ > +struct WatchedResourceDescriptor { > + // The path on the file system of the watched resource. > + nsString path; 'm'-prefixes on all of these members, please. @@ +126,5 @@ > + > + // A buffer containing the latest notifications received for the resource. > + // ScopedDeleteArray<FILE_NOTIFY_INFORMATION> cannot be used as the structure > + // contains a variable length field (FileName). > + mozilla::ScopedDeleteArray<unsigned char> notificationBuffer; No |mozilla::|. @@ +185,5 @@ > +public: > + NativeFileWatcherIOTask(HANDLE aIOCompletionPort, > + nsINativeFileWatcherCallback* aOnChange, > + nsINativeFileWatcherErrorCallback* aOnError) > + : mNotificationBufferSize(WATCHED_RES_MAXIMUM_NOTIFICATIONS * sizeof(FILE_NOTIFY_INFORMATION)) Please make this a proper constant and remove mNotificationBufferSize. @@ +194,5 @@ > + > + /** > + * The watching thread logic. > + */ > + NS_IMETHOD Run() { Virtually all of the methods in this class should be defined out-of-line, please. @@ +231,5 @@ > + nsRefPtr<WatchedChangeEvent> changeRunnable = > + new WatchedChangeEvent(mOnChange, NS_LITERAL_STRING("*")); > + nsresult rv = NS_DispatchToMainThread(changeRunnable); > + if (FAILED(rv)) { > + return rv; At all the |return rv| points in this function, we are ceasing to watch changes to *all* files registered now and forevermore. That seems like a poor failure mode. @@ +241,5 @@ > + // If we reach this point, mIOCompletionPort was probably closed > + // and we need to close this thread. This condition is identified > + // by catching the ERROR_INVALID_HANDLE error. > + return NS_OK; > + } Nit: no braces needed for this case. @@ +268,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + return NS_OK; Arguably OK, but still a little weird, that we're no longer watching things. @@ +338,5 @@ > + | FILE_NOTIFY_CHANGE_FILE_NAME > + | FILE_NOTIFY_CHANGE_DIR_NAME, > + &dwPlaceholder, > + &changedRes->overlappedInfo, > + nullptr)) { This ReadDirectoryChangesW block could be profitably split out and reused for the similar ReadDirectoryChangesW block below. Many of these "call Windows API functions, perform a lot of error-handling" blocks could be split out into their own methods, which would make the main control flow much easier to understand. @@ +372,5 @@ > + * Returns NS_ERROR_UNEXPECTED if OS |HANDLE|s are unexpectedly closed. > + * If the ReadDirectoryChangesW call fails, returns NS_ERROR_FAILURE, > + * otherwise NS_OK. > + */ > + nsresult addPathRunnableMethod(const nsAString& pathToWatch) { Nit: AddPathRunnableMethod. And |aPathToWatch|. @@ +373,5 @@ > + * If the ReadDirectoryChangesW call fails, returns NS_ERROR_FAILURE, > + * otherwise NS_OK. > + */ > + nsresult addPathRunnableMethod(const nsAString& pathToWatch) { > + Nit: no blank line. @@ +413,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + // Error has already been reported through mErrorCallback. Just return NS_OK here. Nit: no need for "Just return NS_OK here." Here and other places. @@ +419,5 @@ > + } > + > + // Initialise the resource descriptor. > + mozilla::ScopedDeletePtr<WatchedResourceDescriptor> > + resourceDesc(new WatchedResourceDescriptor); No |mozilla::|. Which should free enough space to do: ScopedDeletePtr<WatchedResourceDescriptor> resourceDesc = new WatchedResourceDescriptor(...); @@ +423,5 @@ > + resourceDesc(new WatchedResourceDescriptor); > + resourceDesc->resourceHandle = resHandle; > + resourceDesc->path = pathToWatch; > + memset(&resourceDesc->overlappedInfo, 0, sizeof(OVERLAPPED)); > + resourceDesc->notificationBuffer = new unsigned char[mNotificationBufferSize]; This all should go in the constructor of WatchedResourceDescriptor. @@ +494,5 @@ > + * > + * @param pathToRemove > + * The path of the resource to remove from the watch list. > + */ > + nsresult removePathRunnableMethod(const nsAString& pathToRemove) { Nit: RemovePathRunnableMethod. And |aPathToRemove|. @@ +495,5 @@ > + * @param pathToRemove > + * The path of the resource to remove from the watch list. > + */ > + nsresult removePathRunnableMethod(const nsAString& pathToRemove) { > + Nit: no blank line. @@ +531,5 @@ > + /** > + * Removes all the watched resources from the watch list and stops the > + * watcher thread. Frees all the used resources. > + */ > + nsresult deactivateRunnableMethod(DestroyRunnableParametersWrapper* aWrappedCallbacks) { Nit: DeactivateRunnableMethod. @@ +603,5 @@ > + > + /** > + * Helper function to post an error runnable to the main thread. > + */ > + nsresult reportError(nsresult anError, DWORD anOSError) { Nit: ReportError. @@ +678,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + // We hold strong references to the callbacks and pass unsafe pointers to > + // NativeFileWatcherIOTask to make sure that cycles are just on the main thread. Again, it's not about cycles, it's about making sure you're not destroying things off the main thread. @@ +685,5 @@ > + > + // Start the IO worker thread. > + mWorkerIORunnable = > + new NativeFileWatcherIOTask(mIOCompletionPort, mChangeCallback, mErrorCallback); > + nsresult rv = NS_NewThread(getter_AddRefs(mIOThread), mWorkerIORunnable); Every watcher spins up its own thread? That seems...bad, especially since the API is designed for a watcher-per-things-I-am-interested-in, rather than something like: var fileWatchingService = Cc[...].getService(...); fileWatchingService.addWatchedPath(path, activityCallback, errorCallback); @@ +733,5 @@ > + // number of transferred bytes, the changed resource |HANDLE| and the address > + // of the |OVERLAPPED| structure passed to GetQueuedCompletionStatus: we set > + // them to nullptr so that we can recognize that we requested an interruption > + // from the Worker thread. > + PostQueuedCompletionStatus(mIOCompletionPort, 0, 0, nullptr); Don't repeat this block comment multiple times; make a separate helper method, put the comment there, and call that from the appropriate places. @@ +816,5 @@ > + } > + > + // We need to be sure that there will be no calls to 'mIOThread' once we have entered > + // 'Uninit()', even if we exit due to an error. > + nsCOMPtr<nsIThread> ioThread = nullptr; This nullptr initialization is unnecessary. @@ +832,5 @@ > + // Wrap the callbacks in order to pass them using NS_NewRunnableMethodWithArg. > + DestroyRunnableParametersWrapper* > + wrappedCallbacks = new DestroyRunnableParametersWrapper(); > + wrappedCallbacks->mChangeCallbackHandle = changeCallbackHandle; > + wrappedCallbacks->mErrorCallbackHandle = errorCallbackHandle; Make these part of the constructor, please. @@ +836,5 @@ > + wrappedCallbacks->mErrorCallbackHandle = errorCallbackHandle; > + > + // Since this function does a bit of I/O stuff (close file handle), run it > + // in the IO thread. Passing |nsAutoPtr| makes sure the wrapped callbacks > + // are released when deactivateRunnableMethod exits. There is no nsAutoPtr below? ::: toolkit/components/filewatcher/NativeFileWatcherWin.h @@ +13,5 @@ > +#include "nsCycleCollectionParticipant.h" > +#include "nsDataHashtable.h" > +#include "nsHashKeys.h" > +#include "nsProxyRelease.h" > +#include "nsThreadUtils.h" A lot of these headers look unnecessary and/or only needed in the implementation file. Please clean them up. @@ +27,5 @@ > + NS_DECL_NSINATIVEFILEWATCHER > + NS_DECL_CYCLE_COLLECTION_CLASS(NativeFileWatcher) > + > + NativeFileWatcher(); > + ~NativeFileWatcher(); I think you should make the destructor private (see recent threads on dev-platform). @@ +42,5 @@ > + nsCOMPtr<nsINativeFileWatcherCallback> mChangeCallback; > + nsCOMPtr<nsINativeFileWatcherErrorCallback> mErrorCallback; > + > + // Avoid accidental use of built-in operator=. > + void operator=(const NativeFileWatcher& other) MOZ_DELETE; Please declare the copy constructor here, and make it MOZ_DELETE. ::: toolkit/components/filewatcher/moz.build @@ +3,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/. > + > +XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell/xpcshell.ini'] Please add this with the patch that adds the tests, and not before. ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsISupports.idl" > + > +interface nsIVariant; This interface is unused. @@ +33,5 @@ > + * The path of the changed resource. If there were too many changes, > + * the string "*" is passed. > + * @param flags Reserved for future uses, not currently used. > + */ > + void changed(in AString resourcePath, in int32_t flags); This seems like an uninformative API. We have all the information to tell the user whether the file was created, deleted, modified, etc...we should be doing that. @@ +56,5 @@ > + in nsINativeFileWatcherErrorCallback onError); > + > + /** > + * Watches the passed path for changes. If it's a folder, every file > + * it contains is watched. Recursively watches subfolders. If the Nit: please use "directory" instead of "folder" in this comment. @@ +74,5 @@ > + > + /** > + * Un-watches all the paths and releases the used resources. > + */ > + void uninit(); Nit: maybe something like |clearAllPaths| or |clearAllWatches| is a better name.

Ah, I see that we could use this as a service, even though the tests don't reflect it as such. In that case, the separate thread is not a problem, since there will be only one service. The tests should be changed to reflect this, and I think the interface should be renamed to nsIFileWatcherService, perhaps with a corresponding change to the CID.

Nathan, thank you very much for your in depth review. You're right, it makes sense to turn this to a service. I'll start working on your suggestions and keep you and Yoric posted on this.

Ah, bug 759146 does not seem to point to a file watching thing (or similar). Did you mean some other bug?

(In reply to Alessio Placitelli [:Dexter] from comment #44) > Ah, bug 759146 does not seem to point to a file watching thing (or similar). > Did you mean some other bug? Whoops! Yes, I meant bug 759416. Sorry about the mixup.

Again, thank you for your time reviewing this! :) I've got a few comments (below). (In reply to Nathan Froyd (:froydnj) from comment #41) > - The callback API is very sparse. Consider the callback API in bug 759146, > where we provide the callback with information about what exactly changed. > (I realize that the 'flags' argument could provide this information, but > such a "future enhancement" really seems like it ought to be a "present > enhancement".) Yes, you're right, we were planning to add such information in a new release. But might me a good idea, as you suggest, to do it right now in order to stabilise the API/IDL. > Particular comments about this implementation: > > - Some of the error-handling behavior for the I/O thread's main task is a > little odd, insofar as we just stop watching for changes. Good catch! Many of the |return rv;| you see in the code are tied to |NS_DispatchToMainThread| failures, not to errors detected when watching the file system (i.e. OS API call errors). Watcher errors are just reported to the main thread by dispatching a |WatchedErrorEvent| through |ReportError|. I though that a failing call to |NS_DispatchToMainThread| was critical enough to abort watching: if that's not the case we can provide a more robust error handling! > - The Windows implementation in bug 759146 also appears to be somewhat > simpler, though I can't say right away whether that's because of scope goals > or something else. Thanks for pointing me to bug 759416! It makes some very good points (i.e. the double buffering). Yes, it is somewhat simpler, we're handling a couple more of edge cases (notification buffer overflow, failures in rescheduling a watch on Windows, etc.) and using Completion I/O Ports (IOCP). I see that bug 759416 is using asynchronous procedure calls (APC), which makes it clearer. I'm not sure if there's any performance difference in using IOCP or APC, it might be worthwhile investigating. > @@ +33,5 @@ > > + // Avoid accidental use of built-in operator= > > + void operator=(const NativeFileWatcher& other) MOZ_DELETE; > > +}; > > + > > +NS_IMPL_ISUPPORTS(NativeFileWatcher, nsINativeFileWatcher); > > I think this should go in a separate file. Yes, you're right, this should go in the cpp file implementing the nsINativeFileWatcher interface (please correct me if I'm wrong, as this is my first XPCOM component). NativeFileWatcherNotSupported.h has no corresponding implementation (cpp) file, it is a self-contained placeholder for unsupported platforms, so I thought this was its place. Should I split declaration/implementation for this class?

(In reply to Alessio Placitelli [:Dexter] from comment #43) > Nathan, thank you very much for your in depth review. > > You're right, it makes sense to turn this to a service. I'll start working > on your suggestions and keep you and Yoric posted on this. At the moment, callbacks are set in `Init()`. If we turn this into a service, we need to set them in `AddPath()` and change `RemovePath()`, which might require some tweaking to ensure that two distinct clients that watch the same patch won't mess with each other's callbacks. Alternatively, we can think of a JS layer on top of nsINativeFileWatcher to kind of turn this into a service.

Implements most of the changes suggested by frodyn (missing: nsCOMPtr in the Watcher*Callbacks, changing the name of unlink()).

This patch converts the file watcher to a service and slightly changes the IDL. The type of change detected is still not reported.

This patch changes the tests to work with a file watcher as a service.

Comment on attachment 8447158 [details] [diff] [review] bug958280_v7.patch Review of attachment 8447158 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, service looks good. However, despite what we both thought, I'm afraid that we still need something of a shutdown procedure. Also, could you add lots of LOGging? ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +101,5 @@ > + > +// The number of notifications to store within WatchedResourceDescriptor:mNotificationBuffer. > +// If the buffer overflows, its contents are discarded and a change callback is dispatched > +// with "*" as changed path. > +const unsigned int WATCHED_RES_MAXIMUM_NOTIFICATIONS = 25; Now that this is a service, I would increase it to 100. @@ +256,5 @@ > + // The same callback can be associated to multiple watches so we need to keep > + // them alive as long as there is a watch using them. We create two hashtables > + // to map directory names to lists of nsMainThreadPtr<callbacks>. > + typedef nsClassHashtable<nsStringHashKey, ChangeCallbackArray> ChangeCallbacksHashtable; > + typedef nsClassHashtable<nsStringHashKey, ErrorCallbackArray> ErrorCallbacksHashtable; Since you do not typedef the hashtable above, I believe that you shouldn't typedef these ones either. @@ +306,5 @@ > + > + // Will hold the |HANDLE| to the watched resource returned by GetQueuedCompletionStatus > + // which generated the change events. > + ULONG_PTR changedResourceHandle = 0; > + What's the behavior here if we call GetQueuedCompletionStatus before the first call to ReadDirectoryChangesW? @@ +333,5 @@ > + case ERROR_ABANDONED_WAIT_0: > + case ERROR_INVALID_HANDLE: > + // If we reach this point, mIOCompletionPort was probably closed > + // and we need to close this thread. This condition is identified > + // by catching the ERROR_INVALID_HANDLE error. Could you LOG here? In case we end up here by error, this will help find out what's going wrong. @@ +335,5 @@ > + // If we reach this point, mIOCompletionPort was probably closed > + // and we need to close this thread. This condition is identified > + // by catching the ERROR_INVALID_HANDLE error. > + return NS_OK; > + case ERROR_OPERATION_ABORTED: { Same here. @@ +450,5 @@ > + > + if (!wrappedParameters || > + !wrappedParameters->mChangeCallbackHandle || > + !wrappedParameters->mErrorCallbackHandle) { > + return NS_ERROR_NULL_POINTER; You should also LOG the error. @@ +548,5 @@ > + WatchedResourceDescriptor* resource = resourceDesc.forget(); > + mWatchedResourcesByPath.Put(wrappedParameters->mPath, resource); > + mWatchedResourcesByHandle.Put(resHandle, resource); > + > + // Append them to the hash tables as well. Why aren't you also calling AppendCallbacksToDescriptor? @@ +584,5 @@ > + if (!toRemove) { > + // We are trying to remove a path which wasn't being watched. Silently ignore. > + return NS_OK; > + } > + Could you check that both the change callback and the error callback are valid in `toRemove`? @@ +618,5 @@ > + // the worker thread when GetQueuedCompletionStatus detects that a > + // resource was removed from the watch list. > + // Since when closing |HANDLE|s relative to watched resources > + // GetQueuedCompletionStatus is notified one last time, it would end > + // up referring to deallocated memory if we would free the memory here. "if we were to free the memory here" @@ +624,5 @@ > + // again once we complete executing this function. > + > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > + // remove the entry from mWatchedResourceBy* since the completion port might > + // still be using the notification buffer. So when do you remove it, exactly? @@ +638,5 @@ > +nsresult > +NativeFileWatcherIOTask::DeactivateRunnableMethod() > +{ > + MOZ_ASSERT(!NS_IsMainThread()); > + I believe that you should cause an assertion failure if there is any resource still being watched by now. Also, to avoid race conditions, you should still do something comparable to the Shutdown protocol: 1/ Make it clear to other methods that shutdown is in progress; 2/ Stop the IO completion port wait; 3/ Schedule the rest of the deactivation for after every currently pending method call is complete. @@ +755,5 @@ > + * main thread fails as well, the error code is stored here. If the OS API call > + * does not fail, it gets set to NS_OK. > + * @return |true| if |ReadDirectoryChangesW| returned no error, |false| otherwise. > + */ > +bool Let's have it return a `nsresult`, just like the other methods. This will remove the need for `aDispatchErrorCode`. @@ +758,5 @@ > + */ > +bool > +NativeFileWatcherIOTask::WatchDirectoryForChanges( > + WatchedResourceDescriptor* aDirectoryDescriptor, > + nsresult& aDispatchErrorCode) Generally, we prefer pointers to references, for clarity at the call-site. @@ +765,5 @@ > + // Tells the OS to watch out on mResourceHandle for the changes specified > + // with the FILE_NOTIFY_* flags. We monitor the creation, renaming and > + // deletion of a file (FILE_NOTIFY_CHANGE_FILE_NAME), changes to the last > + // modification time (FILE_NOTIFY_CHANGE_LAST_WRITE) and the creation and > + // deletion of a folder (FILE_NOTIFY_CHANGE_DIR_NAME). Could you mention that this essentially initializes the data structure for the next call to GetQueuedCompletionStatus? Also, `WatchDirectoryForChanges` is perhaps not the best name. `AddDirectoryToWatchList`, maybe? @@ +822,5 @@ > + aDescriptor->mErrorCallbacks.IndexOf(aOnErrorHandle); > + > + // If the callbacks are not attached to the descriptor, append them. > + if (changeCallbackIndex == ChangeCallbackArray::NoIndex) { > + aDescriptor->mChangeCallbacks.AppendElement(aOnChangeHandle); I don't think this check is really useful. If the user really wants to add twice the same callback, let them do it. @@ +850,5 @@ > +{ > + // First check to see if we've got an entry already. > + ChangeCallbackArray* callbacksArray = mChangeCallbacksTable.Get(aPath); > + if (!callbacksArray) { > + // We don't have an entry. Create an array, add the callback and exit. Well, you're not exiting. @@ +860,5 @@ > + // already there. > + ChangeCallbackArray::index_type changeCallbackIndex = > + callbacksArray->IndexOf(aOnChangeHandle); > + > + // If the callback is not attached to the descriptor, append it. Again, I would let the user register the same callback several times, if they truly want to. @@ +911,5 @@ > + 0, // CompletionKey > + 2); // NumberOfConcurrentThreads > + if (!mIOCompletionPort) { > + return NS_ERROR_FAILURE; > + } To handle cleanup on error properly, you should keep the HANDLE as a ScopedPtr and move it to `mIOCompletionPort` only at the end of the method. @@ +972,5 @@ > + new nsMainThreadPtrHolder<nsINativeFileWatcherErrorCallback>(aOnError); > + > + // Wrap the path and the callbacks in order to pass them using NS_NewRunnableMethodWithArg. > + PathRunnablesParametersWrapper* wrappedCallbacks = new PathRunnablesParametersWrapper( > + aPathToWatch, changeCallbackHandle, errorCallbackHandle); Make it a ScopedDeletePtr (part 1 of 2). @@ +974,5 @@ > + // Wrap the path and the callbacks in order to pass them using NS_NewRunnableMethodWithArg. > + PathRunnablesParametersWrapper* wrappedCallbacks = new PathRunnablesParametersWrapper( > + aPathToWatch, changeCallbackHandle, errorCallbackHandle); > + > + // Since this function does a bit of I/O stuff (close file handle), run it It closes the file handle? I believe that there's an error in the comment, isn't there? @@ +988,5 @@ > + // Prevent wrappedCallbacks from leaking in case of errors. > + delete wrappedCallbacks; > + return rv; > + } > + (part 2 of 2) and call wrappedCallbacks.forget() once `Dispatch` has succeeded. Same thing for the next method. @@ +1094,5 @@ > + * This works by posting a bogus event to the blocking |GetQueuedCompletionStatus| > + * call in |NativeFileWatcherIOTask::Run()|. > + */ > +void > +NativeFileWatcherService::WakeWorkerThreadUp() I'd prefer `WakeUpWorkerThread()`. @@ +1105,5 @@ > +} > + > +/** > + * This method is used to catch the "xpcom-shutdown-threads" event in order > + * to shutdown this service when closing the application. Note: If my memory serves, I believe that xpcom-shutdown-threads this will wait until all threads are down. Which is good, because it will ensure that your shutdown mechanism has time to complete. ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +63,5 @@ > + in nsINativeFileWatcherErrorCallback onError); > + > + /** > + * Removes the provided path from the watched resources. If the path > + * was not being watched, silently ignores the request. Please document what happens if `onChange` or `onError` are not registered either (i.e. nothing).

As always, thanks for the time you spent reviewing this patch! > @@ +306,5 @@ > > + > > + // Will hold the |HANDLE| to the watched resource returned by GetQueuedCompletionStatus > > + // which generated the change events. > > + ULONG_PTR changedResourceHandle = 0; > > + > > What's the behavior here if we call GetQueuedCompletionStatus before the > first call to ReadDirectoryChangesW? GetQueuedCompletionStatus is always called before the first call to ReadDirectoryChangesW. This isn't a problem, since mIOCompletionPort is already a valid |HANDLE| even though it doesn't have any associated notification handles (through ReadDirectoryChangesW). > @@ +548,5 @@ > > + WatchedResourceDescriptor* resource = resourceDesc.forget(); > > + mWatchedResourcesByPath.Put(wrappedParameters->mPath, resource); > > + mWatchedResourcesByHandle.Put(resHandle, resource); > > + > > + // Append them to the hash tables as well. > > Why aren't you also calling AppendCallbacksToDescriptor? When creating a new WatchedResourceDescriptor, this is done automatically by WatchedResourceDescriptor's ctor. > @@ +584,5 @@ > > + if (!toRemove) { > > + // We are trying to remove a path which wasn't being watched. Silently ignore. > > + return NS_OK; > > + } > > + > > Could you check that both the change callback and the error callback are > valid in `toRemove`? Do you mean the callback arrays? > @@ +624,5 @@ > > + // again once we complete executing this function. > > + > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > > + // remove the entry from mWatchedResourceBy* since the completion port might > > + // still be using the notification buffer. > > So when do you remove it, exactly? When handling ERROR_OPERATION_ABORTED in NativeFileWatcherIOTask::Run. > @@ +638,5 @@ > > +nsresult > > +NativeFileWatcherIOTask::DeactivateRunnableMethod() > > +{ > > + MOZ_ASSERT(!NS_IsMainThread()); > > + > > I believe that you should cause an assertion failure if there is any > resource still being watched by now. Even though the watches are released below this line? Should this be done in order to remind users to manually remove the watches before quitting? > @@ +755,5 @@ > > + * main thread fails as well, the error code is stored here. If the OS API call > > + * does not fail, it gets set to NS_OK. > > + * @return |true| if |ReadDirectoryChangesW| returned no error, |false| otherwise. > > + */ > > +bool > > Let's have it return a `nsresult`, just like the other methods. This will > remove the need for `aDispatchErrorCode`. It was like that in my first write-up, but then I noticed that I had to distinguish the following cases: 1. ReadDirectoryChangesW fails (returns false), dispatching the error callbacks fails (aDispatchErrorCode is the related error code) 2. ReadDirectoryChangesW fails (returns false), dispatching the error callbacks succeeds (aDispatchErrorCode is NS_OK) 3. ReadDirectoryChangesW succeeds (returns true), aDispatchErrorCode is NS_OK. I thought the bool return value was needed unless another nsresult success code was used in case 3. > @@ +822,5 @@ > > + aDescriptor->mErrorCallbacks.IndexOf(aOnErrorHandle); > > + > > + // If the callbacks are not attached to the descriptor, append them. > > + if (changeCallbackIndex == ChangeCallbackArray::NoIndex) { > > + aDescriptor->mChangeCallbacks.AppendElement(aOnChangeHandle); > > I don't think this check is really useful. If the user really wants to add > twice the same callback, let them do it. Oh, I thought that was something undesirable. Out of curiosity, is there any particular use case you were thinking of?

Comment on attachment 8447159 [details] [diff] [review] bug958280_tests_v4.patch Review of attachment 8447159 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Is there a chance you could add a test that fills the buffer by very quickly changing hundreds of files? ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_file_creation_single.js @@ +39,5 @@ > + // Wait until the watcher informs us that the file was created. > + let changed = yield deferred.promise; > + do_check_eq(changed, tmpFilePath); > + > + // Remove the watch and free the associated memory. // (we need to reuse `deferred.resolve` and `deferred.reject` to unregister). ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js @@ +38,5 @@ > + // resources. > + let detectedChanges = 0; > + > + let deferred = Promise.defer(); > + Nit: Whitespace. @@ +39,5 @@ > + let detectedChanges = 0; > + > + let deferred = Promise.defer(); > + > + // Define the change callback function. Nit: Whitespace.

(In reply to Alessio Placitelli [:Dexter] from comment #52) > As always, thanks for the time you spent reviewing this patch! Thanks for writing it in the first place. > > What's the behavior here if we call GetQueuedCompletionStatus before the > > first call to ReadDirectoryChangesW? > > GetQueuedCompletionStatus is always called before the first call to > ReadDirectoryChangesW. This isn't a problem, since mIOCompletionPort is > already a valid |HANDLE| even though it doesn't have any associated > notification handles (through ReadDirectoryChangesW). Can you document this in the code? > > Why aren't you also calling AppendCallbacksToDescriptor? > > When creating a new WatchedResourceDescriptor, this is done automatically by > WatchedResourceDescriptor's ctor. That looks a bit confusing to me. Since we cannot always auto-add, we might want to never auto-add. This will make the code a bit more regular, I believe. > > Could you check that both the change callback and the error callback are > > valid in `toRemove`? > > Do you mean the callback arrays? Yes. > > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > > > + // remove the entry from mWatchedResourceBy* since the completion port might > > > + // still be using the notification buffer. > > > > So when do you remove it, exactly? > > When handling ERROR_OPERATION_ABORTED in NativeFileWatcherIOTask::Run. Can you document this? > > I believe that you should cause an assertion failure if there is any > > resource still being watched by now. > > Even though the watches are released below this line? Should this be done in > order to remind users to manually remove the watches before quitting? Exactly. > > Let's have it return a `nsresult`, just like the other methods. This will > > remove the need for `aDispatchErrorCode`. > > It was like that in my first write-up, but then I noticed that I had to > distinguish the following cases: > > 1. ReadDirectoryChangesW fails (returns false), dispatching the error > callbacks fails (aDispatchErrorCode is the related error code) > 2. ReadDirectoryChangesW fails (returns false), dispatching the error > callbacks succeeds (aDispatchErrorCode is NS_OK) > 3. ReadDirectoryChangesW succeeds (returns true), aDispatchErrorCode is > NS_OK. > > I thought the bool return value was needed unless another nsresult success > code was used in case 3. I believe that you can safely merge everything in a single nsresult. > > I don't think this check is really useful. If the user really wants to add > > twice the same callback, let them do it. > > Oh, I thought that was something undesirable. Out of curiosity, is there any > particular use case you were thinking of? Not really. I believe it's a dumb thing to do as a user of the API. However, if they register twice, I believe they will expect to be warned twice. And this simplifies a little the code.

This patch implements the suggestions from Yoric's previous review.

Comment on attachment 8448924 [details] [diff] [review] bug958280_v8.patch Review of attachment 8448924 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with a few minor changes Next review is for froydnj :) ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +189,5 @@ > + > +/** > + * This runnable is dispatched to the main thread in order to safely > + * shutdown the worker thread. Please note that this can be dispatched > + * from the main thread to the main thread. Really? That doesn't seem to be the case in the code. @@ +286,5 @@ > + */ > +NS_IMETHODIMP > +NativeFileWatcherIOTask::Run() > +{ > + MOZ_ASSERT(!NS_IsMainThread()); Here, too, you should bailout immediately if `mShutdown` is `true`. @@ +406,5 @@ > + + resourceName); > + > + nsresult rv = DispatchChangeCallbacks(changedRes, resourcePath); > + if (FAILED(rv)) { > + return rv; So this error is apparently fatal (you don't call `NS_DispatchToCurrentThread`). In that case, you should LOG that you bailout. @@ +421,5 @@ > + nsresult rv = AddDirectoryToWatchList(changedRes); > + if (NS_FAILED(rv)) { > + // We failed to watch the folder. But did we fail to dispatch the error callbacks > + // as well? If that's the case, abort. > + if (rv == NS_ERROR_ABORT) { Here, too, LOG that you bailout. @@ +497,5 @@ > + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, > + nullptr); // Template file (only used when creating files) > + if (resHandle == INVALID_HANDLE_VALUE) { > + DWORD dwError = GetLastError(); > + nsresult rv = (dwError == ERROR_FILE_NOT_FOUND) Could you: - add the case in which we do not have the necessary rights for opening the file and return `NS_ERROR_FILE_ACCESS_DENIED`; - add the case in which the destination is not a directory and return `NS_ERROR_FILE_DESTINATION_NOT_DIR`? @@ +514,5 @@ > + // Error has already been reported through mErrorCallback. > + return NS_OK; > + } > + > + // Initialise the resource descriptor (this also adds the callbacks to the descriptor). Does it? Is this comment still valid? @@ +552,5 @@ > + if (NS_FAILED(rv)) { > + // We failed to watch the folder. But did we fail to dispatch the error > + // callbacks as well? > + if (rv == NS_ERROR_ABORT) { > + // If that's the case, abort. log, log, log! @@ +580,5 @@ > + * Removes the path from the list of watched resources. Silently ignores the request > + * if the path was not being watched. > + * > + * @param pathToRemove > + * The path of the resource to remove from the watch list. Documentation is obsolete. @@ +586,5 @@ > +nsresult > +NativeFileWatcherIOTask::RemovePathRunnableMethod( > + PathRunnablesParametersWrapper* aWrappedParameters) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); Nit: I believe it would be useful if you documented the Remove Protocol just as you document the Shutdown Protocol. @@ +614,5 @@ > + MOZ_ASSERT(toRemove->mChangeCallbacks.Length()); > + MOZ_ASSERT(toRemove->mErrorCallbacks.Length()); > + > + // Remove the change callback from the descriptor and the hashtable. > + toRemove->mChangeCallbacks.RemoveElement(wrappedParameters->mChangeCallbackHandle); We should check whether `RemoveElement` actually removed an element. If not, we should crash (in DEBUG mode) or log (in non-DEBUG mode). @@ +622,5 @@ > + > + // This should always be valid. > + MOZ_ASSERT(changeCallbackArray); > + > + changeCallbackArray->RemoveElement(wrappedParameters->mChangeCallbackHandle); Same here. @@ +625,5 @@ > + > + changeCallbackArray->RemoveElement(wrappedParameters->mChangeCallbackHandle); > + > + // Remove the error callback as well. > + toRemove->mErrorCallbacks.RemoveElement(wrappedParameters->mErrorCallbackHandle); Same here. @@ +632,5 @@ > + mErrorCallbacksTable.Get(toRemove->mPath); > + > + MOZ_ASSERT(errorCallbackArray); > + > + errorCallbackArray->RemoveElement(wrappedParameters->mErrorCallbackHandle); Same here. @@ +673,5 @@ > + MOZ_ASSERT(!mWatchedResourcesByHandle.Count()); > + > + // We return immediately if |mShuttingDown| is true (see below for the > + // shutdown protocol). > + if (mShuttingDown) { If this happens, we are in a weird situation. I believe that we should crash / log. @@ +799,5 @@ > + */ > +nsresult > +NativeFileWatcherIOTask::AddDirectoryToWatchList( > + WatchedResourceDescriptor* aDirectoryDescriptor) > +{ Do you need to bailout in case of Shutdown?

(In reply to David Rajchenbach Teller [:Yoric] from comment #56) > r=me, with a few minor changes > Next review is for froydnj :) Awesome :-) > @@ +497,5 @@ > > + FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, > > + nullptr); // Template file (only used when creating files) > > + if (resHandle == INVALID_HANDLE_VALUE) { > > + DWORD dwError = GetLastError(); > > + nsresult rv = (dwError == ERROR_FILE_NOT_FOUND) > > Could you: > - add the case in which the destination is not a directory and return > `NS_ERROR_FILE_DESTINATION_NOT_DIR`? Unfortunately, CreateFileW doesn't fail if you're trying to open a file instead of a directory (even though it should given the parameters, IMHO). We could either add an additional check here to be 100% safe it's a directory (by querying for |HANDLE| information) or just let it go (it will fail and report the error when attempting ReadDirectoryChangesW). > @@ +799,5 @@ > > + */ > > +nsresult > > +NativeFileWatcherIOTask::AddDirectoryToWatchList( > > + WatchedResourceDescriptor* aDirectoryDescriptor) > > +{ > > Do you need to bailout in case of Shutdown? I'm not entirely convinced we should, since AddDirectoryToWatchList is called in ::Run() and we've already added a check there. Should I add it here as well, as a precaution?

This patch adds a test to stress the file watcher with hundreds of modifications within a single watched directory.

This patch implement the suggestions from Yoric: it's still missing the type of change detected, but I'm planning to implement this within this week.

This patch deals with the suggestions coming from both :Yoric and :froydnj (or at least most of them).

Comment on attachment 8452419 [details] [diff] [review] bug958280_v10.patch Review of attachment 8452419 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks good. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +20,5 @@ > + > +/** > + * An event used to notify the main thread when an error happens. > + */ > +class WatchedErrorEvent MOZ_FINAL : public nsRunnable Put this in the anonymous namespace below? @@ +129,5 @@ > +typedef Scoped<AutoCloseHandleTraits> AutoCloseHandle; > + > +// Define these callback array types to make the code easier to read. > +typedef nsTArray<nsMainThreadPtrHandle<nsINativeFileWatcherCallback> > ChangeCallbackArray; > +typedef nsTArray<nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback> > ErrorCallbackArray; Nit: we can use >> in templates nowadays. @@ +326,5 @@ > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > + > + nsresult rv = DispatchChangeCallbacks(changedRes, NS_LITERAL_STRING("*")); > + if (FAILED(rv)) { > + return rv; I still think it's incorrect to cease watching files for errors like this. @@ +479,5 @@ > + > + // Is aPathToWatch already being watched? > + WatchedResourceDescriptor* watchedPath = > + mWatchedResourcesByPath.Get(wrappedParameters->mPath); > + if (watchedPath) { Nit: maybe |watchedResource| instead of |watchedPath|? @@ +568,5 @@ > + // AddDirectoryToWatchList could use the error callback. > + AppendCallbacksToDescriptor( > + resourceDesc, > + wrappedParameters->mChangeCallbackHandle, > + wrappedParameters->mErrorCallbackHandle); Is there a reason we're not adding things to the hashtable as well? (Which then suggests we should have something that adds to the watch descriptor and the hash tables simultaneously.) It seems odd to have things in this in-between watched state; I think having things added everywhere or nowhere would be easier to reason about. Is there potential for a leak here? Would the user expect that an error requires them to remove the watchers for a given file? @@ +653,5 @@ > + MOZ_ASSERT(toRemove->mChangeCallbacks.Length()); > + MOZ_ASSERT(toRemove->mErrorCallbacks.Length()); > + > + // Remove the change callback from the descriptor and the hashtable. > + bool bRemoved = Just |removed|, please. @@ +659,5 @@ > + if (!bRemoved) { > + MOZ_ASSERT(false); > + FILEWATCHERLOG( > + "NativeFileWatcherIOTask::RemovePathRunnableMethod - Unable to remove the change " > + "callback from the callback list for resource %S.", wrappedParameters->mPath); It is a little weird to log things after you have failed an assert. :) At a minimum, I think interchanging these calls would be an improvement. (And other calls like them.) Probably not worth asserting on failure here and similar places. @@ +720,5 @@ > + > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > + // remove the entry from mWatchedResourceBy* since the completion port might > + // still be using the notification buffer. Entry remove is performed when > + // handling ERROR_OPERATION_ABORTED in NativeFileWatcherIOTask::Run. Are we *guaranteed* that's going to happen? Or should we be dispatching a separate runnable to remove things from |mWatchedResourceBy| and just ignoring the ERROR_OPERATION_ABORTED? For instance, can we get too many notifications such that ERROR_OPERATION_ABORTED never gets sent and we essentially leak memory? @@ +795,5 @@ > + > + for (size_t i = 0; i < aResourceDescriptor->mChangeCallbacks.Length(); i++) { > + nsRefPtr<WatchedChangeEvent> changeRunnable = new WatchedChangeEvent( > + (aResourceDescriptor->mChangeCallbacks)[i], aChangedResource); > + nsresult rv = NS_DispatchToMainThread(changeRunnable); Might be good to have a ReportChange function that encapsulated these two lines, for symmetry with the error case. @@ +1077,5 @@ > + > + // Be sure valid arguments were passed. > + if (!aOnChange || !aOnError) { > + return NS_ERROR_NULL_POINTER; > + } This is unusual API; we don't require error callbacks in other cases (Promise.prototype.then was the one that sprung to mind). ::: toolkit/components/filewatcher/moz.build @@ +17,5 @@ > +XPIDL_SOURCES += [ > + 'nsINativeFileWatcher.idl', > +] > + > +FINAL_LIBRARY = 'toolkitcomps' \ No newline at end of file Please add FAIL_ON_WARNINGS = True to the moz.build and fixup any warnings that occur on Try.

Comment on attachment 8452415 [details] [diff] [review] bug958280_tests_v6.patch Review of attachment 8452415 [details] [diff] [review]: ----------------------------------------------------------------- Very complete tests! ::: toolkit/components/filewatcher/tests/xpcshell/head.js @@ +11,5 @@ > +function makeWatcher() { > + let watcher = Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'].getService(); > + watcher.QueryInterface(Ci.nsINativeFileWatcherService); > + return watcher; > +}; This probably wants to be just: var gWatcherService = Cc['...'].getService(Ci.nsINativeFileWatcherService); ::: toolkit/components/filewatcher/tests/xpcshell/test_watch_many_changes.js @@ +21,5 @@ > + // Create and watch a sub-folder of the profile directory so we don't > + // catch notifications we're not interested in (i.e. "startupCache"). > + let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); > + yield OS.File.makeDir(watchedDir); > + Please remove trailing whitespace wherever appropriate.

Thanks for your time :Froydnj! I've fixed the tests based on your suggestions! We should thank :Yoric for his valuable suggestions for the tests ;) (In reply to Nathan Froyd (:froydnj) from comment #63) > Comment on attachment 8452415 [details] [diff] [review] > bug958280_tests_v6.patch > > Review of attachment 8452415 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very complete tests!

(In reply to Alessio Placitelli [:Dexter] from comment #65) > Thanks for your time :Froydnj! I've fixed the tests based on your > suggestions! We should thank :Yoric for his valuable suggestions for the > tests ;) You're welcome! Yoric indeed deserves credit for the tests as well. I'll note it in my bugzilla status, but just in case you don't see it: I'll be unavailable for reviews for the next two weeks. So if you r? me and wonder why nothing's happening...that's why. :)

Thanks for reviewing this as well! I've got a few things I'd like to discuss (see below). (In reply to Nathan Froyd [AFK through July 21st] (:froydnj) from comment #62) > Comment on attachment 8452419 [details] [diff] [review] > bug958280_v10.patch > > Review of attachment 8452419 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +326,5 @@ > > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > > + > > + nsresult rv = DispatchChangeCallbacks(changedRes, NS_LITERAL_STRING("*")); > > + if (FAILED(rv)) { > > + return rv; > > I still think it's incorrect to cease watching files for errors like this. By digging through the documentation (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIThread and, more specifically, https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIEventTarget#dispatch%28%29) and DXR, I found that |NS_DispatchToMainThread| can basically fail for three reasons: 1) It fails to get the main thread through |NS_GetMainThread| (see http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp#164 ). 2) NS_ERROR_UNEXPECTED - The call to |Dispatch| fails because the thread is shutting down and has finished processing events, so this event would never run and has not been dispatched. 3) NS_ERROR_INVALID_ARG - Indicates that the passed event is null. If any of the above occur, I feel we could safely stop watching because we would have some problems communicating with the main thread. Do you feel I need to change something to make it clearer or do you still think we should be more tolerant? > @@ +568,5 @@ > > + // AddDirectoryToWatchList could use the error callback. > > + AppendCallbacksToDescriptor( > > + resourceDesc, > > + wrappedParameters->mChangeCallbackHandle, > > + wrappedParameters->mErrorCallbackHandle); > > Is there a reason we're not adding things to the hashtable as well? (Which > then suggests we should have something that adds to the watch descriptor and > the hash tables simultaneously.) > > It seems odd to have things in this in-between watched state; I think having > things added everywhere or nowhere would be easier to reason about. > Is there potential for a leak here? Would the user expect that an error > requires them to remove the watchers for a given file? Both you and Yoric pointed me to this bit of code: there's certainly room for improvement. I'll verify if we can keep the callbacks just in the hashtables and generally refactor this part. > @@ +659,5 @@ > > + if (!bRemoved) { > > + MOZ_ASSERT(false); > > + FILEWATCHERLOG( > > + "NativeFileWatcherIOTask::RemovePathRunnableMethod - Unable to remove the change " > > + "callback from the callback list for resource %S.", wrappedParameters->mPath); > > It is a little weird to log things after you have failed an assert. :) > > At a minimum, I think interchanging these calls would be an improvement. > (And other calls like them.) > > Probably not worth asserting on failure here and similar places. Good catch, I've swapped the assertion and the log! IMHO, should keep them both: MOZ_ASSERT should just work in debug builds while logging makes it easier to trace errors in release builds as well. What do you think? > @@ +720,5 @@ > > + > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > > + // remove the entry from mWatchedResourceBy* since the completion port might > > + // still be using the notification buffer. Entry remove is performed when > > + // handling ERROR_OPERATION_ABORTED in NativeFileWatcherIOTask::Run. > > Are we *guaranteed* that's going to happen? Or should we be dispatching a > separate runnable to remove things from |mWatchedResourceBy| and just > ignoring the ERROR_OPERATION_ABORTED? For instance, can we get too many > notifications such that ERROR_OPERATION_ABORTED never gets sent and we > essentially leak memory? I don't know if that's *guaranteed*, as MSDN doesn't mention it (but not even the contrary), but I'm not quite sure that dispatching a runnable would help. Basically, if we close an handle through |toRemove->mResourceHandle.dispose()|, when the worker thread gets woken up again it uses the notification buffer at least once before throwing that error. If we dispatch a runnable to do the cleanup, we would need to be sure the runnable gets called AFTER the worker thread wakes up again. Something like that: (1) Close the handle (2) Enqueue the resource cleanup runnable (3) Wake Up the worker thread [... some ops interrupt the worker thread again ] (4) The cleanup runnable gets called Moreover, if we keep it as it is right now, in the case ERROR_OPERATION_ABORTED doesn't get fired, memory is eventually cleaned up in "uninit()". In the end, I think that dispatching a new runnable to cleanup each watch adds complexity and potentially new problems. > @@ +1077,5 @@ > > + > > + // Be sure valid arguments were passed. > > + if (!aOnChange || !aOnError) { > > + return NS_ERROR_NULL_POINTER; > > + } > > This is unusual API; we don't require error callbacks in other cases > (Promise.prototype.then was the one that sprung to mind). Great point! I'll be making the error callback optional here as well.

I've added a few more tests: no error callback passed and sharing the same callback among multiple watches.

This patch makes the error callback not mandatory and implements the other suggestions by froydnj. Most importantly, it refactors the AddCallbacksTo* and moves the callbacks out of the watcher descriptors into the hash tables. This should make things a bit clearer.

Comment on attachment 8457429 [details] [diff] [review] bug958280_tests_v8.patch Review of attachment 8457429 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry, I won't have time to look at the full patch for a few days. Can you quickly summarize what changed?

Comment on attachment 8457440 [details] [diff] [review] bug958280_v11.patch Review of attachment 8457440 [details] [diff] [review]: ----------------------------------------------------------------- First part of the review. Sorry about the delay, I'm multitasking a lot. I will try and finish the review tomorrow. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +287,5 @@ > +NativeFileWatcherIOTask::Run() > +{ > + MOZ_ASSERT(!NS_IsMainThread()); > + > + // We return immediately if |mShuttingDown| is true (see below for the Nit: see where exactly? @@ +351,5 @@ > + WatchedResourceDescriptor* toFree = > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > + > + if (toFree) { > + // Take care of removing the resource and freeing the memory Nit: Could you add one more newline? Otherwise, it looks like a comment on the next line, rather than a comment on the entire block. ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +43,5 @@ > +{ > + /** > + * Initialises the watcher service. > + */ > + void init(); I suspect that we can now get rid of `init()`. @@ +73,5 @@ > + * The registered callback invoked whenever a change on a watched > + * resource is detected. > + * @param onError > + * The optionally registered callback invoked whenever an error > + * occurs. So, what happens if we register `onChange` and `onError` but unregister only `onChange`?

Thank you for your review Yoric! > I suspect that we can now get rid of `init()`. Well, yes and no. I've used the NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(NativeFileWatcherService, Init) macro in nsToolkitCompsModule.cpp to let it automatically initialise. Removing the Init() would mean I should take care of initialising the file watcher on the first use. Do you think that's worth the change? > So, what happens if we register `onChange` and `onError` but unregister only `onChange`? The file watcher just considers the onChange callbacks when deciding to close a watch on a resource. If there are no more onChange callbacks associated to the watch, it gets closed (even though it still has some error callbacks associated). Do not worry: the error callbacks get automatically freed anyway.

(In reply to Alessio Placitelli [:Dexter] from comment #67) > > @@ +326,5 @@ > > > + mWatchedResourcesByHandle.Get((HANDLE)changedResourceHandle); > > > + > > > + nsresult rv = DispatchChangeCallbacks(changedRes, NS_LITERAL_STRING("*")); > > > + if (FAILED(rv)) { > > > + return rv; > > > > I still think it's incorrect to cease watching files for errors like this. > If any of the above occur, I feel we could safely stop watching because we > would have some problems communicating with the main thread. > > Do you feel I need to change something to make it clearer or do you still > think we should be more tolerant? I think that, at root, the whole thing is a readability issue. The way things are structured now, the reader of the code knows that we should always dispatch this runnable back to the event loop. But then we encounter this odd process of sometimes dispatching back the event loop in the middle, sometimes bailing out, and sometimes falling all the way through to the end, where we wind up dispatching anyway. Every place that we don't explicitly dispatch, the reader (or at least the reviewer) has to ask "does it make sense for things to stop being watched entirely". I think it's hard to verify that without going on quite some code-diving (which you did, and thank you for that!). The alternative that I've had in mind was to just |goto redispatch;| everyplace where you would otherwise |return|, so it's obvious that we are always at least attempting to go through the event loop. The one exception, of course, would be the ERROR_INVALID_HANDLE, which is already nicely commented to indicate why we're *not* going through the event loop. Another alternative is to separate things: IOTask::Run() { nsresult rv = RunInternal(); if (NS_FAILED(rv)) { // Log error. Maybe specially catch the ERROR_INVALID_HANDLE case, too, and // not re-dispatch ourself. // // Bonus points for identifying unique errors indicating which failure case // occurred, and that roughly correspond to the failure cases. } return NS_DispatchToCurrentThread(this); } WDYT? > > @@ +659,5 @@ > > > + if (!bRemoved) { > > > + MOZ_ASSERT(false); > > > + FILEWATCHERLOG( > > > + "NativeFileWatcherIOTask::RemovePathRunnableMethod - Unable to remove the change " > > > + "callback from the callback list for resource %S.", wrappedParameters->mPath); > > > > It is a little weird to log things after you have failed an assert. :) > > > > At a minimum, I think interchanging these calls would be an improvement. > > (And other calls like them.) > > > > Probably not worth asserting on failure here and similar places. > > Good catch, I've swapped the assertion and the log! IMHO, should keep them > both: MOZ_ASSERT should just work in debug builds while logging makes it > easier to trace errors in release builds as well. What do you think? Swapping them is fine. I'm still not sure that failure to find the thing is really something asserting over, but I'm not willing to argue over it. > > @@ +720,5 @@ > > > + > > > + // Enforce CloseHandle/CancelIO by disposing the AutoCloseHandle. We don't > > > + // remove the entry from mWatchedResourceBy* since the completion port might > > > + // still be using the notification buffer. Entry remove is performed when > > > + // handling ERROR_OPERATION_ABORTED in NativeFileWatcherIOTask::Run. > > > > Are we *guaranteed* that's going to happen? Or should we be dispatching a > > separate runnable to remove things from |mWatchedResourceBy| and just > > ignoring the ERROR_OPERATION_ABORTED? For instance, can we get too many > > notifications such that ERROR_OPERATION_ABORTED never gets sent and we > > essentially leak memory? > > I don't know if that's *guaranteed*, as MSDN doesn't mention it (but not > even the contrary), but I'm not quite sure that dispatching a runnable would > help. Basically, if we close an handle through > |toRemove->mResourceHandle.dispose()|, when the worker thread gets woken up > again it uses the notification buffer at least once before throwing that > error. If we dispatch a runnable to do the cleanup, we would need to be sure > the runnable gets called AFTER the worker thread wakes up again. > > Something like that: > > (1) Close the handle > (2) Enqueue the resource cleanup runnable > (3) Wake Up the worker thread > > [... some ops interrupt the worker thread again ] > > (4) The cleanup runnable gets called > > Moreover, if we keep it as it is right now, in the case > ERROR_OPERATION_ABORTED doesn't get fired, memory is eventually cleaned up > in "uninit()". In the end, I think that dispatching a new runnable to > cleanup each watch adds complexity and potentially new problems. Well, the memory is cleaned up in uninit(), but at that point we're shutting down! Would have been nicer to free memory incrementally as we went along, rather than leaking the memory until the end of the process. I agree that it adds complexity. The question is whether the complexity is worth it, and I think being able to avoid memory leaks might be worth it. The flow would be something like: (1) Close the handle (2) Create resource cleanup runnable if needed, or add handle to list of things for the cleanup runnable (this list can live on the runnable itself) (3) Store runnable in an IOTask field (4) Worker thread, whenever it wakes up, can check for a cleanup runnable to run, and if so, dispatch it right before it re-dispatches itself (made easier by following suggestions above ;), and clear the field. WDYT?

(In reply to Nathan Froyd (:froydnj) from comment #74) > I think that, at root, the whole thing is a readability issue. The way > things are structured now, the reader of the code knows that we should > always dispatch this runnable back to the event loop. > But then we encounter this odd process of sometimes dispatching back the > event loop in the middle, sometimes bailing out, and sometimes falling all > the way through to the end, where we wind up dispatching anyway. Every > place that we don't explicitly dispatch, the reader (or at least the > reviewer) has to ask "does it make sense for things to stop being watched > entirely". I think it's hard to verify that without going on quite some > code-diving (which you did, and thank you for that!). This is indeed a good point! > The alternative that I've had in mind was to just |goto redispatch;| > everyplace where you would otherwise |return|, so it's obvious that we are > always at least attempting to go through the event loop. The one exception, > of course, would be the ERROR_INVALID_HANDLE, which is already nicely > commented to indicate why we're *not* going through the event loop. This is the way I'd like to go! There are at least two cases in which I think we could remove the |return| part: - NativeFileWatcherWin.cpp:418, as dispatching again in that point would prevent watching that directory again. - NativeFileWatcherWin.cpp:437, as the next executed instruction would be line 443 which reschedules. > Well, the memory is cleaned up in uninit(), but at that point we're shutting > down! Would have been nicer to free memory incrementally as we went along, > rather than leaking the memory until the end of the process. > I agree that it adds complexity. The question is whether the complexity is > worth it, and I think being able to avoid memory leaks might be worth it. Again, this is definitely something worth considering. Good point, thanks for rising the awareness on this issue! I read the MSDN documentation again (YAY! :-) ) and it seems like ERROR_OPERATION_ABORTED guaranteed to be called when an I/O operation is canceled: "All I/O operations that are canceled complete with the error ERROR_OPERATION_ABORTED, and all completion notifications for the I/O operations occur normally." (http://msdn.microsoft.com/en-us/library/windows/desktop/aa363791%28v=vs.85%29.aspx) Moreover, this behaviour is further confirmed by another MSDN page (http://msdn.microsoft.com/en-us/library/windows/desktop/aa363789%28v=vs.85%29.aspx) which suggests the correct way to cancel and cleanup an asynchronous I/O operation: "You can determine if the cancel occurred by examining the status returned in the overlapped structure or in the completion callback. A status of ERROR_OPERATION_ABORTED indicates that the operation was canceled." That basically means that we cannot cleanup the resources until the I/O operation is canceled (and ERROR_OPERATION_ABORTED is dispatched), otherwise we would endup freeing something that's still being used!

(In reply to Alessio Placitelli [:Dexter] from comment #75) > (In reply to Nathan Froyd (:froydnj) from comment #74) > > The alternative that I've had in mind was to just |goto redispatch;| > > everyplace where you would otherwise |return|, so it's obvious that we are > > always at least attempting to go through the event loop. The one exception, > > of course, would be the ERROR_INVALID_HANDLE, which is already nicely > > commented to indicate why we're *not* going through the event loop. > > This is the way I'd like to go! OK, let's do that, then. If it gets too messy with an abundance of gotos, let's split things out into a separate RunLoop() or similar function. > I read the MSDN documentation again (YAY! :-) ) and it seems like > ERROR_OPERATION_ABORTED guaranteed to be called when an I/O operation is > canceled: Thanks for going back and investigating this. Let's trust the documentation and keep the scheme that we have now.

Comment on attachment 8457440 [details] [diff] [review] bug958280_v11.patch Review of attachment 8457440 [details] [diff] [review]: ----------------------------------------------------------------- Here we go. As far as I am concerned, code is good, with a few style/doc-nits and a question. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +393,5 @@ > + > + // Parse the changes and notify the main thread. > + const unsigned char* rawNotificationBuffer = changedRes->mNotificationBuffer; > + > + while(true) { Nit: `while (true) {` (whitespace) @@ +407,5 @@ > + nsAutoString resourcePath(changedRes->mPath > + + ((changedRes->mPath.RFindChar('/') != kNotFound) > + ? NS_LITERAL_STRING("/") > + : NS_LITERAL_STRING("\\")) > + + resourceName); Remind me: why don't you create a nsILocalFile from `mPath` then call `GetPath`? This should handle normalization with code that has seen plenty of testing. (and still has some bugs, unfortunately, but at least, they are rather well-known) @@ +480,5 @@ > + // Is aPathToWatch already being watched? > + WatchedResourceDescriptor* watchedResource = > + mWatchedResourcesByPath.Get(wrappedParameters->mPath); > + if (watchedResource) { > + // Append them to the hash tables as well. Nit: Who's "them" and why "as well"? @@ +568,5 @@ > + > + // We finally watch the resource for changes. > + nsresult rv = AddDirectoryToWatchList(resourceDesc); > + if (NS_FAILED(rv)) { > + // TODO: Remove the callbacks from the hashtables. Well, please do :) The most robust way to do this would probably to add a nice ScopedCustomThingy to remove the callbacks by default. Not sure it's worth it, though. Your call. @@ +579,5 @@ > + "NativeFileWatcherIOTask::AddPathRunnableMethod - Failed to watch %s and" > + " to dispatch the related error callbacks", resourceDesc->mPath); > + return rv; > + } else { > + // Otherwise, just don't add the descriptor to the watch list. Nit: since there is a `return`, shortest branch first and no `else` if (rv != NS_ERROR_ABORT) { // Just don't add the descriptor to the watch list. return NS_OK; } // Otherwise, abort FILEWATCHERLOG(...) return rv; @@ +587,5 @@ > + > + // Add the resource pointer to both indexes. > + WatchedResourceDescriptor* resource = resourceDesc.forget(); > + mWatchedResourcesByPath.Put(wrappedParameters->mPath, resource); > + mWatchedResourcesByHandle.Put(resHandle, resource); I think it would be clearer if you turned your if (NS_FAILED(rv)) { // Handle failure (long block) } into a if (NS_SUCCEEDED(rv)) { // Handle success (short block) } as this would keep all the adds syntactically close to each other. @@ +650,5 @@ > + if (!removed) { > + FILEWATCHERLOG( > + "NativeFileWatcherIOTask::RemovePathRunnableMethod - Unable to remove the change " > + "callback from the change callback hash map."); > + MOZ_ASSERT(false); Keep the log, but replace `MOZ_ASSERT` with `MOZ_CRASH([optional literal string explanation])`. Also, any chance you could put `mPath` in the log? Same below. @@ +674,5 @@ > + if (changeCallbackArray->Length()) { > + return NS_OK; > + } > + > + // We just Cancel the I/O in this runnable. Resources are freed by "In this runnable, we just cancel callbacks (see above) and I/O (see below)." @@ +702,5 @@ > +{ > + MOZ_ASSERT(!NS_IsMainThread()); > + > + // Remind users to manually remove the watches before quitting. > + MOZ_ASSERT(!mWatchedResourcesByHandle.Count()); Could you log all the resources that haven't been removed yet? MOZ_ASSERT(..., "Clients of the nsIFileWatcher must remove watches manually before quitting.") @@ +710,5 @@ > + if (mShuttingDown) { > + // If this happens, we are in a strange situation. > + FILEWATCHERLOG( > + "NativeFileWatcherIOTask::DeactivateRunnableMethod - We are already shutting down"); > + MOZ_ASSERT(false); MOZ_CRASH @@ +732,5 @@ > + > + // Close the IO completion port, eventually making > + // the watcher thread exit from the watching loop. > + if (mIOCompletionPort) { > + CloseHandle(mIOCompletionPort); Could `CloseHandle` fail? If so, could you LOG it? @@ +754,5 @@ > + */ > +nsresult > +NativeFileWatcherIOTask::DispatchChangeCallbacks( > + WatchedResourceDescriptor* aResourceDescriptor, > + const nsAString& aChangedResource) No need to make this a reference. @@ +768,5 @@ > + > + for (size_t i = 0; i < changeCallbackArray->Length(); i++) { > + nsresult rv = > + ReportChange((*changeCallbackArray)[i], aChangedResource); > + if (FAILED(rv)) { I don't remember that we have a macro `FAILED`. Could it be a Windows macro? We use `NS_FAILED`. @@ +918,5 @@ > + return NS_OK; > +} > + > +/** > + * Appends the change and error callbacks to the relative hash tables. "the relative" => "their respective" ::: toolkit/components/filewatcher/nsINativeFileWatcher.idl @@ +43,5 @@ > +{ > + /** > + * Initialises the watcher service. > + */ > + void init(); Let me be clearer: we need `init()` in the .h and .cpp, but not in the .idl.

(In reply to Alessio Placitelli [:Dexter] from comment #73) > > I suspect that we can now get rid of `init()`. > > Well, yes and no. I've used the > NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(NativeFileWatcherService, Init) macro in > nsToolkitCompsModule.cpp to let it automatically initialise. Removing the > Init() would mean I should take care of initialising the file watcher on the > first use. Do you think that's worth the change? I just meant that it doesn't need to appear in the .idl, just in the .h/.cpp. > > So, what happens if we register `onChange` and `onError` but unregister only `onChange`? > > The file watcher just considers the onChange callbacks when deciding to > close a watch on a resource. If there are no more onChange callbacks > associated to the watch, it gets closed (even though it still has some error > callbacks associated). Do not worry: the error callbacks get automatically > freed anyway. Can you document this?

This patch introduces the use of nsILocalFile in order to normalise the paths returned by the watcher. Moreover, it splits the watcher thread loop in two functions to help readability. Finally, it takes care of the other issues both Yoric and Froydnj pointed me to. @Yoric: You are going to hate me for this but... are we sure the |init| needs to be removed from the IDL file? In toolkit/components/build/nsToolkitCompsModule.cpp:100 we use the |init| so it MUST be there, IMHO, unless modify the |AddPath| to initialise the service on the first call. Otherwise, if somebody tries to write a new implementation of this component, how would he know about the need for an |init| function (well, except from the compiler complaining!)? @Froydnj: does the |RunInternal|/|Run| part look clearer now? I've settled for your second suggestion which seems somehow better (I figured it out while writing the code)!

(In reply to Alessio Placitelli [:Dexter] from comment #79) > @Froydnj: does the |RunInternal|/|Run| part look clearer now? I've settled > for your second suggestion which seems somehow better (I figured it out > while writing the code)! Yes, this looks better, thank you!

(In reply to Alessio Placitelli [:Dexter] from comment #79) > Created attachment 8464117 [details] [diff] [review] > bug958280_v12.patch > > This patch introduces the use of nsILocalFile in order to normalise the > paths returned by the watcher. Moreover, it splits the watcher thread loop > in two functions to help readability. Finally, it takes care of the other > issues both Yoric and Froydnj pointed me to. > > @Yoric: You are going to hate me for this but... are we sure the |init| > needs to be removed from the IDL file? In > toolkit/components/build/nsToolkitCompsModule.cpp:100 we use the |init| so > it MUST be there, IMHO, unless modify the |AddPath| to initialise the > service on the first call. > > Otherwise, if somebody tries to write a new implementation of this > component, how would he know about the need for an |init| function (well, > except from the compiler complaining!)? It uses NativeFileWatcherService::Init(), not nsINativeFileWatcherService::Init().

Comment on attachment 8464117 [details] [diff] [review] bug958280_v12.patch Waiting for the Init() to be settled before I review this patch.

This patch removes the Init() function from the IDL and replaces the deprecated ScopedDeleteArray with UniquePtr<>.

This patch enables user to provide an optional completion callback which will be called when a resource is successfully watched or unwatched.

The tests now make use of the optional success callback in order to prevent test failures due to files being written before a directory is actually watched. Additionally, every occurrence of the word "folder" was changed to "directory".

Comment on attachment 8480475 [details] [diff] [review] bug958280_v14.patch Review of attachment 8480475 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with only one question. ::: toolkit/components/filewatcher/NativeFileWatcherWin.cpp @@ +747,5 @@ > + > + if (!wrappedParameters || > + !wrappedParameters->mChangeCallbackHandle || > + !wrappedParameters->mErrorCallbackHandle || > + !wrappedParameters->mSuccessCallbackHandle) { I don't understand why we need mSErrorCallbackHandle and mSuccessCallbackHandle to be non-null.

Comment on attachment 8480482 [details] [diff] [review] bug958280_tests_v9.patch Review of attachment 8480482 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I believe that my version would be a little cleaner :) ::: toolkit/components/filewatcher/tests/xpcshell/head.js @@ +5,5 @@ > +"use strict"; > + > +let {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; > + > +Cu.import("resource://gre/modules/osfile.jsm", this); Cu.import("resource://gre/modules/Promise.jsm", this); (this will give you the version of Promise with extended error reporting) @@ +11,5 @@ > +function makeWatcher() { > + let watcher = > + Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] > + .getService(Ci.nsINativeFileWatcherService); > + return watcher; I would add a function function promiseAddPath(watcher, onChange=null, onError=null) { return new Promise(resolve => watcher.addPath(onChange, onError, resolve) ); } and call `yield promiseAddPath(...)` everywhere, to be sure that we only proceed once the `addPath` operation is complete.

Thank you for your review. I've followed your suggestion used the promiseAddPath (I've introduced a promiseRemovePath as well): the tests are much more readable now! As for the mErrorCallbackHandle and mSuccessCallbackHandle forced to be non-null, good catch! There's no reason they should be non-null, as we don't require them to be always passed as arguments.

Comment on attachment 8481906 [details] [diff] [review] bug958280_tests_v10.patch ># HG changeset patch ># Parent abf932950cea2498fa62716c4c38147cd0be8bd7 ># User Alessio Placitelli <alessio.placitelli@gmail.com> >Bug 958280: adds xpcshell tests for the NativeFileWatcher. > >diff --git a/toolkit/components/filewatcher/moz.build b/toolkit/components/filewatcher/moz.build >--- a/toolkit/components/filewatcher/moz.build >+++ b/toolkit/components/filewatcher/moz.build >@@ -1,14 +1,16 @@ > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > # vim: set filetype=python: > # 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/. > >+XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell/xpcshell.ini'] >+ > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > EXPORTS += ['NativeFileWatcherWin.h'] > UNIFIED_SOURCES += [ > 'NativeFileWatcherWin.cpp', > ] > else: > EXPORTS += ['NativeFileWatcherNotSupported.h'] > >diff --git a/toolkit/components/filewatcher/tests/xpcshell/head.js b/toolkit/components/filewatcher/tests/xpcshell/head.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/head.js >@@ -0,0 +1,29 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+let {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components; >+ >+Cu.import("resource://gre/modules/osfile.jsm", this); >+Cu.import("resource://gre/modules/Promise.jsm", this); >+ >+function makeWatcher() { >+ let watcher = >+ Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'] >+ .getService(Ci.nsINativeFileWatcherService); >+ return watcher; >+}; >+ >+function promiseAddPath(watcher, resource, onChange=null, onError=null) { >+ return new Promise(resolve => >+ watcher.addPath(resource, onChange, onError, resolve) >+ ); >+} >+ >+function promiseRemovePath(watcher, resource, onChange=null, onError=null) { >+ return new Promise(resolve => >+ watcher.removePath(resource, onChange, onError, resolve) >+ ); >+} >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_arguments.js b/toolkit/components/filewatcher/tests/xpcshell/test_arguments.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_arguments.js >@@ -0,0 +1,71 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test for addPath usage with null arguments. >+ */ >+add_task(function* test_null_args_addPath() { >+ >+ let watcher = makeWatcher(); >+ let testPath = 'someInvalidPath'; >+ >+ // Define a dummy callback function. In this test no callback is >+ // expected to be called. >+ let dummyFunc = function(changed) { >+ do_throw("Not expected in this test."); >+ }; >+ >+ // Check for error when passing a null first argument >+ try { >+ watcher.addPath(testPath, null, dummyFunc); >+ } catch (ex if ex.result == Cr.NS_ERROR_NULL_POINTER) { >+ do_print("Initialisation thrown NS_ERROR_NULL_POINTER as expected."); >+ } >+ >+ // Check for error when passing both null arguments >+ try { >+ watcher.addPath(testPath, null, null); >+ } catch (ex if ex.result == Cr.NS_ERROR_NULL_POINTER) { >+ do_print("Initialisation thrown NS_ERROR_NULL_POINTER as expected."); >+ } >+}); >+ >+/** >+ * Test for removePath usage with null arguments. >+ */ >+add_task(function* test_null_args_removePath() { >+ >+ let watcher = makeWatcher(); >+ let testPath = 'someInvalidPath'; >+ >+ // Define a dummy callback function. In this test no callback is >+ // expected to be called. >+ let dummyFunc = function(changed) { >+ do_throw("Not expected in this test."); >+ }; >+ >+ // Check for error when passing a null first argument >+ try { >+ watcher.removePath(testPath, null, dummyFunc); >+ } catch (ex if ex.result == Cr.NS_ERROR_NULL_POINTER) { >+ do_print("Initialisation thrown NS_ERROR_NULL_POINTER as expected."); >+ } >+ >+ // Check for error when passing both null arguments >+ try { >+ watcher.removePath(testPath, null, null); >+ } catch (ex if ex.result == Cr.NS_ERROR_NULL_POINTER) { >+ do_print("Initialisation thrown NS_ERROR_NULL_POINTER as expected."); >+ } >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_no_error_callback.js b/toolkit/components/filewatcher/tests/xpcshell/test_no_error_callback.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_no_error_callback.js >@@ -0,0 +1,69 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test the component behaves correctly when no error callback is >+ * provided and an error occurs. >+ */ >+add_task(function* test_error_with_no_error_callback() { >+ >+ let watcher = makeWatcher(); >+ let testPath = 'someInvalidPath'; >+ >+ // Define a dummy callback function. In this test no callback is >+ // expected to be called. >+ let dummyFunc = function(changed) { >+ do_throw("Not expected in this test."); >+ }; >+ >+ // We don't pass an error callback and try to watch an invalid >+ // path. >+ watcher.addPath(testPath, dummyFunc); >+}); >+ >+/** >+ * Test the component behaves correctly when no error callback is >+ * provided (no error should occur). >+ */ >+add_task(function* test_watch_single_path_file_creation_no_error_cb() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ let tempFileName = "test_filecreation.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Watch the profile directory but do not pass an error callback. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve); >+ >+ // Create a file within the watched directory. >+ let tmpFilePath = OS.Path.join(watchedDir, tempFileName); >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Wait until the watcher informs us that the file was created. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' to unregister). >+ watcher.removePath(watchedDir, deferred.resolve); >+ >+ // Remove the test directory and all of its content. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_remove_non_watched.js b/toolkit/components/filewatcher/tests/xpcshell/test_remove_non_watched.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_remove_non_watched.js >@@ -0,0 +1,39 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test removing non watched path >+ */ >+add_task(function* test_remove_not_watched() { >+ let nonExistingDir = >+ OS.Path.join(OS.Constants.Path.profileDir, "absolutelyNotExisting"); >+ >+ // Instantiate the native watcher. >+ let watcher = makeWatcher(); >+ >+ // Try to un-watch a path which wasn't being watched. >+ watcher.removePath( >+ nonExistingDir, >+ function(changed) { >+ do_throw("No change is expected in this test."); >+ }, >+ function(xpcomError, osError) { >+ // When removing a resource which wasn't being watched, it should silently >+ // ignore the request. >+ do_throw("Unexpected exception: " >+ + xpcomError + " (XPCOM) " >+ + osError + " (OS Error)"); >+ } >+ ); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_shared_callback.js b/toolkit/components/filewatcher/tests/xpcshell/test_shared_callback.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_shared_callback.js >@@ -0,0 +1,62 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test the watcher correctly handles two watches sharing the same >+ * change callback. >+ */ >+add_task(function* test_watch_with_shared_callback() { >+ >+ // Create and watch two sub-directories of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDirs = >+ [ >+ OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"), >+ OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground2") >+ ]; >+ >+ yield OS.File.makeDir(watchedDirs[0]); >+ yield OS.File.makeDir(watchedDirs[1]); >+ >+ let tempFileName = "test_filecreation.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Watch both directories using the same callbacks. >+ yield promiseAddPath(watcher, watchedDirs[0], deferred.resolve, deferred.reject); >+ yield promiseAddPath(watcher, watchedDirs[1], deferred.resolve, deferred.reject); >+ >+ // Remove the watch for the first directory, but keep watching >+ // for changes in the second: we need to make sure the callback >+ // survives the removal of the first watch. >+ watcher.removePath(watchedDirs[0], deferred.resolve, deferred.reject); >+ >+ // Create a file within the watched directory. >+ let tmpFilePath = OS.Path.join(watchedDirs[1], tempFileName); >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Wait until the watcher informs us that the file was created. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ watcher.removePath(watchedDirs[1], deferred.resolve, deferred.reject); >+ >+ // Remove the test directories and all of their content. >+ yield OS.File.removeDir(watchedDirs[0]); >+ yield OS.File.removeDir(watchedDirs[1]); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_creation_single.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_creation_single.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_creation_single.js >@@ -0,0 +1,49 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files >+ do_get_profile(); >+ >+ // Start executing the tests >+ run_next_test(); >+} >+ >+/** >+ * Tests that the watcher correctly notifies of a directory creation when watching >+ * a single path. >+ */ >+add_task(function* test_watch_single_path_directory_creation() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ let tmpDirPath = OS.Path.join(watchedDir, "testdir"); >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Once ready, create a directory within the watched directory. >+ yield OS.File.makeDir(tmpDirPath); >+ >+ // Wait until the watcher informs us that the file has changed. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpDirPath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Clean up the test directory. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_deletion_single.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_deletion_single.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_directory_deletion_single.js >@@ -0,0 +1,46 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Tests that the watcher correctly notifies of a directory deletion when watching >+ * a single path. >+ */ >+add_task(function* test_watch_single_path_directory_deletion() { >+ >+ let watchedDir = OS.Constants.Path.profileDir; >+ let tempDirName = "test"; >+ let tmpDirPath = OS.Path.join(watchedDir, tempDirName); >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Create a directory within the watched directory. >+ yield OS.File.makeDir(tmpDirPath); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching it. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the directory. >+ OS.File.removeDir(tmpDirPath); >+ >+ // Wait until the watcher informs us that the file has changed. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpDirPath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_creation_single.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_creation_single.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_creation_single.js >@@ -0,0 +1,51 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test the watcher correctly notifies of a file creation when watching >+ * a single path. >+ */ >+add_task(function* test_watch_single_path_file_creation() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ let tempFileName = "test_filecreation.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ let tmpFilePath = OS.Path.join(watchedDir, tempFileName); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // create the file within the watched directory. >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Wait until the watcher informs us that the file was created. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the test directory and all of its content. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_deletion_single.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_deletion_single.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_deletion_single.js >@@ -0,0 +1,54 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+/** >+ * Test the watcher correctly notifies of a file deletion when watching >+ * a single path. >+ */ >+add_task(function* test_watch_single_path_file_deletion() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ let tempFileName = "test_filedeletion.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Create a file within the directory to be watched. We do this >+ // before watching the directory so we do not get the creation notification. >+ let tmpFilePath = OS.Path.join(watchedDir, tempFileName); >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching it. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the file we created (should trigger a notification). >+ do_print('Removing ' + tmpFilePath); >+ yield OS.File.remove(tmpFilePath); >+ >+ // Wait until the watcher informs us that the file was deleted. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the test directory and all of its content. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_modification_single.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_modification_single.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_file_modification_single.js >@@ -0,0 +1,54 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files >+ do_get_profile(); >+ >+ // Start executing the tests >+ run_next_test(); >+} >+ >+/** >+ * Tests that the watcher correctly notifies of a file modification when watching >+ * a single path. >+ */ >+add_task(function* test_watch_single_path_file_modification() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ let tempFileName = "test_filemodification.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Create a file within the directory to be watched. We do this >+ // before watching the directory so we do not get the creation notification. >+ let tmpFilePath = OS.Path.join(watchedDir, tempFileName); >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching it. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Once ready, modify the file to trigger the notification. >+ yield OS.File.writeAtomic(tmpFilePath, "some new data"); >+ >+ // Wait until the watcher informs us that the file has changed. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the test directory and all of its content. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_many_changes.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_many_changes.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_many_changes.js >@@ -0,0 +1,73 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files >+ do_get_profile(); >+ >+ // Start executing the tests >+ run_next_test(); >+} >+ >+/** >+ * Test that we correctly handle watching directories when hundreds of files >+ * change simultaneously. >+ */ >+add_task(function* test_fill_notification_buffer() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ // The number of files to create. >+ let numberOfFiles = 100; >+ let fileNameBase = "testFile"; >+ >+ // This will be used to keep track of the number of changes within the watched >+ // directory. >+ let detectedChanges = 0; >+ >+ // We expect at least the following notifications for each file: >+ // - File creation >+ // - File deletion >+ let expectedChanges = numberOfFiles * 2; >+ >+ // Instantiate the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Initialise the change callback. >+ let changeCallback = function(changed) { >+ do_print(changed + " has changed."); >+ >+ detectedChanges += 1; >+ >+ // Resolve the promise if we get all the expected changes. >+ if (detectedChanges >= expectedChanges) { >+ deferred.resolve(); >+ } >+ }; >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching it. >+ yield promiseAddPath(watcher, watchedDir, changeCallback, deferred.reject); >+ >+ // Create and then remove the files within the watched directory. >+ for (let i = 0; i < numberOfFiles; i++) { >+ let tmpFilePath = OS.Path.join(watchedDir, fileNameBase + i); >+ yield OS.File.writeAtomic(tmpFilePath, "test content"); >+ yield OS.File.remove(tmpFilePath); >+ } >+ >+ // Wait until the watcher informs us that all the files were >+ // created, modified and removed. >+ yield deferred.promise; >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'changeCallback' and 'errorCallback' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, changeCallback, deferred.reject); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_multi_paths.js >@@ -0,0 +1,114 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files >+ do_get_profile(); >+ >+ // Start executing the tests >+ run_next_test(); >+} >+ >+/** >+ * Tests the watcher by watching several resources. >+ * This test creates the specified number of directory inside the profile >+ * directory, adds each one of them to the watched list the creates >+ * a file in them in order to trigger the notification. >+ * The test keeps track of the number of times the changes callback is >+ * called in order to verify the success of the test. >+ */ >+add_task(function* test_watch_multi_paths() { >+ >+ // The number of resources to watch. We expect changes for >+ // creating a file within each directory. >+ let resourcesToWatch = 5; >+ let watchedDir = OS.Constants.Path.profileDir; >+ >+ // The directories to be watched will be created with. >+ let tempDirNameBase = "FileWatcher_Test_"; >+ let tempFileName = "test.tmp"; >+ >+ // Instantiate the native watcher. >+ let watcher = makeWatcher(); >+ >+ // This will be used to keep track of the number of changes within the watched >+ // resources. >+ let detectedChanges = 0; >+ let watchedResources = 0; >+ let unwatchedResources = 0; >+ >+ let deferredChanges = Promise.defer(); >+ let deferredSuccesses = Promise.defer(); >+ let deferredShutdown = Promise.defer(); >+ >+ // Define the change callback function. >+ let changeCallback = function(changed) { >+ do_print(changed + " has changed."); >+ >+ detectedChanges += 1; >+ >+ // Resolve the promise if we get all the expected changes. >+ if (detectedChanges === resourcesToWatch) { >+ deferredChanges.resolve(); >+ } >+ }; >+ >+ // Define the watch success callback function. >+ let watchSuccessCallback = function(resourcePath) { >+ do_print(resourcePath + " is being watched."); >+ >+ watchedResources += 1; >+ >+ // Resolve the promise when all the resources are being >+ // watched. >+ if (watchedResources === resourcesToWatch) { >+ deferredSuccesses.resolve(); >+ } >+ }; >+ >+ // Define the watch success callback function. >+ let unwatchSuccessCallback = function(resourcePath) { >+ do_print(resourcePath + " is being un-watched."); >+ >+ unwatchedResources += 1; >+ >+ // Resolve the promise when all the resources are being >+ // watched. >+ if (unwatchedResources === resourcesToWatch) { >+ deferredShutdown.resolve(); >+ } >+ }; >+ >+ // Create the directories and add them to the watched resources list. >+ for (let i = 0; i < resourcesToWatch; i++) { >+ let tmpSubDirPath = OS.Path.join(watchedDir, tempDirNameBase + i); >+ do_print("Creating the " + tmpSubDirPath + " directory."); >+ yield OS.File.makeDir(tmpSubDirPath); >+ watcher.addPath(tmpSubDirPath, changeCallback, deferredChanges.reject, watchSuccessCallback); >+ } >+ >+ // Wait until the watcher informs us that all the desired resources >+ // are being watched. >+ yield deferredSuccesses.promise; >+ >+ // Create a file within each watched directory. >+ for (let i = 0; i < resourcesToWatch; i++) { >+ let tmpFilePath = OS.Path.join(watchedDir, tempDirNameBase + i, tempFileName); >+ yield OS.File.writeAtomic(tmpFilePath, "test content"); >+ } >+ >+ // Wait until the watcher informs us that all the files were created. >+ yield deferredChanges.promise; >+ >+ // Remove the directories we have created. >+ for (let i = 0; i < resourcesToWatch; i++) { >+ let tmpSubDirPath = OS.Path.join(watchedDir, tempDirNameBase + i); >+ watcher.removePath(tmpSubDirPath, changeCallback, deferredChanges.reject, unwatchSuccessCallback); >+ } >+ >+ // Wait until the watcher un-watches the resources. >+ yield deferredShutdown.promise; >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_recursively.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_recursively.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_recursively.js >@@ -0,0 +1,55 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files. >+ do_get_profile(); >+ >+ // Start executing the tests. >+ run_next_test(); >+} >+ >+/** >+ * Test the watcher correctly notifies of a file creation in a subdirectory >+ * of the watched sub-directory (recursion). >+ */ >+add_task(function* test_watch_recursively() { >+ >+ // Create and watch a sub-directory of the profile directory so we don't >+ // catch notifications we're not interested in (i.e. "startupCache"). >+ let watchedDir = OS.Path.join(OS.Constants.Path.profileDir, "filewatcher_playground"); >+ yield OS.File.makeDir(watchedDir); >+ >+ // We need at least 2 levels of directories to test recursion. >+ let subdirectory = OS.Path.join(watchedDir, "level1"); >+ yield OS.File.makeDir(subdirectory); >+ >+ let tempFileName = "test_filecreation.tmp"; >+ >+ // Instantiate and initialize the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ let tmpFilePath = OS.Path.join(subdirectory, tempFileName); >+ >+ // Add the profile directory to the watch list and wait for the file watcher >+ // to start watching it. >+ yield promiseAddPath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Create a file within the subdirectory of the watched directory. >+ yield OS.File.writeAtomic(tmpFilePath, "some data"); >+ >+ // Wait until the watcher informs us that the file was created. >+ let changed = yield deferred.promise; >+ do_check_eq(changed, tmpFilePath); >+ >+ // Remove the watch and free the associated memory (we need to >+ // reuse 'deferred.resolve' and 'deferred.reject' to unregister). >+ yield promiseRemovePath(watcher, watchedDir, deferred.resolve, deferred.reject); >+ >+ // Remove the test directory and all of its content. >+ yield OS.File.removeDir(watchedDir); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/test_watch_resource.js b/toolkit/components/filewatcher/tests/xpcshell/test_watch_resource.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/test_watch_resource.js >@@ -0,0 +1,32 @@ >+/* 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/. */ >+ >+"use strict"; >+ >+function run_test() { >+ // Set up profile. We will use profile path create some test files >+ do_get_profile(); >+ >+ // Start executing the tests >+ run_next_test(); >+} >+ >+/** >+ * Test watching non-existing path >+ */ >+add_task(function* test_watching_non_existing() { >+ let notExistingDir = >+ OS.Path.join(OS.Constants.Path.profileDir, "absolutelyNotExisting"); >+ >+ // Instantiate the native watcher. >+ let watcher = makeWatcher(); >+ let deferred = Promise.defer(); >+ >+ // Try watch a path which doesn't exist. >+ watcher.addPath(notExistingDir, deferred.reject, deferred.resolve); >+ >+ // Wait until the watcher informs us that there was an error. >+ let error = yield deferred.promise; >+ do_check_eq(error, Components.results.NS_ERROR_FILE_NOT_FOUND); >+}); >diff --git a/toolkit/components/filewatcher/tests/xpcshell/xpcshell.ini b/toolkit/components/filewatcher/tests/xpcshell/xpcshell.ini >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/filewatcher/tests/xpcshell/xpcshell.ini >@@ -0,0 +1,18 @@ >+[DEFAULT] >+head = head.js >+tail = >+ >+skip-if = os != "win" >+[test_arguments.js] >+[test_no_error_callback.js] >+[test_remove_non_watched.js] >+[test_shared_callback.js] >+[test_watch_file_creation_single.js] >+[test_watch_file_deletion_single.js] >+[test_watch_file_modification_single.js] >+[test_watch_directory_creation_single.js] >+[test_watch_directory_deletion_single.js] >+[test_watch_many_changes.js] >+[test_watch_multi_paths.js] >+[test_watch_recursively.js] >+[test_watch_resource.js]

Comment on attachment 8481912 [details] [diff] [review] bug958280_v15.patch Review of attachment 8481912 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me \o/

Comment on attachment 8481907 [details] [diff] [review] bug958280_tests_v10.patch Review of attachment 8481907 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me \o/

I've added the .xpt to the various package manifests (b2g, android, browser). Tests were failing with v15 because of that :)

Try server: https://tbpl.mozilla.org/?tree=Try&rev=171fdbf91fbe

Comment on attachment 8482650 [details] [diff] [review] bug958280_v16.patch As suggested by Yoric, I'm requesting this review since my patch modifies the build system as well :) Thanks in advance!

Comment on attachment 8482650 [details] [diff] [review] bug958280_v16.patch Review of attachment 8482650 [details] [diff] [review]: ----------------------------------------------------------------- There's nothing in the build system changes in this patch that needs a build config peer review. ::: toolkit/components/filewatcher/moz.build @@ +6,5 @@ > + > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > + EXPORTS += ['NativeFileWatcherWin.h'] > + UNIFIED_SOURCES += [ > + 'NativeFileWatcherWin.cpp', Okay, just a nit here, on the indentation.

Comment on attachment 8482650 [details] [diff] [review] bug958280_v16.patch I have done the rest of the review already. Oh, Dexter, don't forget the `,r=Yoric` in the commit message.

Thank you glandium, I've fixed the nit and added the r=Yoric in both patches as requested!

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc66ed2e4ce https://hg.mozilla.org/integration/mozilla-inbound/rev/db85cbf798e2 I fixed the bug number in your commit messages. Please double check them in the future before requesting checkin.

Whoops, sorry. I'll keep that in mind next time! (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #102) > https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc66ed2e4ce > https://hg.mozilla.org/integration/mozilla-inbound/rev/db85cbf798e2 > > I fixed the bug number in your commit messages. Please double check them in > the future before requesting checkin.

https://hg.mozilla.org/mozilla-central/rev/bbc66ed2e4ce https://hg.mozilla.org/mozilla-central/rev/db85cbf798e2

My nightly throws errors with on first run of this code. Then on second run it crashes :( Please copy and paste this to scrathpad: ////////// var nfw = Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'].getService(Ci.nsINativeFileWatcherService); var doc = OS.Path.join(OS.Constants.Path.desktopDir, 'Word doc.docx'); function onc(a) { console.log('onc', 'a:', a); } function one(e) { console.log('one', 'e:', e); } nfw.addPath(doc, onc, one); ////////////// create a doc file on desktop named "Word doc.docx" run the scratchpad code, browser console contains errors. run the scratchpad code again and the browser crashes :(

(In reply to noitidart from comment #105) > My nightly throws errors with on first run of this code. Then on second run > it crashes :( Thank you for reporting this! I've filed bug 1104660 to track this issue. Moreover, you should be creating a watch on the directory where "Word doc.docx" is (see http://dxr.mozilla.org/mozilla-central/source/toolkit/components/filewatcher/nsINativeFileWatcher.idl#59 for more information). Paste the following example to scratchpad (you don't need to wait for bug 1104660 to land!): // --- var nfw = Cc['@mozilla.org/toolkit/filewatcher/native-file-watcher;1'].getService(Ci.nsINativeFileWatcherService); function onc(a) { if (a.indexOf('Word doc.docx') !== -1) { console.log('onc', 'a:', a); } } function one(e) { console.log('one', 'e:', e); } nfw.addPath(OS.Constants.Path.desktopDir, onc, one); // --- Please, let us know if that works for you!

Thanks Alessio! That works great, but some issues. 1. When the file is modified it always throws two messages within 1ms of each other. I couldn't figure iit out but then i renamed the file and it doing a `console.log('a:', a; logged this: "onc" "a:" "C:\Users\Noit\Desktop\Word Doc.docx" Scratchpad/1:5:4 "onc" "a:" "C:\Users\Noit\Desktop\Word doc ren.docx" It looks real useful, it looks like the first message holds the old file name and the new one the new file name, is this really two seperate events? I would think this should be one message but with a being an object {newName: 'Word doc ren.docx', oldName:'Word doc.docx'}. I'm just thinking. You made a superb thing here! Im not trying to ding it just learn about it and maybe by chance help imrpvoe it :) 1.5 issue: I have a bunch of files being modified in this directory like a bunch a bunch so this notifier is running like 100 times out of which only one of them is the file i want to watch. Is watching directly the only way? It just seems to be a lot of overhead because every time it notifies I use regex to do a case insentive test on the path name. 2. After running the code, and not doing anything, but wait like 15 seconds. It doesn't throw any errors but it does throw this exception 5 times instantly. ////////////// A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'? See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise Date: Tue Nov 25 2014 10:50:00 GMT-0800 (Pacific Standard Time) Full Message: null Full Stack: JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 165 JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 678 JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_handleException :: line 448 JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 326 JS frame :: resource://gre/modules/Task.jsm :: TaskImpl :: line 275 JS frame :: resource://gre/modules/Task.jsm :: createAsyncFunction/asyncFunction :: line 249 JS frame :: resource://gre/modules/Task.jsm :: Task_spawn :: line 164 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: .linkToInspector :: line 3095 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: ._renderElementNode :: line 3076 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: .render :: line 2941 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Widgets.JSObject.prototype<.render :: line 2286 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.Extended.prototype<._renderObjectActor :: line 1190 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.Extended.prototype<._renderValueGrip :: line 1098 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.Extended.prototype<._renderBodyPiece :: line 1064 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.ConsoleGeneric.prototype<._renderBodyPiece :: line 1404 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.ConsoleGeneric.prototype<._renderBodyPieces :: line 1395 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/console-output.js :: Messages.ConsoleGeneric.prototype<.render :: line 1310 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/webconsole.js :: WCF_logConsoleAPIMessage :: line 1221 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/webconsole.js :: WCF__outputMessageFromQueue :: line 2260 JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/webconsole.js :: WCF__flushMessageQueue :: line 2148 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 ///////////////////////////////////////////////

Also a note about issue 1. If throw two messages per file modify, is there any reliabile way to test if the messages are due to a single change? Like other then comparing timestmaps to see if it's within 1ms.

btw one of the applications for this is an addon which allows editing of files straight out of cloud drives: on cloud drive sites addon adds a button for "Open with", it then checks to see if the file on the hardrive is synced with it, if it is then it opens that. If it is not, or file is not even on hd, then it downloads a copy to temp folder and then opens it with the application (ie: photoshop). file watcher watches the file, and as user saves it uploads to cloud service.

(In reply to noitidart from comment #107) > Thanks Alessio! That works great, but some issues. Thanks for testing this in the first place! > 1. When the file is modified it always throws two messages within 1ms of > each other. > > I couldn't figure iit out but then i renamed the file and it doing a > `console.log('a:', a; logged this: > > "onc" "a:" "C:\Users\Noit\Desktop\Word Doc.docx" Scratchpad/1:5:4 > "onc" "a:" "C:\Users\Noit\Desktop\Word doc ren.docx" > > > It looks real useful, it looks like the first message holds the old file > name and the new one the new file name, is this really two seperate events? > I would think this should be one message but with a being an object > {newName: 'Word doc ren.docx', oldName:'Word doc.docx'}. I'm just thinking. > You made a superb thing here! Im not trying to ding it just learn about it > and maybe by chance help imrpvoe it :) Don't worry, it's good to discuss things like these :-) Also, feel free to drop by IRC ;) Some modifications (renaming a file being the most immediate example) trigger multiple changes (i.e. when renaming a file, a change callback is triggered with a "DELETE" event with the old file name, and new one with a "CREATED" event for the new file name). The next iteration of the file watcher will feature a new change callback parameter which will allow to understand what kind of change is taking place. > 1.5 issue: I have a bunch of files being modified in this directory like a > bunch a bunch so this notifier is running like 100 times out of which only > one of them is the file i want to watch. Is watching directly the only way? > It just seems to be a lot of overhead because every time it notifies I use > regex to do a case insentive test on the path name. Some platforms restrict what you can watch. Windows, as an example, allows to watch a directory for changes and there isn't, at least to my knowledge, an API to directly watch a file. We could resort to polling, but it would probably have a bigger impact on performances. > 2. After running the code, and not doing anything, but wait like 15 seconds. > It doesn't throw any errors but it does throw this exception 5 times > instantly. It does not seem to be related to the file watcher, but I'll keep on eye on it.

Hi Alessio what's your irc name I had some questions as i was implementing this in my addon. Thanks!

Hey Dexter, P0lip and I were working on this in js-ctypes and we were wondering if ReadDirectoryChangesW works only per a single file path. Like can we addPaths to this? Also are you on IRC under a different name I was looking for you to ask you some questions on this :P as we're trying the same via js-ctypes (got linux, *bsd, and mac in working-ish orders, winnt has least work done im trying to understand the API)

BTW out current assumption was that ReadDirectoryChangesW only exists per file so the way we make .addPath work is, we make our poll worker, create a new LPOVERLAPPED with the same callback. And on watcher.close we terminate that poll worker. so we are right now making a worker per each new Watcher (going by the api defined here: https://bugzilla.mozilla.org/show_bug.cgi?id=958280#c1)

(In reply to noitidart from comment #112) > Hey Dexter, P0lip and I were working on this in js-ctypes and we were > wondering if ReadDirectoryChangesW works only per a single file path. It works per single directory path, as described in [1]. [1] - https://msdn.microsoft.com/en-us/library/windows/desktop/aa365465%28v=vs.85%29.aspx

Hi all I have js-ctypes version ready for use. However after build 40.0a1 - 2015-04-16 my `poll` and `read` calls are returning immeidately with error of EINTR. If I loop it 100 times, i get 100 EINTR immeidately. If I try 1k times, I get 1k EINTR. So the inotify is not working. Is anyone interested in helping me figure out why I can't do a simple `read`/`poll`, please see bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1288293