Remove call to GetBlockingPool in RLZ

rlz/lib/financial_ping.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.
 //
 // Library functions related to the Financial Server ping.
 
 #include "rlz/lib/financial_ping.h"
 
 #include <stdint.h>
 
 #include <memory>
 
 #include "base/atomicops.h"
 #include "base/location.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "base/single_thread_task_runner.h"
+#include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "rlz/lib/assert.h"
 #include "rlz/lib/lib_values.h"
 #include "rlz/lib/machine_id.h"
 #include "rlz/lib/rlz_lib.h"
 #include "rlz/lib/rlz_value_store.h"
 #include "rlz/lib/string_utils.h"
 
 #if !defined(OS_WIN)
@@ skipping 165 matching lines @@
 
 bool FinancialPing::SetURLRequestContext(
     net::URLRequestContextGetter* context) {
   base::subtle::Release_Store(
       &g_context, reinterpret_cast<AtomicWord>(context));
   return true;
 }
 
 namespace {
 
+class RefCountedWaitableEvent
+    : public base::RefCountedThreadSafe<RefCountedWaitableEvent> {
+ public:
+  RefCountedWaitableEvent()
+      : event_(base::WaitableEvent::ResetPolicy::MANUAL,
+               base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  void Signal() { event_.Signal(); }
+
+  bool TimedWait(const base::TimeDelta& timeout) {
+    return event_.TimedWait(timeout);
+  }
+
+ private:
+  ~RefCountedWaitableEvent() {}
+  friend class base::RefCountedThreadSafe<RefCountedWaitableEvent>;
+
+  base::WaitableEvent event_;
+};
+
 class FinancialPingUrlFetcherDelegate : public net::URLFetcherDelegate {
  public:
-  FinancialPingUrlFetcherDelegate(const base::Closure& callback)
-      : callback_(callback) {
+  FinancialPingUrlFetcherDelegate(scoped_refptr<RefCountedWaitableEvent> event)
+      : event_(event) {}

[gab, 2017/07/17 16:44:20]

std::move(event)

[Roger Tawa OOO till Jul 10th, 2017/07/17 19:19:37]

This happens exactly once per run of chrome.exe in a background thread.  Not
sure the optimization is worth it.

[gab, 2017/07/17 21:05:20]

It's just a good habit to have when taking scoped_refptr (or any moveable type
for that matter) by value as a parameter. Don't see why diverge here.

+
+  void OnURLFetchComplete(const net::URLFetcher* source) override {

[gab, 2017/07/17 16:44:20]

Does this work? url_fetch.h says:

// You may create the URLFetcher instance on any sequence; OnURLFetchComplete()
// will be called back on the same sequence you use to create the instance.

which tells me the blocking wait will prevent this sequence from processing this
notification and signal as the URLFetcher is created on the stack just before
the Wait().

What you could do to keep the API synchronous (blocking until done) while having
a responsive sequence for the URLFetcher to notify is to create a dedicated
sequence to create and start the URL fetcher.

That way you'd block your calling sequence and wait for a signal from the
processing sequence.

[Roger Tawa OOO till Jul 10th, 2017/07/17 19:19:36]

I think that comment in URLFetcher may no longer be completely accurate.  It
seems to date from the days when we had dedicated threads.  The behaviour as I
experience it now is that the sequence that calls Start() is the one that will
be used to run delegate.  Since Start is being posted to another sequence (see
line 369 below) there is no deadlock.  Also why your comment at line 369 about
calling Start() inline won't work.  I could add a comment to clarify this.

[gab, 2017/07/17 21:05:20]

Hmmmm, well it's definitely also not running on the sequence of base::PostTask()
because parallel tasks currently don't provide a sequence (i.e.
SequencedTaskRunnerHandle::Get() returns null) -- we're planning to fix that to
allow parallel tasks to be extended into a sequence should the task wish to grab
a reference to itself for continuations but that's WIP.

It seems it's running on URLRequestContextGetter::GetNetworkTaskRunner() which
I'm having trouble figuring out where that goes in this case.

[Roger Tawa OOO till Jul 10th, 2017/07/21 13:53:42]

Got it.  The main point is to not block this sequence, which is what happens
here.  The real requirements is for the delegate to run in "some other sequence"
but it does not matter which one.  If the other sequence changes in the future
then all is still fine.

[gab, 2017/07/21 22:39:17]

True, so long as that doesn't become SequencedTaskRunnerHandle::Get() as then
you'd hang (and since RLZ isn't really tested -- at least not on Chromium
waterfall -- this can result in disasters, right?).

I'm mostly leery because the documentation says explicitly that it happens on
the sequence the URLFetcher is created on... so that doesn't make this feel very
future proof...

Can you check with the owners how this API's documentation should be updated to
both be correct and future proof?

If you prefer to avoid that, I think the most correct approach is to post the
core of PingServer() on a different sequence (can be an inline
base::CreateSequencedTaskRunnerWithTraits()->PostTask()). So that PingServer()
essentially just becomes a Post+Wait. Then at least we're guaranteed that the
Wait() will never be blocking the very signal it's waiting on.

+    event_->Signal();
   }
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
 
  private:
-  base::Closure callback_;
+  scoped_refptr<RefCountedWaitableEvent> event_;
 };
 
-void FinancialPingUrlFetcherDelegate::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  callback_.Run();
-}
-
 bool send_financial_ping_interrupted_for_test = false;
 
 }  // namespace
 
-void ShutdownCheck(base::WeakPtr<base::RunLoop> weak) {
-  if (!weak.get())
-    return;
+void ShutdownCheck(scoped_refptr<RefCountedWaitableEvent> event) {
   if (!base::subtle::Acquire_Load(&g_context)) {
     send_financial_ping_interrupted_for_test = true;
-    weak->QuitClosure().Run();
+    event->Signal();
     return;
   }
   // How frequently the financial ping thread should check
   // the shutdown condition?
   const base::TimeDelta kInterval = base::TimeDelta::FromMilliseconds(500);
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::Bind(&ShutdownCheck, weak), kInterval);
+  base::PostDelayedTask(FROM_HERE, base::Bind(&ShutdownCheck, event),
+                        kInterval);
 }
 #endif
 
 bool FinancialPing::PingServer(const char* request, std::string* response) {
   if (!response)
     return false;
 
   response->clear();
 
 #if defined(RLZ_NETWORK_IMPLEMENTATION_WIN_INET)
@@ skipping 51 matching lines @@
   // in different thread. The instance is guaranteed to exist while
   // the method is running.
   net::URLRequestContextGetter* context =
       reinterpret_cast<net::URLRequestContextGetter*>(
           base::subtle::Acquire_Load(&g_context));
 
   // Browser shutdown will cause the context to be reset to NULL.
   if (!context)
     return false;
 
-  // Run a blocking event loop to match the win inet implementation.

[gab, 2017/07/17 16:44:20]

Should we keep the comment to say that this is "blocking to match the win inet
implementation"?

[Roger Tawa OOO till Jul 10th, 2017/07/17 19:19:36]

Done.

-  std::unique_ptr<base::MessageLoop> message_loop;
-  // Ensure that we have a MessageLoop.
-  if (!base::MessageLoop::current())
-    message_loop.reset(new base::MessageLoop);
-  base::RunLoop loop;
-  FinancialPingUrlFetcherDelegate delegate(loop.QuitClosure());
+  auto event = base::MakeRefCounted<RefCountedWaitableEvent>();
+  FinancialPingUrlFetcherDelegate delegate(event);
 
   std::string url = base::StringPrintf("http://%s:%d%s",
                                        kFinancialServer, kFinancialPort,
                                        request);
 
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("rlz_ping", R"(
         semantics {
           sender: "RLZ Ping"
           description:
@@ skipping 20 matching lines @@
 
   fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE |
                         net::LOAD_DO_NOT_SEND_AUTH_DATA |
                         net::LOAD_DO_NOT_SEND_COOKIES |
                         net::LOAD_DO_NOT_SAVE_COOKIES);
 
   // Ensure rlz_lib::SetURLRequestContext() has been called before sending
   // pings.
   fetcher->SetRequestContext(context);
 
-  base::WeakPtrFactory<base::RunLoop> weak(&loop);
+  base::PostTask(FROM_HERE, base::Bind(&ShutdownCheck, event));
+  base::PostTask(FROM_HERE, base::Bind(&net::URLFetcher::Start,

[gab, 2017/07/17 16:44:20]

net::URLFetcher::Start() seems to just PostTask itself:
https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=...

so I was going to say that this call probably still needed to happen on
SequencedTaskRunnerHandle::Get() (and that wouldn't work per the blocking wait)
but now I'm thinking you can probably just call it inline here.

[Roger Tawa OOO till Jul 10th, 2017/07/17 19:19:36]

No, see comment above.

[gab, 2017/07/17 21:05:20]

If the fetcher replies on another sequence, isn't the |fetcher->| usage below
racy in the event of a timeout which coincides with a late OnURLFetchComplete()
(and already was prior to this CL)?

[Roger Tawa OOO till Jul 10th, 2017/07/21 13:53:42]

Correct, this was racy and still is.  That's OK though, this is best effort
only.  Which is why it is done on a background thread with skip-on-shutdown
semantics.

If it does not get done this time it will happen on the next chrome run.  If it
happens this time and does not get "recorded", it will happen again next time
but will be de-duped server side.  By "not recorded" I mean the response is not
parsed and remembered locally that it was done.

[gab, 2017/07/21 22:39:17]

It's okay that this may fail and will be retried but what I'm saying is the code
can result in a data race and this is never okay as it results in undefined
behaviour :
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...

+                                       base::Unretained(fetcher.get())));
 
-  const base::TimeDelta kTimeout = base::TimeDelta::FromMinutes(5);
-  base::MessageLoop::ScopedNestableTaskAllower allow_nested(
-      base::MessageLoop::current());
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::Bind(&ShutdownCheck, weak.GetWeakPtr()));
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(&net::URLFetcher::Start, base::Unretained(fetcher.get())));
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, loop.QuitClosure(), kTimeout);
-
-  loop.Run();
+  event->TimedWait(base::TimeDelta::FromMinutes(5));
 
   if (fetcher->GetResponseCode() != 200)
     return false;
 
   return fetcher->GetResponseAsString(response);
 #endif
 }
 
 bool FinancialPing::IsPingTime(Product product, bool no_delay) {
   ScopedRlzValueStoreLock lock;
@@ skipping 50 matching lines @@
 }
 
 bool WasSendFinancialPingInterrupted() {
   return send_financial_ping_interrupted_for_test;
 }
 
 }  // namespace test
 #endif
 
 }  // namespace rlz_lib