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/lock.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 {
 
+// A waitable event used to detect when either:
+//
+//   1/ the RLZ ping request completes
+//   2/ the RLZ ping request times out
+//   3/ browser shutdown begins
+class RefCountedWaitableEvent
+    : public base::RefCountedThreadSafe<RefCountedWaitableEvent> {
+ public:
+  RefCountedWaitableEvent()
+      : event_(base::WaitableEvent::ResetPolicy::MANUAL,
+               base::WaitableEvent::InitialState::NOT_SIGNALED),
+        response_code_(net::URLFetcher::RESPONSE_CODE_INVALID) {}
+
+  void SignalShutdown() { event_.Signal(); }
+
+  void SignalFetchComplete(int response_code, std::string&& response) {

[gab, 2017/07/27 18:29:38]

I'm all for r-value params but style guide still bans them in member methods:
https://google.github.io/styleguide/cppguide.html#Rvalue_references

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:06]

Thanks, I had not seen that style guideline.

+    base::AutoLock autolock(lock_);
+    response_code_ = response_code;
+    response_ = response;
+    event_.Signal();
+  }
+
+  bool TimedWait(const base::TimeDelta& timeout) {

[gab, 2017/07/27 18:29:37]

Pass TimeDelta by value (see time.h)

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:07]

Done.

+    return event_.TimedWait(timeout);
+  }
+
+  int GetResponseCode() {
+    base::AutoLock autolock(lock_);
+    return response_code_;
+  }
+
+  void TakeResponse(std::string* response) {

[gab, 2017/07/27 18:29:38]

I think 

std::string TakeResponse();

is a simpler API, you can

return std::move(response_) to get the same effect.

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:07]

Done.

+    base::AutoLock autolock(lock_);
+    *response = std::move(response_);
+    response_.clear();

[gab, 2017/07/27 18:29:37]

The above std::move() would clear() implicitly I assume.

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:06]

Actually, it does not.  After an object is moved, it is left in an unspecified
but consistent state.  I want to clear() it so that its value becomes specified
(i.e. empty).

[gab, 2017/07/28 19:28:57]

Hmmm true, but you only TakeResponse() once per RefCountedWaitableEvent so I
don't see why it matters? Can't TakeResponse() also just have move semantics
(leaving |response_| in a valid but unspecified state)?

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

I'd rather not leave the member variable in an unspecified state.

+  }
+
+ private:
+  ~RefCountedWaitableEvent() {}
+  friend class base::RefCountedThreadSafe<RefCountedWaitableEvent>;
+
+  base::WaitableEvent event_;
+  base::Lock lock_;
+  std::string response_;
+  int response_code_;

[gab, 2017/07/27 18:29:38]

For POD types I prefer C++11 inline member initialization instead of initializer
list, i.e.

int response_code_ = net::URLFetcher::RESPONSE_CODE_INVALID;

easier to read (and make sure a future member doesn't forget to init)

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:07]

Done.

+};
+
+// A fetcher delegate that signals an instance of RefCountedWaitableEvent when
+// the fetch completes.
 class FinancialPingUrlFetcherDelegate : public net::URLFetcherDelegate {
  public:
-  FinancialPingUrlFetcherDelegate(const base::Closure& callback)
-      : callback_(callback) {
+  FinancialPingUrlFetcherDelegate(scoped_refptr<RefCountedWaitableEvent> event)
+      : event_(std::move(event)) {}
+
+  void SetFetcher(std::unique_ptr<net::URLFetcher> fetcher) {
+    fetcher_ = std::move(fetcher);
   }
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
 
  private:
-  base::Closure callback_;
+  void OnURLFetchComplete(const net::URLFetcher* source) override {
+    std::string response;
+    source->GetResponseAsString(&response);
+    event_->SignalFetchComplete(source->GetResponseCode(), std::move(response));
+    base::SequencedTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  }
+
+  scoped_refptr<RefCountedWaitableEvent> event_;
+  std::unique_ptr<net::URLFetcher> fetcher_;
 };
 
-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->SignalShutdown();
     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);
+}
+
+void PingRlzServer(std::string url,
+                   scoped_refptr<RefCountedWaitableEvent> event) {
+  // Copy the pointer to stack because g_context may be set to NULL
+  // 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.
+  // ShutdownCheck will catch this.
+  if (!context)
+    return;
+
+  // Delegate will delete itself when the fetch completes.
+  FinancialPingUrlFetcherDelegate* delegate =
+      new FinancialPingUrlFetcherDelegate(event);
+
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation("rlz_ping", R"(
+        semantics {
+          sender: "RLZ Ping"
+          description:
+            "Used for measuring the effectiveness of a promotion. See the "
+            "Chrome Privacy Whitepaper for complete details."
+          trigger:
+            "1- At Chromium first run.\n"
+            "2- When Chromium is re-activated by a new promotion.\n"
+            "3- Once a week thereafter as long as Chromium is used.\n"
+          data:
+            "1- Non-unique cohort tag of when Chromium was installed.\n"
+            "2- Unique machine id on desktop platforms.\n"
+            "3- Whether Google is the default omnibox search.\n"
+            "4- Whether google.com is the default home page."
+          destination: GOOGLE_OWNED_SERVICE
+        }
+        policy {
+          cookies_allowed: NO
+          setting: "This feature cannot be disabled in settings."
+          policy_exception_justification: "Not implemented."
+        })");
+  std::unique_ptr<net::URLFetcher> fetcher = net::URLFetcher::Create(
+      GURL(url), net::URLFetcher::GET, delegate, traffic_annotation);
+
+  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);
+  fetcher->Start();
+
+  // Pass ownership of the fetcher to the delegate.  Otherwise the fetch will
+  // be canceled when the URLFetcher object is destroyed.
+  delegate->SetFetcher(std::move(fetcher));
 }
 #endif
 
 bool FinancialPing::PingServer(const char* request, std::string* response) {
   if (!response)
     return false;
 
   response->clear();
 
 #if defined(RLZ_NETWORK_IMPLEMENTATION_WIN_INET)

[gab, 2017/07/27 18:29:38]

Curious, is this still ever a thing or should that be cleaned up (not in this CL
of course)

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:06]

Possibly, not sure, but not in this CL as you say.

   // Initialize WinInet.
   InternetHandle inet_handle = InternetOpenA(kFinancialPingUserAgent,
                                              INTERNET_OPEN_TYPE_PRECONFIG,
                                              NULL, NULL, 0);
   if (!inet_handle)
     return false;
 
   // Open network connection.
   InternetHandle connection_handle = InternetConnectA(inet_handle,
       kFinancialServer, kFinancialPort, "", "", INTERNET_SERVICE_HTTP,
@@ skipping 30 matching lines @@
 
   DWORD bytes_read = 0;
   while (InternetReadFile(http_handle, buffer.get(), kMaxPingResponseLength,
                           &bytes_read) && bytes_read > 0) {
     response->append(buffer.get(), bytes_read);
     bytes_read = 0;
   };
 
   return true;
 #else
-  // Copy the pointer to stack because g_context may be set to NULL
-  // 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.
-  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());
-
   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:
-            "Used for measuring the effectiveness of a promotion. See the "
-            "Chrome Privacy Whitepaper for complete details."
-          trigger:
-            "1- At Chromium first run.\n"
-            "2- When Chromium is re-activated by a new promotion.\n"
-            "3- Once a week thereafter as long as Chromium is used.\n"
-          data:
-            "1- Non-unique cohort tag of when Chromium was installed.\n"
-            "2- Unique machine id on desktop platforms.\n"
-            "3- Whether Google is the default omnibox search.\n"
-            "4- Whether google.com is the default home page."
-          destination: GOOGLE_OWNED_SERVICE
-        }
-        policy {
-          cookies_allowed: NO
-          setting: "This feature cannot be disabled in settings."
-          policy_exception_justification: "Not implemented."
-        })");
-  std::unique_ptr<net::URLFetcher> fetcher = net::URLFetcher::Create(
-      GURL(url), net::URLFetcher::GET, &delegate, traffic_annotation);
+  // Use a waitable event to cause this function to block, to match the
+  // wininet implementation.
+  auto event = base::MakeRefCounted<RefCountedWaitableEvent>();
 
-  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);
+  base::PostTask(FROM_HERE, base::Bind(&ShutdownCheck, event));

[gab, 2017/07/27 18:29:38]

PostTaskWithTraits with TaskPriority::BACKGROUND?

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:07]

Done.

 
-  // Ensure rlz_lib::SetURLRequestContext() has been called before sending
-  // pings.
-  fetcher->SetRequestContext(context);
+  scoped_refptr<base::SequencedTaskRunner> background_runner(

[gab, 2017/07/27 18:29:38]

Add a comment here (or on PingRlzServer() or both) to explain why it's important
to run this on a separate sequence as we've determined.

[Roger Tawa OOO till Jul 10th, 2017/07/28 18:43:06]

Done.

+      base::CreateSequencedTaskRunnerWithTraits(
+          {base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
+           base::TaskPriority::BACKGROUND}));
+  background_runner->PostTask(FROM_HERE,
+                              base::Bind(&PingRlzServer, url, event));
 
-  base::WeakPtrFactory<base::RunLoop> weak(&loop);
-
-  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();
-
-  if (fetcher->GetResponseCode() != 200)
+  bool is_signaled = event->TimedWait(base::TimeDelta::FromMinutes(5));
+  if (!is_signaled || event->GetResponseCode() != 200)
     return false;
 
-  return fetcher->GetResponseAsString(response);
+  event->TakeResponse(response);
+  return true;
 #endif
 }
 
 bool FinancialPing::IsPingTime(Product product, bool no_delay) {
   ScopedRlzValueStoreLock lock;
   RlzValueStore* store = lock.GetStore();
   if (!store || !store->HasAccess(RlzValueStore::kReadAccess))
     return false;
 
   int64_t last_ping = 0;
@@ skipping 45 matching lines @@
 }
 
 bool WasSendFinancialPingInterrupted() {
   return send_financial_ping_interrupted_for_test;
 }
 
 }  // namespace test
 #endif
 
 }  // namespace rlz_lib