Turn GoogleURLTrackerNavigationHelper(Impl) into a per-tab object.

chrome/browser/google/google_url_tracker_unittest.cc

 // Copyright (c) 2012 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 "chrome/browser/google/google_url_tracker.h"
 
 #include <set>
 #include <string>
 
 #include "base/message_loop/message_loop.h"
@@ skipping 119 matching lines @@
 }
 
 void TestGoogleURLTrackerClient::SetListeningForNavigationStart(bool listen) {
   observe_nav_start_ = listen;
 }
 
 bool TestGoogleURLTrackerClient::IsListeningForNavigationStart() {
   return observe_nav_start_;
 }
 
+
 // TestGoogleURLTrackerNavigationHelper ---------------------------------------
 
 class TestGoogleURLTrackerNavigationHelper
     : public GoogleURLTrackerNavigationHelper {
  public:
-  TestGoogleURLTrackerNavigationHelper();
+  explicit TestGoogleURLTrackerNavigationHelper(GoogleURLTracker* tracker);
   virtual ~TestGoogleURLTrackerNavigationHelper();
 
-  virtual void SetGoogleURLTracker(GoogleURLTracker* tracker) OVERRIDE;
-  virtual void SetListeningForNavigationCommit(
-      const content::NavigationController* nav_controller,
-      bool listen) OVERRIDE;
-  virtual bool IsListeningForNavigationCommit(
-      const content::NavigationController* nav_controller) OVERRIDE;
-  virtual void SetListeningForTabDestruction(
-      const content::NavigationController* nav_controller,
-      bool listen) OVERRIDE;
-  virtual bool IsListeningForTabDestruction(
-      const content::NavigationController* nav_controller) OVERRIDE;
+  virtual void SetListeningForNavigationCommit(bool listen) OVERRIDE;
+  virtual bool IsListeningForNavigationCommit() OVERRIDE;
+  virtual void SetListeningForTabDestruction(bool listen) OVERRIDE;
+  virtual bool IsListeningForTabDestruction() OVERRIDE;
 
  private:
-  GoogleURLTracker* tracker_;
-  std::set<const content::NavigationController*>
-      nav_controller_commit_listeners_;
-  std::set<const content::NavigationController*>
-      nav_controller_tab_close_listeners_;
+  bool listening_for_nav_commit_;
+  bool listening_for_tab_destruction_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestGoogleURLTrackerNavigationHelper);
 };
 
-TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper()
-    : tracker_(NULL) {
+TestGoogleURLTrackerNavigationHelper::TestGoogleURLTrackerNavigationHelper(
+    GoogleURLTracker* tracker)
+    : GoogleURLTrackerNavigationHelper(tracker),
+      listening_for_nav_commit_(false),
+      listening_for_tab_destruction_(false) {
 }
 
-TestGoogleURLTrackerNavigationHelper::
-    ~TestGoogleURLTrackerNavigationHelper() {
-}
-
-void TestGoogleURLTrackerNavigationHelper::SetGoogleURLTracker(
-    GoogleURLTracker* tracker) {
-  tracker_ = tracker;
+TestGoogleURLTrackerNavigationHelper::~TestGoogleURLTrackerNavigationHelper() {
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetListeningForNavigationCommit(
-    const content::NavigationController* nav_controller,
     bool listen) {
-  if (listen)
-    nav_controller_commit_listeners_.insert(nav_controller);
-  else
-    nav_controller_commit_listeners_.erase(nav_controller);
+  listening_for_nav_commit_ = listen;
 }
 
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit(
-    const content::NavigationController* nav_controller) {
-  return nav_controller_commit_listeners_.count(nav_controller) > 0;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForNavigationCommit() {
+  return listening_for_nav_commit_;
 }
 
 void TestGoogleURLTrackerNavigationHelper::SetListeningForTabDestruction(
-    const content::NavigationController* nav_controller,
     bool listen) {
-  if (listen)
-    nav_controller_tab_close_listeners_.insert(nav_controller);
-  else
-    nav_controller_tab_close_listeners_.erase(nav_controller);
+  listening_for_tab_destruction_ = listen;
 }
 
-bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction(
-    const content::NavigationController* nav_controller) {
-  return nav_controller_tab_close_listeners_.count(nav_controller) > 0;
+bool TestGoogleURLTrackerNavigationHelper::IsListeningForTabDestruction() {
+  return listening_for_tab_destruction_;
 }
 
 }  // namespace
 
 
 // GoogleURLTrackerTest -------------------------------------------------------
 
 // Ths class exercises GoogleURLTracker.  In order to avoid instantiating more
 // of the Chrome infrastructure than necessary, the GoogleURLTracker functions
 // are carefully written so that many of the functions which take
@@ skipping 36 matching lines @@
   }
   GURL google_url() const { return google_url_tracker_->google_url(); }
   void SetLastPromptedGoogleURL(const GURL& url);
   GURL GetLastPromptedGoogleURL();
   void SetNavigationPending(intptr_t unique_id, bool is_search);
   void CommitNonSearch(intptr_t unique_id);
   void CommitSearch(intptr_t unique_id, const GURL& search_url);
   void CloseTab(intptr_t unique_id);
   GoogleURLTrackerMapEntry* GetMapEntry(intptr_t unique_id);
   GoogleURLTrackerInfoBarDelegate* GetInfoBarDelegate(intptr_t unique_id);
+  GoogleURLTrackerNavigationHelper* GetNavigationHelper(intptr_t unique_id);
   void ExpectDefaultURLs() const;
   void ExpectListeningForCommit(intptr_t unique_id, bool listening);
   bool listener_notified() const { return listener_.notified(); }
   void clear_listener_notified() { listener_.clear_notified(); }
 
  private:
   // Since |infobar_service| is really a magic number rather than an actual
   // object, we don't add the created infobar to it.  Instead we will simulate
   // any helper<->infobar interaction necessary.  The returned object will be
   // cleaned up in OnInfoBarClosed().
   infobars::InfoBar* CreateTestInfoBar(InfoBarService* infobar_service,
                                        GoogleURLTracker* google_url_tracker,
                                        const GURL& search_url);
 
   // These are required by the TestURLFetchers GoogleURLTracker will create (see
   // test_url_fetcher_factory.h).
   content::TestBrowserThreadBundle thread_bundle_;
   // Creating this allows us to call
   // net::NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests().
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   net::TestURLFetcherFactory fetcher_factory_;
   GoogleURLTrackerClient* client_;
-  GoogleURLTrackerNavigationHelper* nav_helper_;
   TestingProfile profile_;
   scoped_ptr<GoogleURLTracker> google_url_tracker_;
   TestCallbackListener listener_;
   // This tracks the different "tabs" a test has "opened", so we can close them
   // properly before shutting down |google_url_tracker_|, which expects that.
   std::set<int> unique_ids_seen_;
 };
 
 void GoogleURLTrackerTest::OnInfoBarClosed(
     scoped_ptr<infobars::InfoBar> infobar,
@@ skipping 18 matching lines @@
     : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {
   GoogleURLTrackerFactory::GetInstance()->
       RegisterUserPrefsOnBrowserContextForTest(&profile_);
 }
 
 GoogleURLTrackerTest::~GoogleURLTrackerTest() {
 }
 
 void GoogleURLTrackerTest::SetUp() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
-  // Ownership is passed to google_url_tracker_, but weak pointers are kept;
-  // this is safe since GoogleURLTracker keeps these objects for its lifetime.
+  // Ownership is passed to google_url_tracker_, but a weak pointer is kept;
+  // this is safe since GoogleURLTracker keeps the client for its lifetime.
   client_ = new TestGoogleURLTrackerClient();
-  nav_helper_ = new TestGoogleURLTrackerNavigationHelper();
   scoped_ptr<GoogleURLTrackerClient> client(client_);
-  scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper(nav_helper_);
-  google_url_tracker_.reset(
-      new GoogleURLTracker(&profile_,
-                           client.Pass(),
-                           nav_helper.Pass(),
-                           GoogleURLTracker::UNIT_TEST_MODE));
+  google_url_tracker_.reset(new GoogleURLTracker(
+      &profile_, client.Pass(), GoogleURLTracker::UNIT_TEST_MODE));
   google_url_tracker_->infobar_creator_ = base::Bind(
       &GoogleURLTrackerTest::CreateTestInfoBar, base::Unretained(this));
 }
 
 void GoogleURLTrackerTest::TearDown() {
   while (!unique_ids_seen_.empty())
     CloseTab(*unique_ids_seen_.begin());
+  google_url_tracker_->Shutdown();

[blundell, 2014/05/26 12:19:34]

This is the fix, which fixes a latent bug that this CL tickled. GoogleURLTracker
was never removing itself as an IPAddressObserver in these unittests. Another
object adds itself as an IPAddressObserver during TestingProfile shutdown after
the deallocation of the GoogleURLTracker instance, resulting in an ObserverList
DCHECK firing if the address of this second object aliased the address of the
deallocated GoogleURLTracker instance. The changes that my CL made around memory
allocation brought this bug up from the depths.

[Peter Kasting, 2014/05/28 19:18:50]

Is there some way to use the service factory system to instantiate the
GoogleURLTracker object in SetUp(), instead of constructing one manually, so
that Shutdown() gets called automatically?  That seems both less error-prone and
more like the normal control flow (and thus probably better to test).

[blundell, 2014/05/28 19:19:51]

I'm going to be componentizing this unittest whereas the factory will stay in
//chrome.

On 2014/05/28 19:18:50, Peter Kasting wrote:

 }
 
 net::TestURLFetcher* GoogleURLTrackerTest::GetFetcher() {
   // This will return the last fetcher created.  If no fetchers have been
   // created, we'll pass GetFetcherByID() "-1", and it will return NULL.
   return fetcher_factory_.GetFetcherByID(google_url_tracker_->fetcher_id_ - 1);
 }
 
 void GoogleURLTrackerTest::MockSearchDomainCheckResponse(
     const std::string& domain) {
@@ skipping 36 matching lines @@
 void GoogleURLTrackerTest::SetNavigationPending(intptr_t unique_id,
                                                 bool is_search) {
   if (is_search) {
     google_url_tracker_->SearchCommitted();
     // Note that the call above might not have actually registered a listener
     // for navigation starts if the searchdomaincheck response was bogus.
   }
   unique_ids_seen_.insert(unique_id);
   if (client_->IsListeningForNavigationStart()) {
     google_url_tracker_->OnNavigationPending(
-        reinterpret_cast<content::NavigationController*>(unique_id),
-        reinterpret_cast<InfoBarService*>(unique_id), unique_id);
+        scoped_ptr<GoogleURLTrackerNavigationHelper>(
+            new TestGoogleURLTrackerNavigationHelper(
+                google_url_tracker_.get())),
+        reinterpret_cast<InfoBarService*>(unique_id),
+        unique_id);
   }
 }
 
 void GoogleURLTrackerTest::CommitNonSearch(intptr_t unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (!map_entry)
     return;
 
   ExpectListeningForCommit(unique_id, false);
 
   // The infobar should be showing; otherwise the pending non-search should
   // have closed it.
   ASSERT_TRUE(map_entry->has_infobar_delegate());
 
   // The pending_id should have been reset to 0 when the non-search became
   // pending.
   EXPECT_EQ(0, map_entry->infobar_delegate()->pending_id());
 
   // Committing the navigation would close the infobar.
   map_entry->infobar_delegate()->Close(false);
 }
 
 void GoogleURLTrackerTest::CommitSearch(intptr_t unique_id,
                                         const GURL& search_url) {
   DCHECK(search_url.is_valid());
-  if (nav_helper_->IsListeningForNavigationCommit(
-      reinterpret_cast<content::NavigationController*>(unique_id))) {
+  GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
+  if (nav_helper && nav_helper->IsListeningForNavigationCommit()) {
     google_url_tracker_->OnNavigationCommitted(
         reinterpret_cast<InfoBarService*>(unique_id), search_url);
   }
 }
 
 void GoogleURLTrackerTest::CloseTab(intptr_t unique_id) {
   unique_ids_seen_.erase(unique_id);
-  content::NavigationController* nav_controller =
-      reinterpret_cast<content::NavigationController*>(unique_id);
-  if (nav_helper_->IsListeningForTabDestruction(nav_controller)) {
-    google_url_tracker_->OnTabClosed(nav_controller);
+  GoogleURLTrackerNavigationHelper* nav_helper = GetNavigationHelper(unique_id);
+  if (nav_helper && nav_helper->IsListeningForTabDestruction()) {
+    google_url_tracker_->OnTabClosed(nav_helper);
   } else {
     // Closing a tab with an infobar showing would close the infobar.
     GoogleURLTrackerInfoBarDelegate* delegate = GetInfoBarDelegate(unique_id);
     if (delegate)
       delegate->Close(false);
   }
 }
 
 GoogleURLTrackerMapEntry* GoogleURLTrackerTest::GetMapEntry(
     intptr_t unique_id) {
   GoogleURLTracker::EntryMap::const_iterator i =
       google_url_tracker_->entry_map_.find(
           reinterpret_cast<InfoBarService*>(unique_id));
   return (i == google_url_tracker_->entry_map_.end()) ? NULL : i->second;
 }
 
 GoogleURLTrackerInfoBarDelegate* GoogleURLTrackerTest::GetInfoBarDelegate(
     intptr_t unique_id) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   return map_entry ? map_entry->infobar_delegate() : NULL;
 }
 
+GoogleURLTrackerNavigationHelper* GoogleURLTrackerTest::GetNavigationHelper(
+    intptr_t unique_id) {
+  GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
+  return map_entry ? map_entry->navigation_helper() : NULL;
+}
+
 void GoogleURLTrackerTest::ExpectDefaultURLs() const {
   EXPECT_EQ(GURL(GoogleURLTracker::kDefaultGoogleHomepage), google_url());
   EXPECT_EQ(GURL(), fetched_google_url());
 }
 
 void GoogleURLTrackerTest::ExpectListeningForCommit(intptr_t unique_id,
                                                     bool listening) {
   GoogleURLTrackerMapEntry* map_entry = GetMapEntry(unique_id);
   if (map_entry) {
-    EXPECT_EQ(listening, nav_helper_->IsListeningForNavigationCommit(
-        map_entry->navigation_controller()));
+    EXPECT_EQ(listening,
+              map_entry->navigation_helper()->IsListeningForNavigationCommit());
   } else {
     EXPECT_FALSE(listening);
   }
 }
 
 infobars::InfoBar* GoogleURLTrackerTest::CreateTestInfoBar(
     InfoBarService* infobar_service,
     GoogleURLTracker* google_url_tracker,
     const GURL& search_url) {
   return TestInfoBarDelegate::Create(this, infobar_service, google_url_tracker,
@@ skipping 622 matching lines @@
   CommitSearch(2, GURL("http://www.google.co.uk/search?q=test2"));
   EXPECT_FALSE(GetInfoBarDelegate(1) == NULL);
   GoogleURLTrackerInfoBarDelegate* delegate2 = GetInfoBarDelegate(2);
   ASSERT_FALSE(delegate2 == NULL);
   SetNavigationPending(1, true);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, true));
   delegate2->Close(false);
   SetNavigationPending(1, false);
   ASSERT_NO_FATAL_FAILURE(ExpectListeningForCommit(1, false));
 }