Add periodic check to see if context has been reset.

Context may be set to NULL when shutdown is requested
while trying to send ping. With this CL, rlz will cancel
the request when it detects the NULL context.

non chromeos chrome used to wait even after context is set
to NULL, so maybe I should do this only for chromeos. Please
let me know what you think.

This also fixes the edge race case where the g_context
may be set to NULL in the middle of FinancialPing::PingServer.

BUG=261377

Review URL: https://codereview.chromium.org/26929004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@229114 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 126 insertions, 16 deletions

diff --git a/rlz/lib/financial_ping.cc b/rlz/lib/financial_ping.cc
index 5f15256..9c596a4 100644
--- a/rlz/lib/financial_ping.cc
+++ b/rlz/lib/financial_ping.cc
@@ -6,8 +6,10 @@
 
 #include "rlz/lib/financial_ping.h"
 
+#include "base/atomicops.h"
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -84,6 +86,8 @@ int64 GetSystemTimeAsInt64() {
 
 namespace rlz_lib {
 
+using base::subtle::AtomicWord;
+
 bool FinancialPing::FormRequest(Product product,
     const AccessPoint* access_points, const char* product_signature,
     const char* product_brand, const char* product_id,
@@ -177,12 +181,15 @@ bool FinancialPing::FormRequest(Product product,
 }
 
 #if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
-// The URLRequestContextGetter used by FinancialPing::PingServer().
-net::URLRequestContextGetter* g_context;
+// The pointer to URLRequestContextGetter used by FinancialPing::PingServer().
+// It is atomic pointer because it can be accessed and modified by multiple
+// threads.
+AtomicWord g_context;
 
 bool FinancialPing::SetURLRequestContext(
     net::URLRequestContextGetter* context) {
-  g_context = context;
+  base::subtle::NoBarrier_Store(
+      &g_context, reinterpret_cast<AtomicWord>(context));
   return true;
 }
 
@@ -204,10 +211,33 @@ void FinancialPingUrlFetcherDelegate::OnURLFetchComplete(
   callback_.Run();
 }
 
+#if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
+bool send_financial_ping_interrupted_for_test = false;
+#endif
+
 }  // namespace
 
 #endif
 
+#if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
+void ShutdownCheck(base::WeakPtr<base::RunLoop> weak) {
+  if (!weak.get())
+    return;
+  if (!base::subtle::NoBarrier_Load(&g_context)) {
+    send_financial_ping_interrupted_for_test = true;
+    weak->QuitClosure().Run();
+    return;
+  }
+  // How frequently the financial ping thread should check
+  // the shutdown condition?
+  const base::TimeDelta kInterval = base::TimeDelta::FromMilliseconds(500);
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&ShutdownCheck, weak),
+      kInterval);
+}
+#endif
+
 bool FinancialPing::PingServer(const char* request, std::string* response) {
   if (!response)
     return false;
@@ -265,8 +295,15 @@ 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::NoBarrier_Load(&g_context));
+
   // Browser shutdown will cause the context to be reset to NULL.
-  if (!g_context)
+  if (!context)
     return false;
 
   // Run a blocking event loop to match the win inet implementation.
@@ -292,13 +329,18 @@ bool FinancialPing::PingServer(const char* request, std::string* response) {
 
   // Ensure rlz_lib::SetURLRequestContext() has been called before sending
   // pings.
-  fetcher->SetRequestContext(g_context);
+  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::MessageLoop::current()->PostTask(
       FROM_HERE,
+      base::Bind(&ShutdownCheck, weak.GetWeakPtr()));
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
       base::Bind(&net::URLFetcher::Start, base::Unretained(fetcher.get())));
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE, loop.QuitClosure(), kTimeout);
@@ -359,4 +401,18 @@ bool FinancialPing::ClearLastPingTime(Product product) {
   return store->ClearPingTime(product);
 }
 
-}  // namespace
+#if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
+namespace test {
+
+void ResetSendFinancialPingInterrupted() {
+  send_financial_ping_interrupted_for_test = false;
+}
+
+bool WasSendFinancialPingInterrupted() {
+  return send_financial_ping_interrupted_for_test;
+}
+
+}  // namespace test
+#endif
+
+}  // namespace rlz_lib
diff --git a/rlz/lib/financial_ping.h b/rlz/lib/financial_ping.h
index 081ecba..63c20a1 100644
--- a/rlz/lib/financial_ping.h
+++ b/rlz/lib/financial_ping.h
@@ -58,6 +58,13 @@ class FinancialPing {
   ~FinancialPing() {}
 };
 
+#if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
+namespace test {
+void ResetSendFinancialPingInterrupted();
+bool WasSendFinancialPingInterrupted();
+}  // namespace test
+#endif
+
 }  // namespace rlz_lib
 
 
diff --git a/rlz/lib/rlz_lib_test.cc b/rlz/lib/rlz_lib_test.cc
index f21c540..10cf3de 100644
--- a/rlz/lib/rlz_lib_test.cc
+++ b/rlz/lib/rlz_lib_test.cc
@@ -19,6 +19,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#include "rlz/lib/financial_ping.h"
 #include "rlz/lib/rlz_lib.h"
 #include "rlz/lib/rlz_value_store.h"
 #include "rlz/test/rlz_test_helpers.h"
@@ -416,6 +417,16 @@ TEST_F(RlzLibTest, ParsePingResponseWithStatefulEvents) {
   EXPECT_STREQ("events=W1I", value);
 }
 
+class URLRequestRAII {
+ public:
+  URLRequestRAII(net::URLRequestContextGetter* context) {
+    rlz_lib::SetURLRequestContext(context);
+  }
+  ~URLRequestRAII() {
+    rlz_lib::SetURLRequestContext(NULL);
+  }
+};
+
 TEST_F(RlzLibTest, SendFinancialPing) {
   // We don't really check a value or result in this test. All this does is
   // attempt to ping the financial server, which you can verify in Fiddler.
@@ -437,16 +448,6 @@ TEST_F(RlzLibTest, SendFinancialPing) {
           io_thread.message_loop()->message_loop_proxy());
   rlz_lib::SetURLRequestContext(context.get());
 
-  class URLRequestRAII {
-    public:
-     URLRequestRAII(net::URLRequestContextGetter* context) {
-       rlz_lib::SetURLRequestContext(context);
-     }
-     ~URLRequestRAII() {
-       rlz_lib::SetURLRequestContext(NULL);
-     }
-  };
-
   URLRequestRAII set_context(context.get());
 #endif
 
@@ -474,6 +475,52 @@ TEST_F(RlzLibTest, SendFinancialPing) {
       /*skip_time_check=*/true);
 }
 
+#if defined(RLZ_NETWORK_IMPLEMENTATION_CHROME_NET)
+
+void ResetContext() {
+  rlz_lib::SetURLRequestContext(NULL);
+}
+
+TEST_F(RlzLibTest, SendFinancialPingDuringShutdown) {
+  // rlz_lib::SendFinancialPing fails when this is set.
+  if (!rlz_lib::SupplementaryBranding::GetBrand().empty())
+    return;
+
+#if defined(OS_MACOSX)
+  base::mac::ScopedNSAutoreleasePool pool;
+#endif
+
+  base::Thread::Options options;
+  options.message_loop_type = base::MessageLoop::TYPE_IO;
+
+  base::Thread io_thread("rlz_unittest_io_thread");
+  ASSERT_TRUE(io_thread.StartWithOptions(options));
+
+  scoped_refptr<net::TestURLRequestContextGetter> context =
+      new net::TestURLRequestContextGetter(
+          io_thread.message_loop()->message_loop_proxy());
+  rlz_lib::SetURLRequestContext(context.get());
+
+  URLRequestRAII set_context(context.get());
+
+  rlz_lib::AccessPoint points[] =
+    {rlz_lib::IETB_SEARCH_BOX, rlz_lib::NO_ACCESS_POINT,
+     rlz_lib::NO_ACCESS_POINT};
+  rlz_lib::test::ResetSendFinancialPingInterrupted();
+  EXPECT_FALSE(rlz_lib::test::WasSendFinancialPingInterrupted());
+
+  base::MessageLoop loop;
+  loop.PostTask(FROM_HERE, base::Bind(&ResetContext));
+  std::string request;
+  EXPECT_FALSE(rlz_lib::SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER, points,
+      "swg", "GGLA", "SwgProductId1234", "en-UK", false,
+      /*skip_time_check=*/true));
+
+  EXPECT_TRUE(rlz_lib::test::WasSendFinancialPingInterrupted());
+  rlz_lib::test::ResetSendFinancialPingInterrupted();
+}
+#endif
+
 TEST_F(RlzLibTest, ClearProductState) {
   MachineDealCodeHelper::Clear();