[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker.h

Index: components/sync/driver/glue/ui_model_worker.h
diff --git a/components/sync/driver/glue/ui_model_worker.h b/components/sync/driver/glue/ui_model_worker.h
index c3f76946a1fe0fa0b0e44eab19723e1bc047d1e0..60676815ea164b6a97a5c291db1e92d42a9be118 100644
--- a/components/sync/driver/glue/ui_model_worker.h
+++ b/components/sync/driver/glue/ui_model_worker.h
@@ -7,7 +7,9 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "components/sync/engine/model_safe_worker.h"
 
 namespace syncer {
@@ -19,7 +21,8 @@ class UIModelWorker : public ModelSafeWorker {
  public:
   explicit UIModelWorker(scoped_refptr<base::SingleThreadTaskRunner> ui_thread);
 
-  // ModelSafeWorker implementation. Called on syncapi SyncerThread.
+  // ModelSafeWorker implementation.
+  void RequestStop() override;
   ModelSafeGroup GetModelSafeGroup() override;
   bool IsOnModelThread() override;
 
@@ -32,6 +35,20 @@ class UIModelWorker : public ModelSafeWorker {
   // A reference to the UI thread's task runner.
   const scoped_refptr<base::SingleThreadTaskRunner> ui_thread_;
 
+  // Signaled when a task posted by DoWorkAndWaitUntilDoneImpl() is deleted,
+  // i.e. after it runs or when it is abandoned. Reset at the beginning of every
+  // DoWorkAndWaitUntilDoneImpl() call.
+  base::WaitableEvent work_done_or_abandoned_;
+
+  // Signaled from RequestStop(). When this is signaled,
+  // DoWorkAndWaitUntilDoneImpl() returns immediately. This is needed to prevent
+  // the UI thread from joining the sync thread while it is waiting for a
+  // WorkCallback to run on the UI thread. See crbug.com/663600.
+  base::WaitableEvent stop_requested_;

[maxbogue, 2016/11/18 18:26:51]

Really this is necessary to prevent the race between the stopped signal being
set and the work_done_or_abandoned_ signal being reset, right? I'm trying to see
why this works better than checking IsStopped in CallDoWorkAndSignalEvent did.

[fdoray, 2016/11/21 16:57:55]

With your CL:
Thread:  Action:
Sync     DoWorkAndWaitUntilDone()
           sees that the worker isn't stopped
UI       RequestStop()
           sets the stopped flag
           signals the event
Sync     DoWorkAndWaitUntilDoneImpl()
           resets the event
           posts the task
           waits for it forever

With this CL:
Thread:  Action:
Sync     DoWorkAndWaitUntilDone()
           sees that the worker isn't stopped
UI       RequestStop()
           sets the stopped flag
           signals |stop_requested_| - which is never reset
Sync     DoWorkAndWaitUntilDoneImpl()
           resets |work_done_or_abandoned_|
           posts the task
           WaitMany returns immediately because |stop_requested_| is signaled
  

[maxbogue, 2016/11/21 17:17:46]

Ah ok, so mine tried to make the posted task return immediately, but it might
never run or be destroyed before deadlocking, so not waiting for it at all works
better? Makes sense. Thanks!

[fdoray, 2016/11/21 19:06:11]

That's right.

+
+  // Verifies that calls to DoWorkAndWaitUntilDoneImpl() are sequenced.
+  base::SequenceChecker sequence_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(UIModelWorker);
 };