Rework visited link updating mechanism to be more robust.

This is a follow-up to http://src.chromium.org/viewvc/chrome?view=rev&revision=22540,
which eliminated one crash, but still left behind another, this time due to
racing between history backend and WebView creation.

The solution is to make sure no visited-link-related messages are sent to the
rendering process until we know for sure it is created.

BUG=17555
TEST=VisitedLinkeRelayTest.WebViewReadiness
R=darin


Review URL: http://codereview.chromium.org/165210

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

3 files changed, 69 insertions, 24 deletions

diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index 1192996..0fc5e05 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -135,37 +135,48 @@ static const unsigned kVisitedLinkBufferThreshold = 50;
 // This class manages buffering and sending visited link hashes (fingerprints)
 // to renderer based on widget visibility.
 // As opposed to the VisitedLinkEventListener in profile.cc, which coalesces to
-// reduce the rate of messages being send to render processes, this class
+// reduce the rate of messages being sent to render processes, this class
 // ensures that the updates occur only when explicitly requested. This is
 // used by BrowserRenderProcessHost to only send Add/Reset link events to the
-// renderers when their tabs are visible.
+// renderers when their tabs are visible and the corresponding RenderViews are
+// created.
 class VisitedLinkUpdater {
  public:
-  VisitedLinkUpdater() : threshold_reached_(false) {}
+  VisitedLinkUpdater() : reset_needed_(false), has_receiver_(false) {}
 
-  void Buffer(const VisitedLinkCommon::Fingerprints& links) {
-    if (threshold_reached_)
+  // Buffers |links| to update, but doesn't actually relay them.
+  void AddLinks(const VisitedLinkCommon::Fingerprints& links) {
+    if (reset_needed_)
       return;
 
     if (pending_.size() + links.size() > kVisitedLinkBufferThreshold) {
-      threshold_reached_ = true;
       // Once the threshold is reached, there's no need to store pending visited
-      // links.
-      pending_.clear();
+      // link updates -- we opt for resetting the state for all links.
+      AddReset();
       return;
     }
 
     pending_.insert(pending_.end(), links.begin(), links.end());
   }
 
-  void Clear() {
+  // Tells the updater that sending individual link updates is no longer
+  // necessary and the visited state for all links should be reset.
+  void AddReset() {
+    reset_needed_ = true;
     pending_.clear();
   }
 
+  // Sends visited link update messages: a list of links whose visited state
+  // changed or reset of visited state for all links.
   void Update(IPC::Channel::Sender* sender) {
-    if (threshold_reached_) {
+    DCHECK(sender);
+
+    if (!has_receiver_)
+      return;
+
+    if (reset_needed_) {
       sender->Send(new ViewMsg_VisitedLink_Reset());
-      threshold_reached_ = false;
+      reset_needed_ = false;
       return;
     }
 
@@ -177,8 +188,16 @@ class VisitedLinkUpdater {
     pending_.clear();
   }
 
+  // Notifies the updater that it is now safe to send visited state updates.
+  void ReceiverReady(IPC::Channel::Sender* sender) {
+    has_receiver_ = true;
+    // Go ahead and send whatever we already have buffered up.
+    Update(sender);
+  }
+
  private:
-  bool threshold_reached_;
+  bool reset_needed_;
+  bool has_receiver_;
   VisitedLinkCommon::Fingerprints pending_;
 };
 
@@ -196,7 +215,6 @@ BrowserRenderProcessHost::BrowserRenderProcessHost(Profile* profile)
     : RenderProcessHost(profile),
       visible_widgets_(0),
       backgrounded_(true),
-      view_created_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(cached_dibs_cleaner_(
             base::TimeDelta::FromSeconds(5),
             this, &BrowserRenderProcessHost::ClearTransportDIBCache)),
@@ -524,16 +542,14 @@ void BrowserRenderProcessHost::ReceivedBadMessage(uint16 msg_type) {
 }
 
 void BrowserRenderProcessHost::ViewCreated() {
-  view_created_ = true;
-  visited_link_updater_->Update(this);
+  visited_link_updater_->ReceiverReady(this);
 }
 
 void BrowserRenderProcessHost::WidgetRestored() {
   // Verify we were properly backgrounded.
   DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
   visible_widgets_++;
-  if (view_created_)
-    visited_link_updater_->Update(this);
+  visited_link_updater_->Update(this);
   SetBackgrounded(false);
 }
 
@@ -561,7 +577,7 @@ void BrowserRenderProcessHost::AddWord(const std::wstring& word) {
 
 void BrowserRenderProcessHost::AddVisitedLinks(
     const VisitedLinkCommon::Fingerprints& links) {
-  visited_link_updater_->Buffer(links);
+  visited_link_updater_->AddLinks(links);
   if (visible_widgets_ == 0)
     return;
 
@@ -569,8 +585,11 @@ void BrowserRenderProcessHost::AddVisitedLinks(
 }
 
 void BrowserRenderProcessHost::ResetVisitedLinks() {
-  visited_link_updater_->Clear();
-  Send(new ViewMsg_VisitedLink_Reset());
+  visited_link_updater_->AddReset();
+  if (visible_widgets_ == 0)
+    return;
+
+  visited_link_updater_->Update(this);
 }
 
 base::ProcessHandle BrowserRenderProcessHost::GetRendererProcessHandle() {
diff --git a/chrome/browser/renderer_host/browser_render_process_host.h b/chrome/browser/renderer_host/browser_render_process_host.h
index 98a3d09..b8a5c54 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.h
+++ b/chrome/browser/renderer_host/browser_render_process_host.h
@@ -136,9 +136,6 @@ class BrowserRenderProcessHost : public RenderProcessHost,
   // Does this process have backgrounded priority.
   bool backgrounded_;
 
-  // Is true only when the process had a render view created.
-  bool view_created_;
-
   // Used to allow a RenderWidgetHost to intercept various messages on the
   // IO thread.
   scoped_refptr<RenderWidgetHelper> widget_helper_;
diff --git a/chrome/browser/visitedlink_unittest.cc b/chrome/browser/visitedlink_unittest.cc
index d6735ae..8775f9e 100644
--- a/chrome/browser/visitedlink_unittest.cc
+++ b/chrome/browser/visitedlink_unittest.cc
@@ -565,7 +565,6 @@ class VisitedLinkEventsTest : public RenderViewHostTestHarness {
     rvh_factory_.set_render_process_host_factory(&vc_rph_factory_);
     profile_.reset(new VisitCountingProfile(event_listener_.get()));
     RenderViewHostTestHarness::SetUp();
-    rvh()->CreateRenderView();
   }
 
   VisitCountingProfile* profile() const {
@@ -648,6 +647,8 @@ TEST_F(VisitedLinkEventsTest, Coalescense) {
 
 TEST_F(VisitedLinkRelayTest, Basics) {
   VisitedLinkMaster* master = profile_->GetVisitedLinkMaster();
+  rvh()->CreateRenderView();
+
   // Add a few URLs.
   master->AddURL(GURL("http://acidtests.org/"));
   master->AddURL(GURL("http://google.com/"));
@@ -670,6 +671,7 @@ TEST_F(VisitedLinkRelayTest, Basics) {
 
 TEST_F(VisitedLinkRelayTest, TabVisibility) {
   VisitedLinkMaster* master = profile_->GetVisitedLinkMaster();
+  rvh()->CreateRenderView();
 
   // Simulate tab becoming inactive.
   rvh()->WasHidden();
@@ -712,3 +714,30 @@ TEST_F(VisitedLinkRelayTest, TabVisibility) {
   EXPECT_EQ(1, profile()->add_event_count());
   EXPECT_EQ(1, profile()->reset_event_count());
 }
+
+TEST_F(VisitedLinkRelayTest, WebViewReadiness) {
+  VisitedLinkMaster* master = profile_->GetVisitedLinkMaster();
+
+  // Add a few URLs.
+  master->AddURL(GURL("http://acidtests.org/"));
+  master->AddURL(GURL("http://google.com/"));
+  master->AddURL(GURL("http://chromium.org/"));
+
+  WaitForCoalescense();
+
+  std::set<GURL> deleted_urls;
+  deleted_urls.insert(GURL("http://acidtests.org/"));
+  master->DeleteURLs(deleted_urls);
+
+  // We shouldn't have any events, because RenderView hasn't been created,
+  // and we ensure that updates are sent until it is.
+  EXPECT_EQ(0, profile()->add_event_count());
+  EXPECT_EQ(0, profile()->reset_event_count());
+
+  rvh()->CreateRenderView();
+
+  // We should now have just a reset event: adds are eaten up by a reset
+  // that followed.
+  EXPECT_EQ(0, profile()->add_event_count());
+  EXPECT_EQ(1, profile()->reset_event_count());
+}