Move all run-loop calls into the run-loop thread, and use correct memory handling (retain/release).

runtime/bin/file_system_watcher_macos.cc

Index: runtime/bin/file_system_watcher_macos.cc
diff --git a/runtime/bin/file_system_watcher_macos.cc b/runtime/bin/file_system_watcher_macos.cc
index 2fa80a29ba9efd692fad312dbfbe731dcd7ac1fa..be3e858c2c28c8d5053c2a6ea8c954b043f6d5ae 100644
--- a/runtime/bin/file_system_watcher_macos.cc
+++ b/runtime/bin/file_system_watcher_macos.cc
@@ -55,48 +55,94 @@ class FSEventsWatcher {
  public:
   class Node {
    public:
-    Node(FSEventsWatcher* watcher, intptr_t base_path_length, int read_fd,
+    Node(FSEventsWatcher* watcher, char* base_path, int read_fd,
          int write_fd, bool recursive)
         : watcher_(watcher),
           ready_(false),
-          base_path_length_(base_path_length),
+          base_path_length_(strlen(base_path)),
+          path_ref_(CFStringCreateWithCString(
+              NULL, base_path, kCFStringEncodingUTF8)),
           read_fd_(read_fd),
           write_fd_(write_fd),
           recursive_(recursive),
-          ref_(NULL) {}
+          ref_(NULL) {
+      Start();
+    }
 
     ~Node() {
+      Stop();
       close(write_fd_);
+      CFRelease(path_ref_);
     }
 
     void set_ref(FSEventStreamRef ref) {
       ref_ = ref;
     }
 
+    void Start() {
+      // Schedule StartCallback to be executed in the RunLoop.
+      CFRunLoopTimerContext context;
+      memset(&context, 0, sizeof(context));
+      context.info = this;
+      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
+          NULL, 0, 0, 0, 0, Node::StartCallback, &context);
+      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
+      CFRelease(timer);
+      watcher_->monitor_.Enter();
+      while (!ready_) {
+        watcher_->monitor_.Wait(Monitor::kNoTimeout);
+      }
+      watcher_->monitor_.Exit();
+    }
+
     static void StartCallback(CFRunLoopTimerRef timer, void* info) {
       Node* node = reinterpret_cast<Node*>(info);
+#ifdef DEBUG
+      ASSERT(node->watcher_->threadId_ == Thread::GetCurrentThreadId());
+#endif
+      FSEventStreamContext context;
+      memset(&context, 0, sizeof(context));
+      context.info = reinterpret_cast<void*>(node);
+      CFArrayRef array = CFArrayCreate(
+          NULL, reinterpret_cast<const void**>(&node->path_ref_), 1, NULL);
+      FSEventStreamRef ref = FSEventStreamCreate(
+          NULL,
+          Callback,
+          &context,
+          array,
+          kFSEventStreamEventIdSinceNow,
+          0.10,
+          kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents);
+      CFRelease(array);
+
+      node->set_ref(ref);
+
       FSEventStreamScheduleWithRunLoop(
           node->ref_,
           node->watcher_->run_loop_,
           kCFRunLoopDefaultMode);
+
       FSEventStreamStart(node->ref_);
       FSEventStreamFlushSync(node->ref_);
+
       node->watcher_->monitor_.Enter();
       node->ready_ = true;
       node->watcher_->monitor_.Notify();
       node->watcher_->monitor_.Exit();
     }
 
-    void Start() {
-      ASSERT(!ready_);
+    void Stop() {
+      // Schedule StopCallback to be executed in the RunLoop.
+      ASSERT(ready_);
       CFRunLoopTimerContext context;
       memset(&context, 0, sizeof(context));
       context.info = this;
       CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
-          NULL, 0, 0, 0, 0, StartCallback, &context);
+          NULL, 0, 0, 0, 0, StopCallback, &context);
       CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
+      CFRelease(timer);
       watcher_->monitor_.Enter();
-      while (!ready_) {
+      while (ready_) {
         watcher_->monitor_.Wait(Monitor::kNoTimeout);
       }
       watcher_->monitor_.Exit();
@@ -104,6 +150,9 @@ class FSEventsWatcher {
 
     static void StopCallback(CFRunLoopTimerRef timer, void* info) {
       Node* node = reinterpret_cast<Node*>(info);
+#ifdef DEBUG
+      ASSERT(node->watcher_->threadId_ == Thread::GetCurrentThreadId());
+#endif
       FSEventStreamStop(node->ref_);
       FSEventStreamInvalidate(node->ref_);
       FSEventStreamRelease(node->ref_);
@@ -113,21 +162,7 @@ class FSEventsWatcher {
       node->watcher_->monitor_.Exit();
     }
 
-    void Stop() {
-      ASSERT(ready_);
-      CFRunLoopTimerContext context;
-      memset(&context, 0, sizeof(context));
-      context.info = this;
-      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
-          NULL, 0, 0, 0, 0, StopCallback, &context);
-      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
-      watcher_->monitor_.Enter();
-      while (ready_) {
-        watcher_->monitor_.Wait(Monitor::kNoTimeout);
-      }
-      watcher_->monitor_.Exit();
-    }
-
+    FSEventsWatcher* watcher() const { return watcher_; }
     bool ready() const { return ready_; }
     intptr_t base_path_length() const { return base_path_length_; }
     int read_fd() const { return read_fd_; }
@@ -138,31 +173,34 @@ class FSEventsWatcher {
     FSEventsWatcher* watcher_;
     bool ready_;
     intptr_t base_path_length_;
+    CFStringRef path_ref_;
     int read_fd_;
     int write_fd_;
     bool recursive_;
     FSEventStreamRef ref_;
   };
 
-  FSEventsWatcher() : run_loop_(0) {
-    Thread::Start(Run, reinterpret_cast<uword>(this));
-  }
 
-  ~FSEventsWatcher() {
-    CFRunLoopStop(run_loop_);
+  FSEventsWatcher() : run_loop_(0) {
+    Start();
   }
 
-  Monitor& monitor() { return monitor_; }
-
-  bool has_run_loop() const { return run_loop_ != NULL; }
-
-  static void TimerCallback(CFRunLoopTimerRef timer, void* context) {
-    // Dummy callback to keep RunLoop alive.
+  void Start() {
+    Thread::Start(Run, reinterpret_cast<uword>(this));
+    monitor_.Enter();
+    while (run_loop_ == NULL) {
+      monitor_.Wait(Monitor::kNoTimeout);
+    }
+    monitor_.Exit();
   }
 
   static void Run(uword arg) {
     FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(arg);
+#ifdef DEBUG
+    watcher->threadId_ = Thread::GetCurrentThreadId();
+#endif
     watcher->run_loop_ = CFRunLoopGetCurrent();
+    CFRetain(watcher->run_loop_);
 
     // Notify, as the run-loop is set.
     watcher->monitor().Enter();
@@ -177,10 +215,52 @@ class FSEventsWatcher {
         0,
         TimerCallback,
         NULL);
-
     CFRunLoopAddTimer(watcher->run_loop_, timer, kCFRunLoopCommonModes);
+    CFRelease(timer);
 
     CFRunLoopRun();
+
+    CFRelease(watcher->run_loop_);
+    watcher->monitor_.Enter();
+    watcher->run_loop_ = NULL;
+    watcher->monitor_.Notify();
+    watcher->monitor_.Exit();
+  }
+
+  void Stop() {
+    // Schedule StopCallback to be executed in the RunLoop.
+    CFRunLoopTimerContext context;
+    memset(&context, 0, sizeof(context));
+    context.info = this;
+    CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
+        NULL, 0, 0, 0, 0, StopCallback, &context);
+    CFRunLoopAddTimer(run_loop_, timer, kCFRunLoopCommonModes);
+    CFRelease(timer);
+    monitor_.Enter();
+    while (run_loop_ != NULL) {
+      monitor_.Wait(Monitor::kNoTimeout);
+    }
+    monitor_.Exit();
+  }
+
+  static void StopCallback(CFRunLoopTimerRef timer, void* info) {
+    FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(info);
+#ifdef DEBUG
+    ASSERT(watcher->threadId_ == Thread::GetCurrentThreadId());
+#endif
+    CFRunLoopStop(watcher->run_loop_);
+  }
+
+  ~FSEventsWatcher() {
+    Stop();
+  }
+
+  Monitor& monitor() { return monitor_; }
+
+  bool has_run_loop() const { return run_loop_ != NULL; }
+
+  static void TimerCallback(CFRunLoopTimerRef timer, void* context) {
+    // Dummy callback to keep RunLoop alive.
   }
 
   Node* AddPath(const char* path, int events, bool recursive) {
@@ -191,29 +271,8 @@ class FSEventsWatcher {
 
     char base_path[PATH_MAX];
     realpath(path, base_path);
-    CFStringRef path_ref = CFStringCreateWithCString(
-        NULL, base_path, kCFStringEncodingUTF8);
-
-    Node* node = new Node(this, strlen(base_path), fds[0], fds[1], recursive);
-
-    FSEventStreamContext context;
-    context.version = 0;
-    context.info = reinterpret_cast<void*>(node);
-    context.retain = NULL;
-    context.release = NULL;
-    context.copyDescription = NULL;
-    FSEventStreamRef ref = FSEventStreamCreate(
-        NULL,
-        Callback,
-        &context,
-        CFArrayCreate(NULL, reinterpret_cast<const void**>(&path_ref), 1, NULL),
-        kFSEventStreamEventIdSinceNow,
-        0.10,
-        kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents);
 
-    node->set_ref(ref);
-
-    return node;
+    return new Node(this, base_path, fds[0], fds[1], recursive);
   }
 
  private:
@@ -224,6 +283,9 @@ class FSEventsWatcher {
                        const FSEventStreamEventFlags event_flags[],
                        const FSEventStreamEventId event_ids[]) {
     Node* node = reinterpret_cast<Node*>(client);
+#ifdef DEBUG
+    ASSERT(node->watcher()->threadId_ == Thread::GetCurrentThreadId());
+#endif
     // `ready` is set on same thread as this callback is invoked, so we don't
     // need to lock here.
     if (!node->ready()) return;
@@ -243,6 +305,9 @@ class FSEventsWatcher {
 
   Monitor monitor_;
   CFRunLoopRef run_loop_;
+#ifdef DEBUG

[Søren Gjesse, 2014/02/07 13:03:25]

Please get rid of all the #ifdef/#endif, and add a comment saying that threadId_
is only used in debug mode.

[Anders Johnsen, 2014/02/07 13:16:11]

Done.

+  ThreadId threadId_;
+#endif
 };
 
 
@@ -253,19 +318,12 @@ bool FileSystemWatcher::IsSupported() {
 
 
 intptr_t FileSystemWatcher::Init() {
-  FSEventsWatcher* watcher = new FSEventsWatcher();
-  watcher->monitor().Enter();
-  while (!watcher->has_run_loop()) {
-    watcher->monitor().Wait(Monitor::kNoTimeout);
-  }
-  watcher->monitor().Exit();
-  return reinterpret_cast<intptr_t>(watcher);
+  return reinterpret_cast<intptr_t>(new FSEventsWatcher());
 }
 
 
 void FileSystemWatcher::Close(intptr_t id) {
-  FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(id);
-  delete watcher;
+  delete reinterpret_cast<FSEventsWatcher*>(id);
 }
 
 
@@ -274,18 +332,13 @@ intptr_t FileSystemWatcher::WatchPath(intptr_t id,
                                       int events,
                                       bool recursive) {
   FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(id);
-  FSEventsWatcher::Node* node = watcher->AddPath(path, events, recursive);
-  node->Start();
-  return reinterpret_cast<intptr_t>(node);
+  return reinterpret_cast<intptr_t>(watcher->AddPath(path, events, recursive));
 }
 
 
 void FileSystemWatcher::UnwatchPath(intptr_t id, intptr_t path_id) {
   USE(id);
-  FSEventsWatcher::Node* node =
-      reinterpret_cast<FSEventsWatcher::Node*>(path_id);
-  node->Stop();
-  delete node;
+  delete reinterpret_cast<FSEventsWatcher::Node*>(path_id);
 }