[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker.cc

Index: components/sync/driver/glue/ui_model_worker.cc
diff --git a/components/sync/driver/glue/ui_model_worker.cc b/components/sync/driver/glue/ui_model_worker.cc
index c812f9a9b54ed6b95cf0c974a3d07f778f2eac48..6afb5c78d6bfbaeb4f1da7e52d5ed0c9477f1151 100644
--- a/components/sync/driver/glue/ui_model_worker.cc
+++ b/components/sync/driver/glue/ui_model_worker.cc
@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
-#include "base/synchronization/waitable_event.h"
 #include "components/sync/base/scoped_event_signal.h"
 
 namespace syncer {
@@ -26,8 +25,11 @@ void CallDoWorkAndSignalEvent(const WorkCallback& work,
 }  // namespace
 
 UIModelWorker::UIModelWorker(
-    scoped_refptr<base::SingleThreadTaskRunner> ui_thread)
-    : ui_thread_(std::move(ui_thread)) {}
+    const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread)

[fdoray, 2016/11/11 15:40:28]

Pass scoped_refptr by value
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Relevant discussion:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/hDMBe...

[maxbogue, 2016/11/12 03:19:52]

Huh. We do that wrong all over sync code apparently. Fixed.

+    : ui_thread_(ui_thread),
+      work_done_or_abandoned_(base::WaitableEvent::ResetPolicy::AUTOMATIC,

[fdoray, 2016/11/11 15:40:28]

Should be base::WaitableEvent::ResetPolicy::MANUAL to permanently prevent
DoWorkAndWaitUntilDoneImpl() from blocking after RequestStop() is called.

[maxbogue, 2016/11/12 03:19:52]

Sorta right but for the wrong reason. Needs to be MANUAL so
CallDoWorkAndSignalEvent can abort early if it's been set, but it has to be
reset inside DoWorkAndWaitUntilDoneImpl before the post to be able to do work
more than once.

+                              base::WaitableEvent::InitialState::NOT_SIGNALED) {
+}
 
 SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl(
     const WorkCallback& work) {
@@ -38,26 +40,25 @@ SyncerError UIModelWorker::DoWorkAndWaitUntilDoneImpl(
     return work.Run();
   }
 
-  // Signaled when the task is deleted, i.e. after it runs or when it is
-  // abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
   if (!ui_thread_->PostTask(FROM_HERE,
                             base::Bind(&CallDoWorkAndSignalEvent, work,
                                        base::Passed(syncer::ScopedEventSignal(
-                                           &work_done_or_abandoned)),
+                                           &work_done_or_abandoned_)),
                                        &error_info))) {
     DLOG(WARNING) << "Could not post work to UI loop.";
     error_info = CANNOT_DO_WORK;
     return error_info;
   }
-  work_done_or_abandoned.Wait();
+  work_done_or_abandoned_.Wait();
 
   return error_info;
 }
 
+void UIModelWorker::RequestStop() {
+  ModelSafeWorker::RequestStop();
+  work_done_or_abandoned_.Signal();

[fdoray, 2016/11/11 15:40:28]

In sync_integration_tests, this is called before the main MessageLoop is done
running. That means that |work_done_or_abandonned_| is signaled and
DoWorkAndWaitUntilDoneImpl() returns when the WorkCallback still has a chance to
run. The WorkCallback accesses arguments which are no longer guaranteed to exist

https://cs.chromium.org/chromium/src/components/sync/engine_impl/directory_up...

Maybe we could change CallDoWorkAndSignalEvent() to check if the event is
already signaled before running the callback? (event would need to be
ResetPolicy::MANUAL)

[maxbogue, 2016/11/12 03:19:52]

Done.

+}
+
 ModelSafeGroup UIModelWorker::GetModelSafeGroup() {
   return GROUP_UI;
 }