In SdchDictionaryFetcher, don't set state when scheduling if loop is active.

net/url_request/sdch_dictionary_fetcher_unittest.cc

 // Copyright 2014 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.
 
 #include "net/url_request/sdch_dictionary_fetcher.h"
 
 #include <string>
 
 #include "base/bind.h"
-#include "base/message_loop/message_loop_proxy.h"
+#include "base/callback.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
 #include "net/base/sdch_manager.h"
 #include "net/url_request/url_request_data_job.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_interceptor.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 namespace net {
 
-static const char kSampleBufferContext[] = "This is a sample buffer.";
-static const char kTestDomain[] = "top.domain.test";
+namespace {
+
+const char kSampleBufferContext[] = "This is a sample buffer.";
+const char kTestDomain[] = "top.domain.test";
 
 class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
  public:
   URLRequestSpecifiedResponseJob(URLRequest* request,
-                                 NetworkDelegate* network_delegate)
-      : URLRequestSimpleJob(request, network_delegate) {}
-
-  static void AddUrlHandler() {
-    net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
-    jobs_requested_ = 0;
-    filter->AddHostnameHandler(
-        "http", kTestDomain, &URLRequestSpecifiedResponseJob::Factory);
-  }
-
-  static void RemoveUrlHandler() {
-    net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
-    filter->RemoveHostnameHandler("http", kTestDomain);
-    jobs_requested_ = 0;
-  }
-
-  static URLRequestJob* Factory(URLRequest* request,
-                                net::NetworkDelegate* network_delegate,
-                                const std::string& scheme) {
-    ++jobs_requested_;
-    return new URLRequestSpecifiedResponseJob(request, network_delegate);
-  }
+                                 NetworkDelegate* network_delegate,
+                                 const base::Closure& destruction_callback)
+      : URLRequestSimpleJob(request, network_delegate),
+        destruction_callback_(destruction_callback) {}
 
   static std::string ExpectedResponseForURL(const GURL& url) {
     return base::StringPrintf("Response for %s\n%s\nEnd Response for %s\n",
                               url.spec().c_str(),
                               kSampleBufferContext,
                               url.spec().c_str());
   }
 
-  static int jobs_requested() { return jobs_requested_; }
-
  private:
-  ~URLRequestSpecifiedResponseJob() override{};
+  ~URLRequestSpecifiedResponseJob() override {
+    if (!destruction_callback_.is_null())

[mmenke, 2015/01/26 16:06:06]

Since you're never passing in a null callback, shouldn't bother with this (A
DCHECK is fine, if you want.  Note that even if the weak pointer is NULL, the
callback is not - otherwise, couldn't null check on different threads).

[Randy Smith (Not in Mondays), 2015/01/26 19:21:17]

Done.

+      destruction_callback_.Run();
+  }
 
   // URLRequestSimpleJob implementation:
-
   int GetData(std::string* mime_type,
               std::string* charset,
               std::string* data,
               const CompletionCallback& callback) const override {
     GURL url(request_->url());
     *data = ExpectedResponseForURL(url);
     return OK;
   }
 
-  base::TaskRunner* GetTaskRunner() const override {
-    return base::MessageLoopProxy::current().get();
+  base::Closure destruction_callback_;

[mmenke, 2015/01/26 16:06:06]

optional:  Could make this const.

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Done.

+};
+
+class SpecifiedResponseJobInterceptor : public URLRequestInterceptor {
+ public:
+  typedef base::Callback<void(int outstanding_job_delta)> LifecycleCallback;

[mmenke, 2015/01/26 16:06:06]

Think this could use a comment, what it doesn't what its argument does, etc.

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Done.

+
+  SpecifiedResponseJobInterceptor(const LifecycleCallback& lifecycle_callback)

[mmenke, 2015/01/26 16:06:06]

nit:  explicit

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Done.

+      : lifecycle_callback_(lifecycle_callback), factory_(this) {}
+  ~SpecifiedResponseJobInterceptor() override {}
+
+  URLRequestJob* MaybeInterceptRequest(
+      URLRequest* request,
+      NetworkDelegate* network_delegate) const override {
+    if (!lifecycle_callback_.is_null())
+      lifecycle_callback_.Run(1);
+
+    return new URLRequestSpecifiedResponseJob(
+        request, network_delegate,
+        base::Bind(
+            &SpecifiedResponseJobInterceptor::OnSpecfiedResponseJobDestruction,
+            factory_.GetWeakPtr()));

[mmenke, 2015/01/26 16:06:06]

Don't need two weak pointers here (The test fixture has one, and this class has
another)...  In fact, since the callback is never null, can get rid of
"OnSpecfiedResponseJobDestruction" and just do everything here:

base::Bind(lifecycle_callback_, -1)

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Good point.  I'm including the DCHECKs for interface specification, though. 
Done.

   }
 
-  static int jobs_requested_;
+  // The caller must ensure that the callback is valid to call for the
+  // lifetime of the SpecifiedResponseJobInterceptor (i.e. until
+  // Unregister() is called).
+  static void RegisterWithFilter(const LifecycleCallback& lifecycle_callback) {
+    scoped_ptr<SpecifiedResponseJobInterceptor> interceptor(
+        new SpecifiedResponseJobInterceptor(lifecycle_callback));
+
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", kTestDomain, interceptor.Pass());
+  }
+
+  static void Unregister() {
+    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler("http",
+                                                                kTestDomain);
+  }
+
+ private:
+  void OnSpecfiedResponseJobDestruction() const {
+    if (!lifecycle_callback_.is_null())
+      lifecycle_callback_.Run(-1);
+  }
+
+  LifecycleCallback lifecycle_callback_;
+  mutable base::WeakPtrFactory<SpecifiedResponseJobInterceptor> factory_;
 };
 
-int URLRequestSpecifiedResponseJob::jobs_requested_(0);
+// Local test infrastructure
+// * URLRequestSpecifiedResponseJob: A URLRequestJob that returns
+//   a different but derivable response for each URL (used for all
+//   url requests in this file).  The class provides interfaces to
+//   signal whenever the total number of jobs transitions to zero.
+// * SdchDictionaryFetcherTest: Registers a callback with the above
+//   class, and provides blocking interfaces for a transition to zero jobs.
+//   Contains an SdchDictionaryFetcher, and tracks fetcher dictionary
+//   addition callbacks.
+// Most tests schedule a dictionary fetch, wait for no jobs outstanding,
+// then test that the fetch results are as expected.
 
 class SdchDictionaryFetcherTest : public ::testing::Test {
  public:
   struct DictionaryAdditions {
     DictionaryAdditions(const std::string& dictionary_text,
                         const GURL& dictionary_url)
         : dictionary_text(dictionary_text), dictionary_url(dictionary_url) {}
 
     std::string dictionary_text;
     GURL dictionary_url;
   };
 
-  SdchDictionaryFetcherTest() {
-    URLRequestSpecifiedResponseJob::AddUrlHandler();
-    context_.reset(new TestURLRequestContext);
-    fetcher_.reset(new SdchDictionaryFetcher(
-        context_.get(),
-        base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
-                   base::Unretained(this))));
+  SdchDictionaryFetcherTest()
+      : jobs_requested_(0),
+        jobs_outstanding_(0),
+        context_(new TestURLRequestContext),
+        fetcher_(new SdchDictionaryFetcher(
+            context_.get(),
+            base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
+                       base::Unretained(this)))),
+        factory_(this) {
+    SpecifiedResponseJobInterceptor::RegisterWithFilter(
+        base::Bind(&SdchDictionaryFetcherTest::OnNumberJobsChanged,
+                   factory_.GetWeakPtr()));
   }
 
   ~SdchDictionaryFetcherTest() override {
-    URLRequestSpecifiedResponseJob::RemoveUrlHandler();
+    SpecifiedResponseJobInterceptor::Unregister();
   }
 
   void OnDictionaryFetched(const std::string& dictionary_text,
                            const GURL& dictionary_url,
                            const BoundNetLog& net_log) {
     dictionary_additions.push_back(
         DictionaryAdditions(dictionary_text, dictionary_url));
   }
 
   void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {

[mmenke, 2015/01/26 16:06:06]

Suggest comments here.  The clear behavior, in particular, is a bit confusing,
given the name "Get".

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Done.

     out->swap(dictionary_additions);
     dictionary_additions.clear();
   }
 
   SdchDictionaryFetcher* fetcher() { return fetcher_.get(); }
 
   // May not be called outside the SetUp()/TearDown() interval.
-  int JobsRequested() {
-    return URLRequestSpecifiedResponseJob::jobs_requested();
-  }
+  int jobs_requested() { return jobs_requested_; }

[mmenke, 2015/01/26 16:06:06]

nit:  const

[Randy Smith (Not in Mondays), 2015/01/26 19:21:17]

Done.

 
   GURL PathToGurl(const char* path) {
     std::string gurl_string("http://");
     gurl_string += kTestDomain;
     gurl_string += "/";
     gurl_string += path;
     return GURL(gurl_string);
   }
 
+  // Block until there are no outstanding URLRequestSpecifiedResponseJobs.
+  void WaitForNoJobs() {
+    if (jobs_outstanding_ == 0)
+      return;
+
+    run_loop_.reset(new base::RunLoop);
+    run_loop_->Run();
+    run_loop_.reset();
+  }
+
  private:
+  void OnNumberJobsChanged(int outstanding_jobs_delta) {
+    if (outstanding_jobs_delta > 0)
+      jobs_requested_ += outstanding_jobs_delta;
+    jobs_outstanding_ += outstanding_jobs_delta;
+    if (jobs_outstanding_ == 0 && run_loop_.get())

[mmenke, 2015/01/26 16:06:06]

nit:  get() isn't necessary when coercing to bool (I figure since scoped_ptr has
code specifically to support that, we should use it)

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Done.

+      run_loop_->Quit();
+  }
+
+  int jobs_requested_;
+  int jobs_outstanding_;
+  scoped_ptr<base::RunLoop> run_loop_;
   scoped_ptr<TestURLRequestContext> context_;
   scoped_ptr<SdchDictionaryFetcher> fetcher_;
   std::vector<DictionaryAdditions> dictionary_additions;
+  base::WeakPtrFactory<SdchDictionaryFetcherTest> factory_;
 };
 
 // Schedule a fetch and make sure it happens.
 TEST_F(SdchDictionaryFetcherTest, Basic) {
   GURL dictionary_url(PathToGurl("dictionary"));
   fetcher()->Schedule(dictionary_url);
+  WaitForNoJobs();
 
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1, JobsRequested());
+  EXPECT_EQ(1, jobs_requested());
   std::vector<DictionaryAdditions> additions;
   GetDictionaryAdditions(&additions);
   ASSERT_EQ(1u, additions.size());
   EXPECT_EQ(
       URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
       additions[0].dictionary_text);
 }
 
 // Multiple fetches of the same URL should result in only one request.
 TEST_F(SdchDictionaryFetcherTest, Multiple) {
   GURL dictionary_url(PathToGurl("dictionary"));
   fetcher()->Schedule(dictionary_url);
   fetcher()->Schedule(dictionary_url);
   fetcher()->Schedule(dictionary_url);
-  base::RunLoop().RunUntilIdle();
+  WaitForNoJobs();
 
-  EXPECT_EQ(1, JobsRequested());
+  EXPECT_EQ(1, jobs_requested());
   std::vector<DictionaryAdditions> additions;
   GetDictionaryAdditions(&additions);
   ASSERT_EQ(1u, additions.size());
   EXPECT_EQ(
       URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
       additions[0].dictionary_text);
 }
 
 // A cancel should result in no actual requests being generated.
 TEST_F(SdchDictionaryFetcherTest, Cancel) {
   GURL dictionary_url_1(PathToGurl("dictionary_1"));
   GURL dictionary_url_2(PathToGurl("dictionary_2"));
   GURL dictionary_url_3(PathToGurl("dictionary_3"));
 
   fetcher()->Schedule(dictionary_url_1);
   fetcher()->Schedule(dictionary_url_2);
   fetcher()->Schedule(dictionary_url_3);
   fetcher()->Cancel();
-  base::RunLoop().RunUntilIdle();
+  WaitForNoJobs();
 
   // Synchronous execution may have resulted in a single job being scheduled.
-  EXPECT_GE(1, JobsRequested());
+  EXPECT_GE(1, jobs_requested());
 }
+
+// Attempt to confuse the fetcher loop processing by scheduling a
+// dictionary addition while another fetch is in process.
+TEST_F(SdchDictionaryFetcherTest, LoopRace) {
+  GURL dictionary_url(PathToGurl("dictionary"));

[mmenke, 2015/01/26 16:06:06]

Suggest replacing "dictionary" with "dictionary0".  Having dictionary and
dictionary1 is needlessly confusing.  0/1 instead of 1/2 results in matching the
indices of the additions array (I assume that's why you used 1 instead of 2).

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Probably--I don't remember :-}.  Done.

+  GURL dictionary1_url(PathToGurl("dictionary1"));
+  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary1_url);
+  WaitForNoJobs();
+
+  EXPECT_EQ(2, jobs_requested());

[mmenke, 2015/01/26 16:06:06]

This should be an assert, I think.

[Randy Smith (Not in Mondays), 2015/01/26 19:21:18]

Agreed; done.

+  std::vector<DictionaryAdditions> additions;
+  GetDictionaryAdditions(&additions);
+  ASSERT_EQ(2u, additions.size());
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary_url),
+      additions[0].dictionary_text);
+  EXPECT_EQ(
+      URLRequestSpecifiedResponseJob::ExpectedResponseForURL(dictionary1_url),
+      additions[1].dictionary_text);
 }
+
+}  // namespace
+
+}  // namespace net