Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(49)

Unified Diff: rlz/lib/financial_ping.cc

Issue 2949263003: Remove call to GetBlockingPool in RLZ (Closed)
Patch Set: Use PostDelayedTaskWithTraits Created 3 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « ios/chrome/browser/rlz/rlz_tracker_delegate_impl.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: rlz/lib/financial_ping.cc
diff --git a/rlz/lib/financial_ping.cc b/rlz/lib/financial_ping.cc
index 04bd45a64b5289595ab58e1bf625a984be88d01c..775c1adf4a52a85391c0c06420d129587a339b5a 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,147 @@ 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) {}
+
+ void SignalShutdown() { event_.Signal(); }
+
+ void SignalFetchComplete(int response_code, std::string response) {
+ base::AutoLock autolock(lock_);
+ response_code_ = response_code;
+ response_ = std::move(response);
+ event_.Signal();
+ }
+
+ bool TimedWait(base::TimeDelta timeout) { return event_.TimedWait(timeout); }
+
+ int GetResponseCode() {
+ base::AutoLock autolock(lock_);
+ return response_code_;
+ }
+
+ std::string TakeResponse() {
+ base::AutoLock autolock(lock_);
+ std::string temp = std::move(response_);
+ response_.clear();
+ return temp;
}
- 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_ = net::URLFetcher::RESPONSE_CODE_INVALID;
};
-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::PostDelayedTaskWithTraits(FROM_HERE, {base::TaskPriority::BACKGROUND},
+ 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 +406,33 @@ 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);
-
- 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);
-
- 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)
+ // Use a waitable event to cause this function to block, to match the
+ // wininet implementation.
+ auto event = base::MakeRefCounted<RefCountedWaitableEvent>();
+
+ base::PostTaskWithTraits(FROM_HERE, {base::TaskPriority::BACKGROUND},
+ base::Bind(&ShutdownCheck, event));
+
+ // PingRlzServer must be run in a separate sequence so that the TimedWait()
+ // call below does not block the URL fetch response from being handled by
+ // the URL delegate.
+ scoped_refptr<base::SequencedTaskRunner> background_runner(
+ base::CreateSequencedTaskRunnerWithTraits(
+ {base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
+ base::TaskPriority::BACKGROUND}));
+ background_runner->PostTask(FROM_HERE,
+ base::Bind(&PingRlzServer, url, event));
+
+ bool is_signaled = event->TimedWait(base::TimeDelta::FromMinutes(5));
+ if (!is_signaled || event->GetResponseCode() != 200)
return false;
- return fetcher->GetResponseAsString(response);
+ *response = event->TakeResponse();
+ return true;
#endif
}
« no previous file with comments | « ios/chrome/browser/rlz/rlz_tracker_delegate_impl.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698