[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_SYNC_DRIVER_GLUE_UI_MODEL_WORKER_H_
 #define COMPONENTS_SYNC_DRIVER_GLUE_UI_MODEL_WORKER_H_
 
 #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 {
 
 // A ModelSafeWorker for UI models (e.g. bookmarks) that
 // accepts work requests from the syncapi that need to be fulfilled
 // from the MessageLoop home to the native model.
 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;
 
  protected:
   SyncerError DoWorkAndWaitUntilDoneImpl(const WorkCallback& work) override;
 
  private:
   ~UIModelWorker() override;
 
   // 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);
 };
 
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_DRIVER_GLUE_UI_MODEL_WORKER_H_