992896 - [OS.File] Add a function watching for a file being changed under Linux

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Splitting bug 958280 in per-platform implementations.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug_992896.patch

That's the first version, all the tests are green, and the recursive watching of files is supported.

I don't like very much the idea of having |PropagateChangeNotification| as a public method, but making |ChangeNotificationProxy| a static class member required to use GLIB GIO headers in the .h file. Making it a friend method creates the same problem.

Does anybody have any suggestion on a more elegant way to handle this?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

It will probably take me a few days to review this.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8526727 [details] [diff] [review]
bug_992896.patch

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

I'm not going to have time to finish reviewing it this week, as I promised. Sorry about that.
Here's an incomplete review.

::: toolkit/components/build/nsToolkitCompsModule.cpp
@@ +38,5 @@
>  #include "mozilla/AddonPathService.h"
>  
>  #if defined(XP_WIN)
>  #include "NativeFileWatcherWin.h"
> +#elif defined(XP_UNIX)

Actually, that's for all Gtk builds, so use MOZ_ENABLE_GTK instead.

@@ +39,5 @@
>  
>  #if defined(XP_WIN)
>  #include "NativeFileWatcherWin.h"
> +#elif defined(XP_UNIX)
> +#include "NativeFileWatcherLinux.h"

Let's call it NativeFileWatcherGIO

::: toolkit/components/filewatcher/NativeFileWatcherLinux.cpp
@@ +53,5 @@
> +   * @param aError The |nsresult| error value.
> +   * @param osError The error returned by GetLastError().
> +   */
> +  WatchedErrorEvent(const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError,
> +                    const nsresult& anError, const int osError)

`nsresult&` seems overkill, `nsresult` would be sufficient.

@@ +67,5 @@
> +
> +    // Make sure we wrap a valid callback since it's not mandatory to provide
> +    // one when watching a resource.
> +    if (mOnError) {
> +      (void)mOnError->Complete(mError, mOsError);

Do we really need to dispatch if `mOnError` is `null`?

@@ +105,5 @@
> +
> +    // Make sure we wrap a valid callback since it's not mandatory to provide
> +    // one when watching a resource.
> +    if (mOnSuccess) {
> +      (void)mOnSuccess->Complete(mResourcePath);

Do we really need to dispatch if `mOnSuccess` is `null`?

@@ +176,5 @@
> +      g_error_free(anError);
> +    }
> +  }
> +};
> +typedef Scoped<AutoFreeErrorTraits> AutoFreeError;

We are progressively moving from `Scoped` to `UniquePtr`. Could you take the opportunity to use `UniquePtr`, here and below?

@@ +248,5 @@
> + *        The resource which changed.
> + *
> + * @param aSecondFile
> + *        When moving resources, this holds the destination path. Not currently used.
> + *        See https://github.com/GNOME/glib/blob/master/gio/gfilemonitor.c#L234 .

Nit: linking to a C file looks a bit weird. If you link to a specific line, you should probably also attach the link to a specific commit.

@@ +254,5 @@
> + * @param anEvent
> + *        The type of change detected by the monitor.
> + *
> + * @param anUserData
> + *        |NativeFileWatcherService| instance pointer passed by |g_signal_connect|.

Nit: `aUserData` would be sufficient. The prefix "a-" stands for argument, not for "a" :)

@@ +261,5 @@
> +ChangeNotificationProxy(GFileMonitor* aMonitor, GFile* aFirstFile, GFile* aSecondFile,
> +                        GFileMonitorEvent anEvent, gpointer anUserData)
> +{
> +  MOZ_ASSERT(anUserData);
> +  NativeFileWatcherService* watcherService = static_cast<NativeFileWatcherService*>(anUserData);

Could we make this a UniquePtr?

@@ +268,5 @@
> +  gchar* filePath = g_file_get_path(aFirstFile);
> +
> +  nsAutoString utf16Path = NS_ConvertASCIItoUTF16(filePath);
> +
> +  g_free(filePath);

Any way you could make this a UniquePtr?

@@ +635,5 @@
> +  mWatchedResourcesByHandle.Put(resMonitor, resource);
> +
> +  // Connect the result callback function.
> +  int connectionId =
> +    g_signal_connect(resMonitor, "changed", G_CALLBACK (ChangeNotificationProxy), this);

Can you point me to the manual that defines `changed`?

@@ +652,5 @@
> +    }
> +  }
> +
> +  // Watch subdirectories to watch as well.
> +  ScanSubdirectories(utf8Path, wrappedParameters.get(), true);

Note for later: I have come to suspect that we will eventually want to be able to have both a version that watches with subdirectories and one that doesn't. Nothing to do yet, but let's keep it in mind.

Also, I'm a bit surprised that ScanSubdirectories cannot fail.

@@ +783,5 @@
> +  mWatchedResourcesByPath.Remove(toRemove->mPath);
> +
> +  // Stop watching subdirectories.
> +  nsCString utf8Path = NS_ConvertUTF16toUTF8(wrappedParameters->mPath);
> +  ScanSubdirectories(utf8Path, wrappedParameters.get(), false);

Rescanning the directories looks race-condition-ish.
I would prefer if you scanned directly the hashtables to find the subdirectories and unregistered them.

@@ +815,5 @@
> +             "watches manually before quitting.");
> +
> +  // Log any pending watch.
> +  (void)mWatchedResourcesByHandle.EnumerateRead(
> +    &WatchedPathsInfoHashtableTraverser, nullptr);

`WatchedPathsInfoHashtableTraverser` is not a very clear name. Why does it traverse the table?

Also, why is this before `if (mShuttingDown)`?

@@ +823,5 @@
> +  if (mShuttingDown) {
> +    // If this happens, we are in a strange situation.
> +    FILEWATCHERLOG(
> +      "NativeFileWatcherService::DeactivateRunnableMethod - We are already shutting down.");
> +    MOZ_CRASH();

Nit: MOZ_CRASH with a message, for clarity.

@@ +869,5 @@
> +  // Retrieve the change callbacks array.
> +  ChangeCallbackArray* changeCallbackArray =
> +    mChangeCallbacksTable.Get(aResourceDescriptor->mPath);
> +
> +  // This should always be valid.

Could we have a race condition here, whereby we have removed all callbacks for `mPath` just before we dispatched this callback? If not, can you document this?

@@ +1121,5 @@
> +  if (mShuttingDown) {
> +    return NS_OK;
> +  }
> +
> +  // Check to see which resource aMonitor is watching.

Isn't that `wrappedParameters->mEmittingMonitor`?

@@ +1175,5 @@
> +  // the subdirectories.
> +  nsMainThreadPtrHandle<nsINativeFileWatcherSuccessCallback> successCallbackPlaceholder;
> +
> +  AutoCloseFile resHandle(g_file_new_for_path(aParentPath.get()));
> +  MOZ_ASSERT(resHandle, "g_file_new_for_path should always return an handle.");

What if there is a race condition and the file is removed while we're recursing?
In that case, I believe that we should cancel the ScanSubdirectories, remove all the stuff that was added to the hash tables.

@@ +1213,5 @@
> +        (void)RemovePathRunnableMethod(subDirWrappedCallbacks.release());
> +      }
> +
> +      // Free the allocated resources.
> +      g_free(dirPath);

Could you make this a UniquePtr?

@@ +1214,5 @@
> +      }
> +
> +      // Free the allocated resources.
> +      g_free(dirPath);
> +      g_object_unref(dirInfo);

Could you make this a UniquePtr?

@@ +1218,5 @@
> +      g_object_unref(dirInfo);
> +    }
> +
> +    // Free the enumerator.
> +    g_object_unref(subdirEnumerator);

Could you make this a UniquePtr?

@@ +1236,5 @@
> +{
> +  // The next function always returns a valid file handle, even in case of errors.
> +  nsCString utf8Path = NS_ConvertUTF16toUTF8(aWrappedParameters->mChangedResource);
> +  AutoCloseFile changedHandle(g_file_new_for_path(utf8Path.get()));
> +  MOZ_ASSERT(changedHandle, "g_file_new_for_path should always return an handle.");

What if it has been removed already?

@@ +1251,5 @@
> +  if (!parentDescriptor) {
> +    // That's strange. No parent watch?
> +    FILEWATCHERLOG(
> +      "NativeFileWatcherService::HandleCreation - No parent watch for %S.",
> +      aWrappedParameters->mChangedResource.get());

If we reach this branch, I suspect that we have a race condition. Perhaps we should MOZ_CRASH?

@@ +1254,5 @@
> +      "NativeFileWatcherService::HandleCreation - No parent watch for %S.",
> +      aWrappedParameters->mChangedResource.get());
> +    return;
> +  }
> +

WIP I stopped here.

::: toolkit/components/filewatcher/NativeFileWatcherLinux.h
@@ +33,5 @@
> +
> +  // This is dispatched by the static |ChangeCallbackProxy| defined in the cpp
> +  // file and, for this reason, needs to be public. If we declared
> +  // |ChangeCallbackProxy| as friend of this class, we would need to import
> +  // GLIB IO headers here.

An explanation of what it does would be nice.

@@ +62,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>.
> +  nsClassHashtable<nsStringHashKey, ChangeCallbackArray> mChangeCallbacksTable;
> +  nsClassHashtable<nsStringHashKey, ErrorCallbackArray> mErrorCallbacksTable;

Remind me, why aren't we doing
nsClassHashtable<nsStringHashKey, BothCallbacksArray> mCallbacks;
?

I suspect that this would be a bit less fragile if we attempt to add twice the same callback, then remove it twice.

::: toolkit/components/filewatcher/moz.build
@@ +10,5 @@
>      EXPORTS += ['NativeFileWatcherWin.h']
>      UNIFIED_SOURCES += [
>          'NativeFileWatcherWin.cpp',
>      ]
> +elif CONFIG['OS_TARGET'] == 'Linux' and CONFIG['MOZ_WIDGET_GTK']:

That should work for targets other than Linux.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug_992896.patch

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #3)
> @@ +635,5 @@
> > +  mWatchedResourcesByHandle.Put(resMonitor, resource);
> > +
> > +  // Connect the result callback function.
> > +  int connectionId =
> > +    g_signal_connect(resMonitor, "changed", G_CALLBACK (ChangeNotificationProxy), this);
> 
> Can you point me to the manual that defines `changed`?

Sure: https://developer.gnome.org/gio/stable/GFileMonitor.html#GFileMonitor-changed

I've also made it clear in the comments!

> @@ +652,5 @@
> > +    }
> > +  }
> > +
> > +  // Watch subdirectories to watch as well.
> > +  ScanSubdirectories(utf8Path, wrappedParameters.get(), true);
> 
> Note for later: I have come to suspect that we will eventually want to be
> able to have both a version that watches with subdirectories and one that
> doesn't. Nothing to do yet, but let's keep it in mind.

It *should* be as easy as providing an argument in the AddPath to tell the watcher to recurse or not.

> @@ +1175,5 @@
> > +  // the subdirectories.
> > +  nsMainThreadPtrHandle<nsINativeFileWatcherSuccessCallback> successCallbackPlaceholder;
> > +
> > +  AutoCloseFile resHandle(g_file_new_for_path(aParentPath.get()));
> > +  MOZ_ASSERT(resHandle, "g_file_new_for_path should always return an handle.");
> 
> What if there is a race condition and the file is removed while we're
> recursing?
> In that case, I believe that we should cancel the ScanSubdirectories, remove
> all the stuff that was added to the hash tables.

As per our IRC talk, I think that now that this function can fail, all the stuff should be automatically removed when receiving the "delete" change event from the parent folder.

> 
> @@ +62,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>.
> > +  nsClassHashtable<nsStringHashKey, ChangeCallbackArray> mChangeCallbacksTable;
> > +  nsClassHashtable<nsStringHashKey, ErrorCallbackArray> mErrorCallbacksTable;
> 
> Remind me, why aren't we doing
> nsClassHashtable<nsStringHashKey, BothCallbacksArray> mCallbacks;
> ?
> 
> I suspect that this would be a bit less fragile if we attempt to add twice
> the same callback, then remove it twice.

I'm going to change that in the next iteration. If we're going to do this for the GIO/GTK version, I guess this should be done for the Windows version as well!

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Unfortunately, I won't have time to look at it before January.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8537903 [details] [diff] [review]
bug_992896.patch

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

Sorry it took so long.

By the way, in the future, let's use MozReview :)
https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

::: toolkit/components/filewatcher/NativeFileWatcherGIO.cpp
@@ +25,5 @@
> + **/
> +struct NativeFileWatcherService::ChangeRunnableParametersWrapper {
> +  nsString mChangedResource;
> +  GFileMonitor* mEmittingMonitor;
> +  GFileMonitorEvent mChangeEvent;

Nit: We generally put the methods and constructors first, the fields later.

@@ +182,5 @@
> +
> +/**
> + * AutoFreeString is a RAII wrapper for GLib |gchar| arrays.
> + */
> +struct AutoFreeStringPolicy

Nit: `GString` might be clearer here and below (although it lends itself to puns).

@@ +238,5 @@
> + * @param aUserData
> + *        |NativeFileWatcherService| instance pointer passed by |g_signal_connect|.
> + */
> +static void
> +ChangeNotificationProxy(GFileMonitor* aMonitor, GFile* aFirstFile, GFile* aSecondFile,

Don't forget to check if defining but not using `aSecondFile` causes warnings.

@@ +410,5 @@
> +      changeCallbackHandle,
> +      errorCallbackHandle,
> +      successCallbackHandle));
> +
> +  // Since this function does a bit of I/O stuff , run it in the IO thread.

Nit: Actually, with the recursive walking, it's more than "a bit".

@@ +575,5 @@
> +  GError *geError = nullptr;
> +  GFileMonitor* resMonitor =
> +    g_file_monitor(resHandle.get(), G_FILE_MONITOR_NONE, NULL, &geError);
> +  if (!resMonitor) {
> +    // Make |geError| is correctly handled when returning.

Nit: Check that sentence.

@@ +1237,5 @@
> +        successCallbackPlaceholder));
> +
> +    // If the the following function fails, arrors are reported by using the error callback
> +    // and the log file.
> +    nsresult rv = AddPathRunnableMethod(subDirWrappedCallbacks.release());

Unless I'm wrong, you are not waiting until you have finished scanning the subdirectories to dispatch the oncomplete notification, right? If so, that's a bad idea.

@@ +1286,5 @@
> +        successCallbackPlaceholder));
> +
> +    // We ignore the return value of the function below. The errors
> +    // are reported both using the error callbacks and the log.
> +    (void)RemovePathRunnableMethod(subDirWrappedCallbacks.release());

I hear that the modern way to ignore return values is
`unused << RemovePathRunnableMethod(...)`

@@ +1340,5 @@
> +  // Wrap the path and the first change callback in the parent watch.
> +  UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> +    new PathRunnablesParametersWrapper(
> +      aWrappedParameters->mChangedResource,
> +      changeCallbackArray->ElementAt(0),

I'm lost here. What exactly are we doing with the first item of changeCallbackArray?

::: toolkit/components/filewatcher/moz.build
@@ +4,5 @@
>  # 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']
>  

By the way, could you see if it builds with
FAIL_ON_WARNINGS = True
?

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug_992896.patch - v3

Thank you for your time reviewing this, Yoric!

This patch moves AutoCloseMonitor out of the anonymous namespace, as it was making compilation fail with warning as errors.

It also fixes the nits pointed out by Yoric.

Comment 9

[Alessio Placitelli [:Dexter]]

> Sorry it took so long.
> 
> By the way, in the future, let's use MozReview :)
> https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.
> html

No problem for the delay! I tried to use MozReview, but I've got a few weird errors (probably due to using it on Windows). I'm dealing with them and I'll hopefully be able to submit next patches using MozReview! 

> @@ +182,5 @@
> > +
> > +/**
> > + * AutoFreeString is a RAII wrapper for GLib |gchar| arrays.
> > + */
> > +struct AutoFreeStringPolicy
> 
> Nit: `GString` might be clearer here and below (although it lends itself to
> puns).

Are you suggesting to use |GString| instead of |AutoFreeStringPolicy|/|AutoFreeString|?

> @@ +238,5 @@
> > + * @param aUserData
> > + *        |NativeFileWatcherService| instance pointer passed by |g_signal_connect|.
> > + */
> > +static void
> > +ChangeNotificationProxy(GFileMonitor* aMonitor, GFile* aFirstFile, GFile* aSecondFile,
> 
> Don't forget to check if defining but not using `aSecondFile` causes
> warnings.

Just did, does not cause any warning here.

> @@ +1237,5 @@
> > +        successCallbackPlaceholder));
> > +
> > +    // If the the following function fails, arrors are reported by using the error callback
> > +    // and the log file.
> > +    nsresult rv = AddPathRunnableMethod(subDirWrappedCallbacks.release());
> 
> Unless I'm wrong, you are not waiting until you have finished scanning the
> subdirectories to dispatch the oncomplete notification, right? If so, that's
> a bad idea.

The oncomplete notification is dispatched by |AddPathRunnableMethod| on line 653, after |WatchSubdirectories| completes. The latter feeds |AddPathRunnableMethod|s for discovered subdirectories with an empty success callback, so that no on complete notification takes place before every subdirectory is scanned, since |WatchSubdirectories| is blocking.

Do you feel like I could improve commenting to make this clearer?

> @@ +1340,5 @@
> > +  // Wrap the path and the first change callback in the parent watch.
> > +  UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> > +    new PathRunnablesParametersWrapper(
> > +      aWrappedParameters->mChangedResource,
> > +      changeCallbackArray->ElementAt(0),
> 
> I'm lost here. What exactly are we doing with the first item of
> changeCallbackArray?

We are using it as a change callback for the newly created watch. We require at least a change callback in |AddPathRunnableMethod|, so I pass one in and add the other as soon as the watch gets created.

> ::: toolkit/components/filewatcher/moz.build
> @@ +4,5 @@
> >  # 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']
> >  
> 
> By the way, could you see if it builds with
> FAIL_ON_WARNINGS = True
> ?

Thanks for suggesting this, it was failing for the following warning:

"NativeFileWatcherGIO.cpp:280:34: warning: ‘mozilla::NativeFileWatcherService::WatchedResourceDescriptor’ has a field ‘mozilla::NativeFileWatcherService::WatchedResourceDescriptor::mResourceHandle’ whose type uses the anonymous namespace [enabled by default]
 struct NativeFileWatcherService::WatchedResourceDescriptor {"

I moved AutoCloseMonitor out of the anonymous namespace to fix this.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Sorry for the delay, I was sick.

(In reply to Alessio Placitelli [:Dexter] from comment #9)
> > @@ +182,5 @@
> > > +
> > > +/**
> > > + * AutoFreeString is a RAII wrapper for GLib |gchar| arrays.
> > > + */
> > > +struct AutoFreeStringPolicy
> > 
> > Nit: `GString` might be clearer here and below (although it lends itself to
> > puns).
> 
> Are you suggesting to use |GString| instead of
> |AutoFreeStringPolicy|/|AutoFreeString|?

I meant `AutoGString`/`AutoGStringPolicy`.

> > Unless I'm wrong, you are not waiting until you have finished scanning the
> > subdirectories to dispatch the oncomplete notification, right? If so, that's
> > a bad idea.
> 
> The oncomplete notification is dispatched by |AddPathRunnableMethod| on line
> 653, after |WatchSubdirectories| completes. The latter feeds
> |AddPathRunnableMethod|s for discovered subdirectories with an empty success
> callback, so that no on complete notification takes place before every
> subdirectory is scanned, since |WatchSubdirectories| is blocking.
> 
> Do you feel like I could improve commenting to make this clearer?

Yes, that would help, please.

> 
> > @@ +1340,5 @@
> > > +  // Wrap the path and the first change callback in the parent watch.
> > > +  UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> > > +    new PathRunnablesParametersWrapper(
> > > +      aWrappedParameters->mChangedResource,
> > > +      changeCallbackArray->ElementAt(0),
> > 
> > I'm lost here. What exactly are we doing with the first item of
> > changeCallbackArray?
> 
> We are using it as a change callback for the newly created watch. We require
> at least a change callback in |AddPathRunnableMethod|, so I pass one in and
> add the other as soon as the watch gets created.
> 

Could you find a way to make this clearer?

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: bug_992896.patch - v4

Sorry for the huge delay!

Changes:
- Renamed AutoFreeString* to AutoGString*
- Made it clearer in the comments why we're waiting until subdirectories scan is complete before dispatching  the oncomplete notification
- Made it clearer why we are sending the first item of changeCallbackArray in WatchSubdirectories.
- This patch was bitrotting, so I've cleaned it up a little (removed MOZ_FINAL, MOZ_OVERRIDE)

Comment 12

[noitidart]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Created attachment 8583089 [details] [diff] [review]
> bug_992896.patch - v4
> 
> Sorry for the huge delay!
> 
> Changes:
> - Renamed AutoFreeString* to AutoGString*
> - Made it clearer in the comments why we're waiting until subdirectories
> scan is complete before dispatching  the oncomplete notification
> - Made it clearer why we are sending the first item of changeCallbackArray
> in WatchSubdirectories.
> - This patch was bitrotting, so I've cleaned it up a little (removed
> MOZ_FINAL, MOZ_OVERRIDE)

Thanks Alessio!! :)

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8583089 [details] [diff] [review]
bug_992896.patch - v4

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

For this review, I have only looked at initialization and how to add a path.
Tell me if you want me to look at the rest immediately or if you would prefer I wait until you have addressed my comments.

::: toolkit/components/filewatcher/NativeFileWatcherGIO.cpp
@@ +34,5 @@
> +    , mChangeEvent(aChangeEvent)
> +  {
> +  }
> +
> +  nsString mChangedResource;

Nit: Comment that it's a full path.

@@ +45,5 @@
> +
> +/**
> + * An event used to notify the main thread when an error happens.
> + */
> +class WatchedErrorEvent final : public nsRunnable

Ah, thanks for updating `final`.

@@ +56,5 @@
> +   */
> +  WatchedErrorEvent(const nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback>& aOnError,
> +                    const nsresult aError, const int osError)
> +    : mOnError(aOnError)
> +    , mError(aError)

You forgot to initialize `mOsError`. I think that FAIL_ON_WARNING would tell you about that.

@@ +71,5 @@
> +  }
> +
> + private:
> +  nsMainThreadPtrHandle<nsINativeFileWatcherErrorCallback> mOnError;
> +  nsresult mError;

Let's make `mError` and `mOsError` `const`, while we're at it. Same below.

@@ +154,5 @@
> +
> +#define FILEWATCHERLOG(...) PR_LOG(GetFileWatcherContextLog(), PR_LOG_DEBUG, (__VA_ARGS__))
> +
> +/**
> + * AutoFreeError is a RAII wrapper for GLib |GError| handles.

I'd call these `AutoGError` and `AutoGErrorPolicy`.

@@ +179,5 @@
> +typedef UniquePtr<gchar, AutoGStringPolicy> AutoGString;
> +
> +/**
> + * This runnable is dispatched to the main thread in order to safely
> + * shutdown the worker thread.

Nit: "See the shutdown protocol for more detail".

@@ +269,5 @@
> +  {
> +    g_object_unref(aObject);
> +  }
> +};
> +typedef UniquePtr<GFile, ObjectUnrefPolicy<GFile>> AutoCloseFile;

AutoCloseGFile, AutoCloseGMonitor

@@ +278,5 @@
> + * watched resource.
> + */
> +struct NativeFileWatcherService::WatchedResourceDescriptor {
> +  // The path on the file system of the watched resource.
> +  nsString mPath;

const?

@@ +298,5 @@
> + * A structure used to pass the callbacks to the AddPathRunnableMethod() and
> + * RemovePathRunnableMethod().
> + */
> +struct NativeFileWatcherService::PathRunnablesParametersWrapper {
> +  nsString mPath;

const?

@@ +344,5 @@
> +  if (!observerService) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  observerService->AddObserver(this, "xpcom-shutdown-threads", false);

nsresult rv =

@@ +572,5 @@
> +  // The next function always returns a valid file handle, even in case of errors.
> +  AutoCloseFile resHandle(g_file_new_for_path(utf8Path.get()));
> +  MOZ_ASSERT(resHandle, "g_file_new_for_path should always return an handle.");
> +
> +  GError *geError = nullptr;

Nit: `gerror` or `g_error` instead of `geError`?

@@ +574,5 @@
> +  MOZ_ASSERT(resHandle, "g_file_new_for_path should always return an handle.");
> +
> +  GError *geError = nullptr;
> +  GFileMonitor* resMonitor =
> +    g_file_monitor(resHandle.get(), G_FILE_MONITOR_NONE, NULL, &geError);

Nit: can you comment each non-trivial argument?
 g_file_monitor(/*handle*/ resHandle.get(), /*?*/ G_FILE_MONITOR_NONE, /* ? */ NULL, &gerror);

@@ +603,5 @@
> +    new WatchedResourceDescriptor(wrappedParameters->mPath, resMonitor));
> +
> +  // Append the callbacks to the hash tables. We do this now since
> +  // AddDirectoryToWatchList could use the error callback, but we
> +  // need to make sure to remove them if AddDirectoryToWatchList fails.

It would be nice to have a `AutoRemoveCallbackFromHashtable<T>` for this purpose. I suspect that you can have a single class for both tables.

@@ +615,5 @@
> +
> +  // Add the resource pointer to both indexes.
> +  WatchedResourceDescriptor* resource = resourceDesc.release();
> +  mWatchedResourcesByPath.Put(wrappedParameters->mPath, resource);
> +  mWatchedResourcesByHandle.Put(resMonitor, resource);

In case of error in this method past this point, I'm not sure that you release `resource`, or that you clean up `mWatchedResourcesByPath` or `mWatchedResourcesByHandle`.

I suspect that a `AutoRemoveWatchedResource` could be useful to handle errors cleanly.

@@ +621,5 @@
> +  // Connect the callback function to the "changed" as defined in
> +  // https://developer.gnome.org/gio/stable/GFileMonitor.html#GFileMonitor-changed
> +  int connectionId =
> +    g_signal_connect(resMonitor, "changed", G_CALLBACK (ChangeNotificationProxy), this);
> +  if (connectionId <= 0) {

Haven't you forgotten to remove the callbacks from the hashtables?

@@ +1180,5 @@
> +nsresult
> +NativeFileWatcherService::WatchSubdirectories(
> +  const nsCString& aParentPath,
> +  const PathRunnablesParametersWrapper* aParentWrappedParameters,
> +  WatchedResourceDescriptor* aParentWatch)

You don't need to release `aParentWatch`, right?

@@ +1197,5 @@
> +  UniquePtr<GFileEnumerator, ObjectUnrefPolicy<GFileEnumerator>>
> +    subdirEnumerator(g_file_enumerate_children(resHandle.get(), "standard::*",
> +      G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, NULL, &geError));
> +  if (!subdirEnumerator) {
> +    // Make |geError| is correctly handled when returning.

Missing a word in that comment?

@@ +1213,5 @@
> +  while ((dirInfo = g_file_enumerator_next_file(subdirEnumerator.get(), NULL, NULL))) {
> +    // Make sure |dirInfo| is correctly disposed.
> +    UniquePtr<GFileInfo, ObjectUnrefPolicy<GFileInfo>> wrappedInfo(dirInfo);
> +
> +    // We are only interested to subdirectories.

"interested in"

@@ +1231,5 @@
> +    // Wrap the path and the callbacks in order to pass them using
> +    // NS_NewRunnableMethodWithArg.
> +    UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> +      new PathRunnablesParametersWrapper(
> +        utf16Path,

Would it be more efficient (in terms of number of lines of code and string conversions) to have `AddPathRunnableMethod` accept a `GString*`?

::: toolkit/components/filewatcher/NativeFileWatcherGIO.h
@@ +56,5 @@
> +  // resource when a change is detected.
> +  // The objects are not ref counted and get destroyed by mWatchedResourcesByPath
> +  // on NativeFileWatcherService::Destroy or in NativeFileWatcherService::RemovePath.
> +  nsClassHashtable<nsStringHashKey, WatchedResourceDescriptor> mWatchedResourcesByPath;
> +  nsDataHashtable<nsVoidPtrHashKey, WatchedResourceDescriptor*> mWatchedResourcesByHandle;

Nit: I'd slightly rewrite this as
  nsDataHashtable</*GFileMonitor*/nsVoidPtrHashKey, ...>

::: toolkit/components/filewatcher/moz.build
@@ +24,5 @@
>  XPIDL_MODULE = 'toolkit_filewatcher'
>  
>  XPIDL_SOURCES += [
>      'nsINativeFileWatcher.idl',
>  ]

Can you add
 FAIL_ON_WARNINGS = True

Comment 14

[noitidart]

This js-ctypes version of inotify works but needs cleaning up, and it wasnt really approached from an API standpoint, as add_path can add multiple paths, in mine and p0lip's case, this addon is just watching a single path and creates a new inotify instance for every path.



I'm not sure I'm happy with the way I declared name[] field in inotify_event structure. Also if you disable the addon, the polling doesn't shutdown it seems. I have to make sure to terminate() works, probably using the self.onclose event as mentioned in this topic here: https://bugzilla.mozilla.org/show_bug.cgi?id=1113060 due to these i put it in an experimental branch, i just feel its not right, so didnt merge it to master branch.

Anyways if you install this addon and it watches your desktop, any file actions that happen there, opening a folder, rename etc get logged to browser console.

Comment 15

[noitidart]

btw the read function hangs till a file change happens. the reason for the while loop is so it goes back and watches for next file change. otherwise after first file change it quits.

so this doesnt have a way to stop watching yet, but thats the point of making it spawn another worker, so we can send a message to myWorker.js to stop watching and it will terminate the poll

Comment 16

[noitidart]

oh also it needs a mechanism to "pass" a callback from mainthread to chromeworker to call whenever a filechange happens. we'll have to dig into the PromiseWorker for that.

Comment 17

[noitidart]

p0lip and i hooked up the API detailed here: https://bugzilla.mozilla.org/show_bug.cgi?id=958280#c1
additions to the API: callback is gets 3 arguments, aFileName, aEvent, aExtra
aEvent is a string of either: contents-modified, renamed, created, deleted

we did so without having to modify PromiseWorker.jsm

we connected this api today for linux/inotify support today. api logic is tested and ready, need fill in body functions for removePath and close

let watcher1 = new Watcher(callback);
watcher1.addPath(blah);



aExtra holds aFileNameOld when aEvent is renamed



pending work:
* need to figure out why some times events are missed
* need to figure out why sometimes it marks aEvent as renamed-from even though it was deleted
* need to figure out why sometimes when i rename it only triggers the rename-from and not the rename-to
* aExtra should have aOSPath_parentDir in it as dev users can add multiple paths to watch
* need to figure out if inotify only works on directories, if thats the case then addPath should fail if path is not a directory
* shutdown is not terminating poll workers, real weird
* if error in poll we dont want it to keep returning error, should quit after X errors and close Watcher

interesting discoveries:
* paths for inotify are case sensitive, if pass path of /home/noit/desktop it wont work, it has to be /home/noit/Desktop

ok on to the goods. to test, install this addon from this repo on linux, it will notify you (by logging to browser console) of changes on desktop: https://github.com/Noitidart/jscFileWatcher/tree/431407d216de531b1341dcc7489571d92391b69f

Comment 18

[noitidart]

Update on inotify work

* i think i resolved missed thanks to @p0lip, however i havent been able to find a situation where more then 1 notification is returned in a single read, so i havent had the chance to test that
  * i did find out though based on this topic: http://stackoverflow.com/questions/8334676/inotify-fails-to-react-on-in-delete that Ubuntu/Gnome, if you do a GUI delete (like hotkey or right click > delete) it notifies with IN_MOVED_FROM but never triggers the IN_MOVED_TO (unless you have added path to recycle bin im guessing) as its not deleting the file its moving it to recycle bin. so i havent implemented this yet as i need to discuss this, should we by default add path to recyle bin, and on IN_MOVED_TO if its recyle bin then the aEvent should be "recycled"? The other way to detect this other then making each Watcher watch recyle bin by default is, after the "IN_MOVED_FROM" notification triggers, then we can do a nsITimer/setTimeout for 1sec, and if a `IN_MOVED_TO` does not trigger for this then we can assume it was moved to recyle bin.

* Fixed so that 3rd argument of callback (aExtra) has pth to parent dir. aExtra.aOSPath_parentDir has the os path of parent dir
* Changed API in that on aEvent (second arg of callback) is `renamed` then all the old args of the `renamed-from` event are found in aExtra.aOld, so aExtra.aOld.aFileName will have the old file name. and aExtra.aOld.aExtra has all the old aExtra stuff. i deleted the aExtra.aOld.aEvent as that was `renamed-from` and we dont deliever that to the devuser
* Changed in that there is no more duplicate check based on case-insesitivity. By default the jscFileWatcher API is case-sensitive. We do a case-insens check only to through a warning to devuser that he may be adding a duplicate path, but no more basis on aOSPathLower, just on aOSPath (discussed with @p0lip before this change)

* still have to work on filling bodies of .close and .removePath; proper termination of workers on shutdown (code is written but not working); detect error in poll and quit

to test, can install this commit: https://github.com/Noitidart/jscFileWatcher/tree/ce71c9d1cd401144aecb2fca0bf444539804fdd6

Comment 19

[noitidart]

Actually that makes me think, we should probably support aEvent of `move-outof` and `moved-into`. And i cant just wait to see if a file was `move-outof` and assume it was moved to recyle bin as it could have been moved to anywhere else.

Comment 20

[noitidart]

Oh we know we're doing inotify but we'll do Gio:FileMonitor too, this is mainly for open source jsctypes and for those that want to support file watching on older version of Firefox. (like my need is to support from Firefox 29+, esr and those people that dont update in time). Also can give us a good perf test for jsctypes, when we compare to the C work done by dexter.

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: bug_992896.patch - v5

Comment 22

[Alessio Placitelli [:Dexter]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #13)
> Comment on attachment 8583089 [details] [diff] [review]
> bug_992896.patch - v4
> 
> Review of attachment 8583089 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for your time, Yoric.

> @@ +603,5 @@
> > +    new WatchedResourceDescriptor(wrappedParameters->mPath, resMonitor));
> > +
> > +  // Append the callbacks to the hash tables. We do this now since
> > +  // AddDirectoryToWatchList could use the error callback, but we
> > +  // need to make sure to remove them if AddDirectoryToWatchList fails.
> 
> It would be nice to have a `AutoRemoveCallbackFromHashtable<T>` for this
> purpose. I suspect that you can have a single class for both tables.
> 
> @@ +615,5 @@
> > +
> > +  // Add the resource pointer to both indexes.
> > +  WatchedResourceDescriptor* resource = resourceDesc.release();
> > +  mWatchedResourcesByPath.Put(wrappedParameters->mPath, resource);
> > +  mWatchedResourcesByHandle.Put(resMonitor, resource);
> 
> In case of error in this method past this point, I'm not sure that you
> release `resource`, or that you clean up `mWatchedResourcesByPath` or
> `mWatchedResourcesByHandle`.
> 
> I suspect that a `AutoRemoveWatchedResource` could be useful to handle
> errors cleanly.
>
> @@ +621,5 @@
> > +  // Connect the callback function to the "changed" as defined in
> > +  // https://developer.gnome.org/gio/stable/GFileMonitor.html#GFileMonitor-changed
> > +  int connectionId =
> > +    g_signal_connect(resMonitor, "changed", G_CALLBACK (ChangeNotificationProxy), this);
> > +  if (connectionId <= 0) {
> 
> Haven't you forgotten to remove the callbacks from the hashtables?

Good catch. The only case in which we should be removing the callbacks and freeing the |WatchedResourceDescriptor| is when |g_signal_connect| fails. Other cases handle the errors gently, by keeping on watching. That probably means we could do it without introducing AutoRemoveWatchedResource/AutoRemoveCallbackFromHashtable, which would only be used in this case. What do you think?

> @@ +1180,5 @@
> > +nsresult
> > +NativeFileWatcherService::WatchSubdirectories(
> > +  const nsCString& aParentPath,
> > +  const PathRunnablesParametersWrapper* aParentWrappedParameters,
> > +  WatchedResourceDescriptor* aParentWatch)
> 
> You don't need to release `aParentWatch`, right?

No, that's owned by the caller. I made this clearer in the comments.

> @@ +1231,5 @@
> > +    // Wrap the path and the callbacks in order to pass them using
> > +    // NS_NewRunnableMethodWithArg.
> > +    UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> > +      new PathRunnablesParametersWrapper(
> > +        utf16Path,
> 
> Would it be more efficient (in terms of number of lines of code and string
> conversions) to have `AddPathRunnableMethod` accept a `GString*`?

AddPathRunnableMethod also makes use of non GString strings. Wouldn't we have the same problem the other way around?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

(In reply to Alessio Placitelli [:Dexter] from comment #22)
> Good catch. The only case in which we should be removing the callbacks and
> freeing the |WatchedResourceDescriptor| is when |g_signal_connect| fails.
> Other cases handle the errors gently, by keeping on watching. That probably
> means we could do it without introducing
> AutoRemoveWatchedResource/AutoRemoveCallbackFromHashtable, which would only
> be used in this case. What do you think?

I'd say "let's add them anyway", because what we really want is `finally` but C++ still doesn't have it :/

> > @@ +1231,5 @@
> > > +    // Wrap the path and the callbacks in order to pass them using
> > > +    // NS_NewRunnableMethodWithArg.
> > > +    UniquePtr<PathRunnablesParametersWrapper> subDirWrappedCallbacks(
> > > +      new PathRunnablesParametersWrapper(
> > > +        utf16Path,
> > 
> > Would it be more efficient (in terms of number of lines of code and string
> > conversions) to have `AddPathRunnableMethod` accept a `GString*`?
> 
> AddPathRunnableMethod also makes use of non GString strings. Wouldn't we
> have the same problem the other way around?

Fair enough.

Comment 24

[noitidart]

Hey guys question on this. We had an issue with kqueue (no notification on watched directory when file inside is overwritten in some cases) and are having to move to Gio. We leave kqueue though in case we ever need iOS support, but devuser has to watch a specific file if they want to get contents-modified notification.

But about gio, is this a watch per file (like winnt)? Or is a watch and then add paths to it (like kqueue, FSEvents, inotify)?

Comment 25

[Alessio Placitelli [:Dexter]]

Unassigning as I don't time, at the moment, to get back to it.

Comment 26

[Barret Rennie [:barret] (they/them)]

Mass closure: OSFIle is being replaced with IOUtils and PathUtils. If you think this bug was closed in error, please re-open.