[Sync] Signal UIModelWorker to abort on sync shutdown.

components/sync/driver/glue/ui_model_worker.cc

 // 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.
 
 #include "components/sync/driver/glue/ui_model_worker.h"
 
 #include <utility>
 
 #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 {
 
 namespace {
 
 void CallDoWorkAndSignalEvent(const WorkCallback& work,
                               syncer::ScopedEventSignal scoped_event_signal,
                               SyncerError* error_info) {
   *error_info = work.Run();
   // The event in |scoped_event_signal| is signaled at the end of this scope.
 }
 
 }  // 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) {
   SyncerError error_info;
   if (ui_thread_->BelongsToCurrentThread()) {
     DLOG(WARNING) << "DoWorkAndWaitUntilDone called from "
                   << "ui_loop_. Probably a nested invocation?";
     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;
 }
 
 bool UIModelWorker::IsOnModelThread() {
   return ui_thread_->BelongsToCurrentThread();
 }
 
 UIModelWorker::~UIModelWorker() {}
 
 }  // namespace syncer