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

net/url_request/sdch_dictionary_fetcher_unittest.cc

Index: net/url_request/sdch_dictionary_fetcher_unittest.cc
diff --git a/net/url_request/sdch_dictionary_fetcher_unittest.cc b/net/url_request/sdch_dictionary_fetcher_unittest.cc
index 69adfed768007007729b1ca202d4475987266e87..25bf98ee330e8c60090a756784b2b6d83a7830d2 100644
--- a/net/url_request/sdch_dictionary_fetcher_unittest.cc
+++ b/net/url_request/sdch_dictionary_fetcher_unittest.cc
@@ -7,6 +7,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
@@ -18,6 +19,18 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+// 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.

[mmenke, 2015/01/21 21:33:57]

Class level are generally be put just above the class they describe - don't
think there's anything here that warrants doing otherwise.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:40]

Hmmm.  I can do that, and if you really want me to, I will.  But I think it
undercuts the reason I put the documentation in in the first place.  

When I go to modify a test file, the first thing I want to know is what the
infrastructure & usual test pattern is.  My goal with this comment was to
provide that information so that the next time I add a test to this file I can
quickly internalize what tools I have to work with and what the likely pattern
of my test is.

Tell me twice?  (Or not.)

[mmenke, 2015/01/22 19:58:33]

Rather than tell you twice, which is something I prefer not to do unless I feel
it's really needed, I'll tell you my reasoning, and then let you decide.

My feeling is that docs are more generally above classes than in one place like
this, and that's where Chromites have learned to look first, particularly when
navigating through files based on text search (As opposed to opening files and
reading them top to bottom, which I, at least, generally don't do, except for
headers).

In header files, I know that I always miss docs just under the copyright header,
and I've also been known to miss comments just above namespace declarations.  I
have less experience missing comments in those places in CC files, but I suspect
that's because comments in those places are less common in CC files.

[Randy Smith (Not in Mondays), 2015/01/24 23:21:01]

Thinking about it, the thing I care the most about is that this stays together
as a comment, as opposed to being broken up into different places in the file. 
It sounds like your concerns are primarily about discoverability, so I'll put
this entire comment just above the SdchDictionaryFetcherTest class.  (The reason
I didn't do that originally is that it's about the entire file, but I'm happy to
have it there if it means people will find it.)  Let me know if you have any
objection to that compromise.

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

Works for me.

+// Most tests schedule a dictionary fetch, wait for no jobs outstanding,
+// then test that the fetch results are as expected.
+
 namespace net {
 
 static const char* kSampleBufferContext = "This is a sample buffer.";
@@ -27,7 +40,9 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
  public:
   URLRequestSpecifiedResponseJob(URLRequest* request,
                                  NetworkDelegate* network_delegate)
-      : URLRequestSimpleJob(request, network_delegate) {}
+      : URLRequestSimpleJob(request, network_delegate) {
+    ++jobs_outstanding_;
+  }
 
   static void AddUrlHandler() {
     net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
@@ -58,8 +73,24 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
 
   static int jobs_requested() { return jobs_requested_; }
 
+  // |*callback| will be invoked when the total number of outstanding
+  // jobs drops to zero.
+  // Ownership of |*callback| remains with caller; |*callback|
+  // must be kept alive as long as this variable has a pointer to
+  // it.
+  static void SetSignalOnNoJobs(base::Closure* callback) {
+    signal_on_no_jobs_ = callback;
+  }
+
+  static int jobs_outstanding() { return jobs_outstanding_; }
+
  private:
-  ~URLRequestSpecifiedResponseJob() override{};
+  ~URLRequestSpecifiedResponseJob() override {
+    --jobs_outstanding_;
+    if (jobs_outstanding_ == 0 && signal_on_no_jobs_)
+      signal_on_no_jobs_->Run();
+  }
+
   int GetData(std::string* mime_type,
               std::string* charset,
               std::string* data,
@@ -69,11 +100,17 @@ class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {
     return OK;
   }
 
+  static base::Closure* signal_on_no_jobs_;
   static int jobs_requested_;
+  static int jobs_outstanding_;

[mmenke, 2015/01/21 21:33:57]

I'd advise against using non-const static variables, it just seems ugly and bug
prone.  If you're running the tests multiple times without restarting the test
fixture, if gets you into trouble.

I notice that the old code sets jobs_requested_ to 0 to allow for that, but your
code doesn't seem to.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:39]

Good point, and maybe I'm just having a braino, but what's my alternative?  I
want to have a blocking point when the number of outstanding jobs goes to zero,
which means I need to record creation and destruction of those jobs in some
mutable variable that isn't part of a URLRequestSpecifiedResponseJob instance. 
If that's not a static of some sort (e.g. a member variable in the test fixture)
then it needs to be registered with URLRequestSpecifiedResponseJob in some
fashion, and that registration information will be in a static.  What am I
missing?  

(This is presuming I don't want to use RunUntilIdle(), which I don't but could
be convinced to.  But this doesn't seem like the right argument for that.)  
*confusion*  I'm not seeing where my old code set jobs_requested_ to zero,
unless you mean in AddUrlHandler(), in which case it still does.  WRT
jobs_outstanding_, I'm going to claim that if jobs_outstanding_ is non-zero
between tests, resetting it won't help, because that indicates that there's
other global state (the actual outstanding job) that could interfere with the
new test.  I can put in an assert if you'd like.

[mmenke, 2015/01/22 19:58:33]

I'm fine with avoiding RunUntilIdle, like this (I'm not sure the cure is better
than the disease in this case, but I'm not sure it's worse, either, so I'm happy
to defer to your preference).  One option is to use a UrlRequestInterceptor
class, which the URLRequestFilter can also take as an argument, and have it
track the values instead.  Then just keep a raw pointer to the interceptor, and
get the values that way.
That's a valid point, and I suppose you're also setting signal_on_no_jobs_ on
the test's constructor...But relying on all that just seems very icky,
particularly since you have 3 class statics...And they're all initialized in
completely different places.  Does not strike me as a very safe pattern.

[mmenke, 2015/01/24 17:01:24]

And a quick followup...  If keeping around an unowned pointer bothers you, could
either use a weak pointer, or have a shared struct the intercept writes to, and
the test fixture reads from.

[Randy Smith (Not in Mondays), 2015/01/24 23:21:01]

I'm good with your original suggestion, and it didn't require keeping around an
unowned pointer (I just passed a callback with a weak pointer in at construction
time).  For some reason, I wasn't clueing into the possibilities inherent in
intercepting construction from the instantiation side.

 };
 
+base::Closure* URLRequestSpecifiedResponseJob::signal_on_no_jobs_(nullptr);
+
 int URLRequestSpecifiedResponseJob::jobs_requested_(0);
 
+int URLRequestSpecifiedResponseJob::jobs_outstanding_(0);
+
 class SdchDictionaryFetcherTest : public ::testing::Test {
  public:
   struct DictionaryAdditions {
@@ -85,17 +122,26 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
     GURL dictionary_url;
   };
 
-  SdchDictionaryFetcherTest() {
+  SdchDictionaryFetcherTest()
+      : waiting_for_no_jobs_(false),
+        run_loop_(nullptr),
+        no_jobs_seen_callback_(
+            base::Bind(&SdchDictionaryFetcherTest::NotifyNoJobs,
+                       base::Unretained(this))) {
     URLRequestSpecifiedResponseJob::AddUrlHandler();
     context_.reset(new TestURLRequestContext);
     fetcher_.reset(new SdchDictionaryFetcher(
         context_.get(),
         base::Bind(&SdchDictionaryFetcherTest::OnDictionaryFetched,
                    base::Unretained(this))));
+    URLRequestSpecifiedResponseJob::SetSignalOnNoJobs(
+        // Callback will be reset before destruction completion.
+        &no_jobs_seen_callback_);
   }
 
   ~SdchDictionaryFetcherTest() override {
     URLRequestSpecifiedResponseJob::RemoveUrlHandler();
+    URLRequestSpecifiedResponseJob::SetSignalOnNoJobs(nullptr);
   }
 
   void OnDictionaryFetched(const std::string& dictionary_text,
@@ -125,18 +171,41 @@ class SdchDictionaryFetcherTest : public ::testing::Test {
     return GURL(gurl_string);
   }
 
+  // Block until there are no outstanding URLRequestSpecifiedResponseJobs.
+  void WaitForNoJobs() {
+    if (URLRequestSpecifiedResponseJob::jobs_outstanding() == 0)
+      return;
+
+    base::RunLoop run_loop;
+    run_loop_ = &run_loop;

[mmenke, 2015/01/21 21:33:57]

Suggest just a scoped_ptr<RunLoop> in the class - seems cleaner to me.  Can
either reset it or not at the end of this method - I'm fine either way.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:40]

Done.

+    DCHECK(!waiting_for_no_jobs_);
+    waiting_for_no_jobs_ = true;
+    run_loop.Run();
+    run_loop_ = nullptr;
+    waiting_for_no_jobs_ = false;
+  }
+
  private:
+  void NotifyNoJobs() {
+    if (waiting_for_no_jobs_)

[mmenke, 2015/01/21 21:33:57]

Any case where this is not the same as run_loop_ != NULL?  Can certainly debate
over which is cleaner, but I feel that having to both clear run_loop_ and set
waiting_for_no_jobs_ to false outweighs the code clarity of having the book to
check.

[Randy Smith (Not in Mondays), 2015/01/22 19:04:40]

CL history I should have cleaned up--I used to have two signals that this class
could produce.  Shifting over to just using run_loop_.

+      run_loop_->Quit();
+  }
+
+  bool waiting_for_no_jobs_;
+  base::RunLoop* run_loop_;
+
   scoped_ptr<TestURLRequestContext> context_;
   scoped_ptr<SdchDictionaryFetcher> fetcher_;
   std::vector<DictionaryAdditions> dictionary_additions;
+  base::Closure no_jobs_seen_callback_;
 };
 
 // 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());
   std::vector<DictionaryAdditions> additions;
   GetDictionaryAdditions(&additions);
@@ -152,7 +221,7 @@ TEST_F(SdchDictionaryFetcherTest, Multiple) {
   fetcher()->Schedule(dictionary_url);
   fetcher()->Schedule(dictionary_url);
   fetcher()->Schedule(dictionary_url);
-  base::RunLoop().RunUntilIdle();
+  WaitForNoJobs();
 
   EXPECT_EQ(1, JobsRequested());
   std::vector<DictionaryAdditions> additions;
@@ -173,9 +242,30 @@ TEST_F(SdchDictionaryFetcherTest, Cancel) {
   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());
 }
+
+// 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"));
+  GURL dictionary1_url(PathToGurl("dictionary1"));
+  fetcher()->Schedule(dictionary_url);
+  fetcher()->Schedule(dictionary1_url);
+  WaitForNoJobs();
+
+  EXPECT_EQ(2, JobsRequested());
+  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);
+}
 }

[mmenke, 2015/01/21 21:33:57]

// net (And there should be a linebreak before the end of the namespace)

[Randy Smith (Not in Mondays), 2015/01/22 19:04:39]

Whoops.  Done.