Index: rlz/lib/financial_ping.cc |
diff --git a/rlz/lib/financial_ping.cc b/rlz/lib/financial_ping.cc |
index 04bd45a64b5289595ab58e1bf625a984be88d01c..53a8092bbdf08d469460d1f233ba51b09d135f46 100644 |
--- a/rlz/lib/financial_ping.cc |
+++ b/rlz/lib/financial_ping.cc |
@@ -13,11 +13,13 @@ |
#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" |
@@ -203,39 +205,149 @@ bool FinancialPing::SetURLRequestContext( |
namespace { |
-class FinancialPingUrlFetcherDelegate : public net::URLFetcherDelegate { |
+// 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: |
- FinancialPingUrlFetcherDelegate(const base::Closure& callback) |
- : callback_(callback) { |
+ 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 b
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 simple
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 a
Roger Tawa OOO till Jul 10th
2017/07/28 18:43:06
Actually, it does not. After an object is moved,
gab
2017/07/28 19:28:57
Hmmm true, but you only TakeResponse() once per Re
Roger Tawa OOO till Jul 10th
2017/07/28 19:42:17
I'd rather not leave the member variable in an uns
|
} |
- void OnURLFetchComplete(const net::URLFetcher* source) override; |
private: |
- base::Closure callback_; |
+ ~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 initial
Roger Tawa OOO till Jul 10th
2017/07/28 18:43:07
Done.
|
}; |
-void FinancialPingUrlFetcherDelegate::OnURLFetchComplete( |
- const net::URLFetcher* source) { |
- callback_.Run(); |
-} |
+// A fetcher delegate that signals an instance of RefCountedWaitableEvent when |
+// the fetch completes. |
+class FinancialPingUrlFetcherDelegate : public net::URLFetcherDelegate { |
+ public: |
+ FinancialPingUrlFetcherDelegate(scoped_refptr<RefCountedWaitableEvent> event) |
+ : event_(std::move(event)) {} |
+ |
+ void SetFetcher(std::unique_ptr<net::URLFetcher> fetcher) { |
+ fetcher_ = std::move(fetcher); |
+ } |
+ |
+ private: |
+ 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_; |
+}; |
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 |
@@ -296,83 +408,29 @@ bool FinancialPing::PingServer(const char* request, std::string* response) { |
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); |
- |
- base::WeakPtrFactory<base::RunLoop> weak(&loop); |
+ scoped_refptr<base::SequencedTaskRunner> background_runner( |
gab
2017/07/27 18:29:38
Add a comment here (or on PingRlzServer() or both)
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)); |
- 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 |
} |