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/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"
 
+// 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.";
 static const char* kTestDomain = "top.domain.test";

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

nit:  While you're here, "static const char kBlah[]" seems to be preferred.  I
assume because the "const" is better enforced that way.

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

Done.

 
 class URLRequestSpecifiedResponseJob : public URLRequestSimpleJob {

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

This should be in an anonymous namespace, and the above statics can be moved in
as well, remove the statics.

I'd suggest sticking this entire file in, actually.

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

Done, but I'd love to get a bit more guidance or pointers on idiom, if you have
them.  The reason I originally did it this was was the (lazy) urge not to have
to put net:: everywhere.  Switching over to using an anonymous namespace
(without any nesting) results in having to put in namespace qualifiers.  I could
nest an anonymous namespace inside of a net:: namespace, but that seems a bit
ugly, and "using net::" violates the style guide.  Do you have a sense as to the
best idiom?  (I'm perfectly willing to believe it's "anonymous, use net::
everywhere, and suck it up", it's just a shame to have to do that in something
that's conceptually a net/ file.)

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

You don't need namespace qualifiers with an anonymous namespace nested in a net
namespace.  The best practice for net tests, in my opinion, is just to have a
net namespace wrapping an anonymous namespace wrapping the entire file (Some
people prefer to end the anonymous namespace just above the test fixture - test
fixtures can't share names, anyways.  I prefer not to do that, because people
can declare classes below the test fixture, and because it discourages friending
tests (If it's really needed, I'm fine with that, but if not, I prefer to avoid
it).

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

Sounds good.  I'll adopt the whole file double nest idiom, with modifiers for
when tests need to be friended.

  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();
     jobs_requested_ = 0;
     filter->AddHostnameHandler(
         "http", kTestDomain, &URLRequestSpecifiedResponseJob::Factory);
   }
 
   static void RemoveUrlHandler() {
     net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
@@ skipping 10 matching lines @@
 
   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_; }
 
+  // |*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,
               const CompletionCallback& callback) const override {
     GURL url(request_->url());
     *data = ExpectedResponseForURL(url);
     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 {
     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() {
+  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,
                            const GURL& dictionary_url,
                            const BoundNetLog& net_log) {
     dictionary_additions.push_back(
         DictionaryAdditions(dictionary_text, dictionary_url));
   }
 
   void GetDictionaryAdditions(std::vector<DictionaryAdditions>* out) {
     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();
   }
 
   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 (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);
   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());
   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());
 }
+
+// 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.