Make RenderView not have to know about how PhishingClassifierDelegate.

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

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

10 files changed, 170 insertions, 169 deletions

diff --git a/chrome/renderer/render_thread.cc b/chrome/renderer/render_thread.cc
index 8cd0500..6b01036 100644
--- a/chrome/renderer/render_thread.cc
+++ b/chrome/renderer/render_thread.cc
@@ -16,7 +16,6 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/stats_table.h"
 #include "base/process_util.h"
-#include "base/scoped_callback_factory.h"
 #include "base/shared_memory.h"
 #include "base/string_util.h"
 #include "base/task.h"
@@ -69,7 +68,6 @@
 #include "chrome/renderer/renderer_webidbfactory_impl.h"
 #include "chrome/renderer/renderer_webkitclient_impl.h"
 #include "chrome/renderer/safe_browsing/phishing_classifier_delegate.h"
-#include "chrome/renderer/safe_browsing/scorer.h"
 #include "chrome/renderer/search_extension.h"
 #include "chrome/renderer/searchbox_extension.h"
 #include "chrome/renderer/spellchecker/spellcheck.h"
@@ -217,26 +215,6 @@ class RenderViewZoomer : public RenderViewVisitor {
   DISALLOW_COPY_AND_ASSIGN(RenderViewZoomer);
 };
 
-class RenderViewPhishingScorerSetter : public RenderViewVisitor {
- public:
-  explicit RenderViewPhishingScorerSetter(const safe_browsing::Scorer* scorer)
-      : scorer_(scorer) {
-  }
-
-  virtual bool Visit(RenderView* render_view) {
-    safe_browsing::PhishingClassifierDelegate* delegate =
-        render_view->phishing_classifier_delegate();
-    if (delegate)
-      delegate->SetPhishingScorer(scorer_);
-    return true;
-  }
-
- private:
-  const safe_browsing::Scorer* scorer_;
-
-  DISALLOW_COPY_AND_ASSIGN(RenderViewPhishingScorerSetter);
-};
-
 }  // namespace
 
 // When we run plugins in process, we actually run them on the render thread,
@@ -277,7 +255,6 @@ void RenderThread::Init() {
   idle_notification_delay_in_s_ = is_extension_process_ ?
       kInitialExtensionIdleHandlerDelayS : kInitialIdleHandlerDelayS;
   task_factory_.reset(new ScopedRunnableMethodFactory<RenderThread>(this));
-  callback_factory_.reset(new base::ScopedCallbackFactory<RenderThread>(this));
 
   visited_link_slave_.reset(new VisitedLinkSlave());
   user_script_slave_.reset(new UserScriptSlave(&extensions_));
@@ -1114,20 +1091,7 @@ void RenderThread::OnGpuChannelEstablished(
 }
 
 void RenderThread::OnSetPhishingModel(IPC::PlatformFileForTransit model_file) {
-  safe_browsing::Scorer::CreateFromFile(
-      IPC::PlatformFileForTransitToPlatformFile(model_file),
-      GetFileThreadMessageLoopProxy(),
-      callback_factory_->NewCallback(&RenderThread::PhishingScorerCreated));
-}
-
-void RenderThread::PhishingScorerCreated(safe_browsing::Scorer* scorer) {
-  if (!scorer) {
-    DLOG(ERROR) << "Unable to create a PhishingScorer - corrupt model?";
-    return;
-  }
-  phishing_scorer_.reset(scorer);
-  RenderViewPhishingScorerSetter setter(phishing_scorer_.get());
-  RenderView::ForEach(&setter);
+  safe_browsing::PhishingClassifierDelegate::SetPhishingModel(model_file);
 }
 
 scoped_refptr<base::MessageLoopProxy>
diff --git a/chrome/renderer/render_thread.h b/chrome/renderer/render_thread.h
index 07a6e68..bfb9e55 100644
--- a/chrome/renderer/render_thread.h
+++ b/chrome/renderer/render_thread.h
@@ -51,7 +51,6 @@ struct WebPreferences;
 
 namespace base {
 class MessageLoopProxy;
-template<class T> class ScopedCallbackFactory;
 class Thread;
 }
 
@@ -59,10 +58,6 @@ namespace IPC {
 struct ChannelHandle;
 }
 
-namespace safe_browsing {
-class Scorer;
-}
-
 namespace WebKit {
 class WebStorageEventDispatcher;
 }
@@ -201,12 +196,6 @@ class RenderThread : public RenderThreadBase,
     return spellchecker_.get();
   }
 
-  // Returns the phishing Scorer object, or NULL if a model has not been passed
-  // in from the browser yet.
-  const safe_browsing::Scorer* phishing_scorer() const {
-    return phishing_scorer_.get();
-  }
-
   bool plugin_refresh_allowed() const { return plugin_refresh_allowed_; }
 
   // Do DNS prefetch resolution of a hostname.
@@ -346,12 +335,8 @@ class RenderThread : public RenderThreadBase,
   // it is allowed to run on.
   void RegisterExtension(v8::Extension* extension, bool restrict_to_extensions);
 
-  // Callback to be run once the phishing Scorer has been created.
-  void PhishingScorerCreated(safe_browsing::Scorer* scorer);
-
   // These objects live solely on the render thread.
   scoped_ptr<ScopedRunnableMethodFactory<RenderThread> > task_factory_;
-  scoped_ptr<base::ScopedCallbackFactory<RenderThread> > callback_factory_;
   scoped_ptr<VisitedLinkSlave> visited_link_slave_;
   scoped_ptr<UserScriptSlave> user_script_slave_;
   scoped_ptr<RendererNetPredictor> renderer_net_predictor_;
@@ -363,7 +348,6 @@ class RenderThread : public RenderThreadBase,
   scoped_ptr<WebKit::WebStorageEventDispatcher> dom_storage_event_dispatcher_;
   scoped_ptr<WebDatabaseObserverImpl> web_database_observer_impl_;
   scoped_ptr<SpellCheck> spellchecker_;
-  scoped_ptr<const safe_browsing::Scorer> phishing_scorer_;
 
   // Used on the renderer and IPC threads.
   scoped_refptr<DBMessageFilter> db_message_filter_;
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 34c5b90..f5c808b 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -587,15 +587,6 @@ RenderView::RenderView(RenderThreadBase* render_thread,
 #endif
 
   ClearBlockedContentSettings();
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableClientSidePhishingDetection)) {
-    phishing_delegate_.reset(
-        new safe_browsing::PhishingClassifierDelegate(this, NULL));
-    RenderThread* thread = RenderThread::current();
-    if (thread && thread->phishing_scorer()) {
-      phishing_delegate_->SetPhishingScorer(thread->phishing_scorer());
-    }
-  }
 
   routing_id_ = routing_id;
   if (opener_id != MSG_ROUTING_NONE)
@@ -657,6 +648,11 @@ RenderView::RenderView(RenderThreadBase* render_thread,
   page_click_tracker->AddListener(password_autofill_manager);
   page_click_tracker->AddListener(autofill_agent);
   new TranslateHelper(this);
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableClientSidePhishingDetection)) {
+    new safe_browsing::PhishingClassifierDelegate(this, NULL);
+  }
 }
 
 RenderView::~RenderView() {
@@ -688,10 +684,6 @@ RenderView::~RenderView() {
 
   render_thread_->RemoveFilter(audio_message_filter_);
 
-  // Tell the PhishingClassifierDelegate that the view is going away.
-  if (phishing_delegate_.get())
-    phishing_delegate_->CancelPendingClassification();
-
 #ifndef NDEBUG
   // Make sure we are no longer referenced by the ViewMap.
   ViewMap* views = g_view_map.Pointer();
@@ -1249,8 +1241,7 @@ void RenderView::CapturePageInfo(int load_id, bool preliminary_capture) {
     OnCaptureThumbnail();
   }
 
-  if (phishing_delegate_.get())
-    phishing_delegate_->FinishedLoad(&contents);
+  FOR_EACH_OBSERVER(RenderViewObserver, observers_, PageCaptured(contents));
 }
 
 void RenderView::CaptureText(WebFrame* frame, string16* contents) {
@@ -2683,15 +2674,6 @@ void RenderView::show(WebNavigationPolicy policy) {
   SetPendingWindowRect(initial_pos_);
 }
 
-void RenderView::closeWidgetSoon() {
-  // Same for the phishing classifier.
-  if (phishing_delegate_.get())
-    phishing_delegate_->CancelPendingClassification();
-
-  if (script_can_close_)
-    RenderWidget::closeWidgetSoon();
-}
-
 void RenderView::runModal() {
   DCHECK(did_show_) << "should already have shown the view";
 
@@ -3304,6 +3286,9 @@ void RenderView::didStartProvisionalLoad(WebFrame* frame) {
     navigation_state->set_transition_type(PageTransition::AUTO_SUBFRAME);
   }
 
+  FOR_EACH_OBSERVER(
+      RenderViewObserver, observers_, DidStartProvisionalLoad(frame));
+
   Send(new ViewHostMsg_DidStartProvisionalLoadForFrame(
        routing_id_, frame->identifier(), is_top_most, ds->request().url()));
 }
@@ -3339,6 +3324,9 @@ void RenderView::didFailProvisionalLoad(WebFrame* frame,
 
   const WebURLRequest& failed_request = ds->request();
 
+  FOR_EACH_OBSERVER(
+      RenderViewObserver, observers_, DidFailProvisionalLoad(frame, error));
+
   bool show_repost_interstitial =
       (error.reason == net::ERR_CACHE_MISS &&
        EqualsASCII(failed_request.httpMethod(), "POST"));
@@ -3424,10 +3412,6 @@ void RenderView::didCommitProvisionalLoad(WebFrame* frame,
     // We bump our Page ID to correspond with the new session history entry.
     page_id_ = next_page_id_++;
 
-    // Let the phishing classifier decide whether to cancel classification.
-    if (phishing_delegate_.get())
-      phishing_delegate_->CommittedLoadInFrame(frame);
-
     // Advance our offset in session history, applying the length limit.  There
     // is now no forward history.
     history_list_offset_++;
@@ -3462,6 +3446,9 @@ void RenderView::didCommitProvisionalLoad(WebFrame* frame,
     }
   }
 
+  FOR_EACH_OBSERVER(RenderViewObserver, observers_,
+                    DidCommitProvisionalLoad(frame, is_new_navigation));
+
   // Remember that we've already processed this request, so we don't update
   // the session history again.  We do this regardless of whether this is
   // a session history navigation, because if we attempted a session history
@@ -3536,8 +3523,8 @@ void RenderView::didFinishDocumentLoad(WebFrame* frame) {
 
   Send(new ViewHostMsg_DocumentLoadedInFrame(routing_id_, frame->identifier()));
 
-  FOR_EACH_OBSERVER(
-      RenderViewObserver, observers_, DidFinishDocumentLoad(frame));
+  FOR_EACH_OBSERVER(RenderViewObserver, observers_,
+                    DidFinishDocumentLoad(frame));
 
   // Check whether we have new encoding name.
   UpdateEncoding(frame, frame->view()->pageEncoding().utf8());
@@ -3578,7 +3565,7 @@ void RenderView::didHandleOnloadEvents(WebFrame* frame) {
 }
 
 void RenderView::didFailLoad(WebFrame* frame, const WebURLError& error) {
-  // Ignore
+  FOR_EACH_OBSERVER(RenderViewObserver, observers_, DidFailLoad(frame, error));
 }
 
 void RenderView::didFinishLoad(WebFrame* frame) {
diff --git a/chrome/renderer/render_view.h b/chrome/renderer/render_view.h
index 4ffaea9..6a57a03 100644
--- a/chrome/renderer/render_view.h
+++ b/chrome/renderer/render_view.h
@@ -108,10 +108,6 @@ class FullscreenContainer;
 
 }  // namespace webkit
 
-namespace safe_browsing {
-class PhishingClassifierDelegate;
-}
-
 namespace webkit_glue {
 struct CustomContextMenuContext;
 class ImageResourceFetcher;
@@ -235,12 +231,6 @@ class RenderView : public RenderWidget,
     send_content_state_immediately_ = value;
   }
 
-  // May be NULL if client-side phishing detection is disabled.
-  safe_browsing::PhishingClassifierDelegate*
-      phishing_classifier_delegate() const {
-    return phishing_delegate_.get();
-  }
-
   // Returns true if we should display scrollbars for the given view size and
   // false if the scrollbars should be hidden.
   bool should_display_scrollbars(int width, int height) const {
@@ -390,7 +380,6 @@ class RenderView : public RenderWidget,
   virtual void didFocus();
   virtual void didBlur();
   virtual void show(WebKit::WebNavigationPolicy policy);
-  virtual void closeWidgetSoon();
   virtual void runModal();
 
   // WebKit::WebViewClient implementation --------------------------------------
@@ -1388,10 +1377,6 @@ class RenderView : public RenderWidget,
   // Responsible for sending page load related histograms.
   PageLoadHistograms page_load_histograms_;
 
-  // Handles the interaction between the RenderView and the phishing
-  // classifier.
-  scoped_ptr<safe_browsing::PhishingClassifierDelegate> phishing_delegate_;
-
   // Misc ----------------------------------------------------------------------
 
   // The current and pending file chooser completion objects. If the queue is
diff --git a/chrome/renderer/render_view_observer.h b/chrome/renderer/render_view_observer.h
index be0d85f..a65ccb9 100644
--- a/chrome/renderer/render_view_observer.h
+++ b/chrome/renderer/render_view_observer.h
@@ -14,6 +14,7 @@ class RenderView;
 namespace WebKit {
 class WebFrame;
 class WebMouseEvent;
+struct WebURLError;
 }
 
 // Base class for objects that want to filter incoming IPCs, and also get
@@ -27,13 +28,21 @@ class RenderViewObserver : public IPC::Channel::Listener,
 
   // These match the WebKit API notifications.
   virtual void DidFinishDocumentLoad(WebKit::WebFrame* frame) {}
+  virtual void DidFailLoad(WebKit::WebFrame* frame,
+                           const WebKit::WebURLError& error) {}
   virtual void DidFinishLoad(WebKit::WebFrame* frame) {}
+  virtual void DidStartProvisionalLoad(WebKit::WebFrame* frame) {}
+  virtual void DidFailProvisionalLoad(WebKit::WebFrame* frame,
+                                      const WebKit::WebURLError& error) {}
+  virtual void DidCommitProvisionalLoad(WebKit::WebFrame* frame,
+                                        bool is_new_navigation) {}
   virtual void FrameDetached(WebKit::WebFrame* frame) {}
   virtual void FrameWillClose(WebKit::WebFrame* frame) {}
 
   // These match the RenderView methods below.
   virtual void FrameTranslated(WebKit::WebFrame* frame) {}
   virtual void DidHandleMouseEvent(const WebKit::WebMouseEvent& event) {}
+  virtual void PageCaptured(const string16& page_text) {}
 
  protected:
   RenderViewObserver(RenderView* render_view);
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
index a1364ed..d99d3aa 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
@@ -4,38 +4,101 @@
 
 #include "chrome/renderer/safe_browsing/phishing_classifier_delegate.h"
 
+#include <set>
+
 #include "base/callback.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/scoped_callback_factory.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/renderer/navigation_state.h"
+#include "chrome/renderer/render_thread.h"
 #include "chrome/renderer/render_view.h"
 #include "chrome/renderer/safe_browsing/feature_extractor_clock.h"
 #include "chrome/renderer/safe_browsing/phishing_classifier.h"
+#include "chrome/renderer/safe_browsing/scorer.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURL.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
 
 namespace safe_browsing {
 
+typedef std::set<PhishingClassifierDelegate*> PhishingClassifierDelegates;
+static base::LazyInstance<PhishingClassifierDelegates>
+    g_delegates(base::LINKER_INITIALIZED);
+
+static base::LazyInstance<scoped_ptr<const safe_browsing::Scorer> >
+    g_phishing_scorer(base::LINKER_INITIALIZED);
+
+class ScorerCallback {
+ public:
+  static Scorer::CreationCallback* CreateCallback() {
+    ScorerCallback* scorer_callback = new ScorerCallback();
+    return scorer_callback->callback_factory_->NewCallback(
+        &ScorerCallback::PhishingScorerCreated);
+  }
+
+ private:
+  ScorerCallback() {
+    callback_factory_.reset(
+        new base::ScopedCallbackFactory<ScorerCallback>(this));
+  }
+
+  // Callback to be run once the phishing Scorer has been created.
+  void PhishingScorerCreated(safe_browsing::Scorer* scorer) {
+    if (!scorer) {
+      DLOG(ERROR) << "Unable to create a PhishingScorer - corrupt model?";
+      return;
+    }
+
+    g_phishing_scorer.Get().reset(scorer);
+
+    PhishingClassifierDelegates::iterator i;
+    for (i = g_delegates.Get().begin(); i != g_delegates.Get().end(); ++i)
+      (*i)->SetPhishingScorer(scorer);
+
+    delete this;
+  }
+
+  scoped_ptr<base::ScopedCallbackFactory<ScorerCallback> > callback_factory_;
+};
+
+void PhishingClassifierDelegate::SetPhishingModel(
+    IPC::PlatformFileForTransit model_file) {
+  safe_browsing::Scorer::CreateFromFile(
+      IPC::PlatformFileForTransitToPlatformFile(model_file),
+      RenderThread::current()->GetFileThreadMessageLoopProxy(),
+      ScorerCallback::CreateCallback());
+}
+
 PhishingClassifierDelegate::PhishingClassifierDelegate(
     RenderView* render_view,
     PhishingClassifier* classifier)
-    : render_view_(render_view),
+    : RenderViewObserver(render_view),
       last_page_id_sent_to_classifier_(-1),
       pending_classification_(false) {
+  g_delegates.Get().insert(this);
   if (!classifier) {
-    classifier = new PhishingClassifier(render_view_,
+    classifier = new PhishingClassifier(render_view,
                                         new FeatureExtractorClock());
   }
+
   classifier_.reset(classifier);
+
+  if (g_phishing_scorer.Get().get())
+    SetPhishingScorer(g_phishing_scorer.Get().get());
 }
 
 PhishingClassifierDelegate::~PhishingClassifierDelegate() {
   CancelPendingClassification();
+  g_delegates.Get().erase(this);
 }
 
 void PhishingClassifierDelegate::SetPhishingScorer(
     const safe_browsing::Scorer* scorer) {
+  if (!render_view()->webview())
+    return;  // RenderView is tearing down.
+
   classifier_->set_phishing_scorer(scorer);
 
   if (pending_classification_) {
@@ -45,15 +108,18 @@ void PhishingClassifierDelegate::SetPhishingScorer(
     // classification.  This is because we stop any pending classification on
     // main frame loads in RenderView::didCommitProvisionalLoad().
     DCHECK_EQ(StripToplevelUrl(), last_url_sent_to_classifier_);
-    DCHECK_EQ(render_view_->page_id(), last_page_id_sent_to_classifier_);
+    DCHECK_EQ(render_view()->page_id(), last_page_id_sent_to_classifier_);
     classifier_->BeginClassification(
         &classifier_page_text_,
         NewCallback(this, &PhishingClassifierDelegate::ClassificationDone));
   }
 }
 
-void PhishingClassifierDelegate::CommittedLoadInFrame(
-    WebKit::WebFrame* frame) {
+void PhishingClassifierDelegate::DidCommitProvisionalLoad(
+    WebKit::WebFrame* frame, bool is_new_navigation) {
+  if (!is_new_navigation)
+    return;
+
   // A new page is starting to load.  Unless the load is a navigation within
   // the same page, we need to cancel classification since the content will
   // now be inconsistent with the phishing model.
@@ -64,12 +130,12 @@ void PhishingClassifierDelegate::CommittedLoadInFrame(
   }
 }
 
-void PhishingClassifierDelegate::FinishedLoad(string16* page_text) {
+void PhishingClassifierDelegate::PageCaptured(const string16& page_text) {
   // We check that the page id has incremented so that we don't reclassify
   // pages as the user moves back and forward in session history.  Note: we
   // don't send every page id to the classifier, only those where the toplevel
   // URL changed.
-  int load_id = render_view_->page_id();
+  int load_id = render_view()->page_id();
   if (load_id <= last_page_id_sent_to_classifier_) {
     return;
   }
@@ -84,7 +150,7 @@ void PhishingClassifierDelegate::FinishedLoad(string16* page_text) {
 
   last_url_sent_to_classifier_ = url_without_ref;
   last_page_id_sent_to_classifier_ = load_id;
-  classifier_page_text_.swap(*page_text);
+  classifier_page_text_ = page_text;
 
   if (classifier_->is_ready()) {
     classifier_->BeginClassification(
@@ -105,6 +171,18 @@ void PhishingClassifierDelegate::CancelPendingClassification() {
   pending_classification_ = false;
 }
 
+bool PhishingClassifierDelegate::OnMessageReceived(
+    const IPC::Message& message) {
+  /*
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(PhishingClassifierDelegate, message)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+  */
+  return false;
+}
+
 void PhishingClassifierDelegate::ClassificationDone(bool is_phishy,
                                                     double phishy_score) {
   // We no longer need the page text.
@@ -114,14 +192,14 @@ void PhishingClassifierDelegate::ClassificationDone(bool is_phishy,
     return;
   }
 
-  render_view_->Send(new ViewHostMsg_DetectedPhishingSite(
-      render_view_->routing_id(),
+  render_view()->Send(new ViewHostMsg_DetectedPhishingSite(
+      render_view()->routing_id(),
       last_url_sent_to_classifier_,
       phishy_score));
 }
 
 GURL PhishingClassifierDelegate::StripToplevelUrl() {
-  GURL toplevel_url = render_view_->webview()->mainFrame()->url();
+  GURL toplevel_url = render_view()->webview()->mainFrame()->url();
   GURL::Replacements replacements;
   replacements.ClearRef();
   return toplevel_url.ReplaceComponents(replacements);
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate.h b/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
index 9d17127..89788f2 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
@@ -10,20 +10,18 @@
 
 #include "base/scoped_ptr.h"
 #include "base/string16.h"
+#include "chrome/renderer/render_view_observer.h"
 #include "googleurl/src/gurl.h"
-
-class RenderView;
-
-namespace WebKit {
-class WebFrame;
-}
+#include "ipc/ipc_platform_file.h"
 
 namespace safe_browsing {
 class PhishingClassifier;
 class Scorer;
 
-class PhishingClassifierDelegate {
+class PhishingClassifierDelegate : public RenderViewObserver {
  public:
+  static void SetPhishingModel(IPC::PlatformFileForTransit model_file);
+
   // The RenderView owns us.  This object takes ownership of the classifier.
   // Note that if classifier is null, a default instance of PhishingClassifier
   // will be used.
@@ -35,17 +33,10 @@ class PhishingClassifierDelegate {
   // The scorer is passed on to the classifier.
   void SetPhishingScorer(const safe_browsing::Scorer* scorer);
 
-  // Called by the RenderView when a page has started loading in the given
-  // WebFrame.  Typically, this will cause any pending classification to be
-  // cancelled.  However, if the load is for the main frame, and the toplevel
-  // URL has not changed, we continue running the current classification.
-  void CommittedLoadInFrame(WebKit::WebFrame* frame);
-
-  // Called by the RenderView once a page has finished loading.  Determines
-  // whether a new toplevel load has taken place, and if so, begins
-  // classification.  May modify page_text.  Note that it is an error to
-  // call OnNavigate if there is a pending classification.
-  void FinishedLoad(string16* page_text);
+  // RenderViewObserver implementation, public for testing.
+  virtual void PageCaptured(const string16& page_text);
+  virtual void DidCommitProvisionalLoad(WebKit::WebFrame* frame,
+                                        bool is_new_navigation);
 
   // Cancels any pending classification and frees the page text.  Called by
   // the RenderView when the RenderView is going away.
@@ -54,15 +45,15 @@ class PhishingClassifierDelegate {
  private:
   friend class PhishingClassifierDelegateTest;
 
+  // RenderViewObserver implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message);
+
   // Called when classification for the current page finishes.
   void ClassificationDone(bool is_phishy, double phishy_score);
 
   // Returns the RenderView's toplevel URL, with the ref stripped.
   GURL StripToplevelUrl();
 
-  // The RenderView that owns this object.
-  RenderView* render_view_;
-
   // The PhishingClassifier to use for the RenderView.  This is created once
   // a scorer is made available via SetPhishingScorer().
   scoped_ptr<PhishingClassifier> classifier_;
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
index f24d8f4..0863aaf 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
@@ -97,60 +97,64 @@ class PhishingClassifierDelegateTest : public RenderViewFakeResourcesTest {
 TEST_F(PhishingClassifierDelegateTest, Navigation) {
   MockPhishingClassifier* classifier =
       new StrictMock<MockPhishingClassifier>(view_);
-  PhishingClassifierDelegate delegate(view_, classifier);
+  PhishingClassifierDelegate* delegate =
+      new PhishingClassifierDelegate(view_, classifier);
   MockScorer scorer;
-  delegate.SetPhishingScorer(&scorer);
+  delegate->SetPhishingScorer(&scorer);
   ASSERT_TRUE(classifier->is_ready());
 
   // Test an initial load.  We expect classification to happen normally.
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   responses_["http://host.com/"] =
       "<html><body><iframe src=\"http://sub1.com/\"></iframe></body></html>";
   LoadURL("http://host.com/");
   WebKit::WebFrame* child_frame = GetMainFrame()->firstChild();
   string16 page_text = ASCIIToUTF16("dummy");
   EXPECT_CALL(*classifier, CancelPendingClassification()).Times(2);
-  delegate.CommittedLoadInFrame(GetMainFrame());
-  delegate.CommittedLoadInFrame(child_frame);
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
+  delegate->DidCommitProvisionalLoad(child_frame, true);
   Mock::VerifyAndClearExpectations(classifier);
   EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
       WillOnce(DeleteArg<1>());
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
   Mock::VerifyAndClearExpectations(classifier);
 
   // Reloading the same page should not trigger a reclassification.
   // However, it will cancel any pending classification since the
   // content is being replaced.
   EXPECT_CALL(*classifier, CancelPendingClassification());
-  delegate.CommittedLoadInFrame(GetMainFrame());
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
   Mock::VerifyAndClearExpectations(classifier);
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
 
   // Navigating in a subframe will increment the page id, but not change
   // the toplevel URL.  This should cancel pending classification since the
   // page content is changing, and not begin a new classification.
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   child_frame->loadRequest(WebKit::WebURLRequest(GURL("http://sub2.com/")));
   message_loop_.Run();
   EXPECT_CALL(*classifier, CancelPendingClassification());
-  delegate.CommittedLoadInFrame(child_frame);
+  delegate->DidCommitProvisionalLoad(child_frame, true);
   Mock::VerifyAndClearExpectations(classifier);
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
 
   // Scrolling to an anchor will increment the page id, but should not
   // not trigger a reclassification.  A pending classification should not
   // be cancelled, since the content is not changing.
   LoadURL("http://host.com/#foo");
-  delegate.CommittedLoadInFrame(GetMainFrame());
-  delegate.FinishedLoad(&page_text);
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
+  delegate->PageCaptured(page_text);
 
   // Now load a new toplevel page, which should trigger another classification.
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   LoadURL("http://host2.com/");
   page_text = ASCIIToUTF16("dummy2");
   EXPECT_CALL(*classifier, CancelPendingClassification());
-  delegate.CommittedLoadInFrame(GetMainFrame());
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
   Mock::VerifyAndClearExpectations(classifier);
   EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
       WillOnce(DeleteArg<1>());
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
   Mock::VerifyAndClearExpectations(classifier);
 
   // The delegate will cancel pending classification on destruction.
@@ -161,28 +165,29 @@ TEST_F(PhishingClassifierDelegateTest, PendingClassification) {
   // For this test, we'll create the delegate with no scorer available yet.
   MockPhishingClassifier* classifier =
       new StrictMock<MockPhishingClassifier>(view_);
-  PhishingClassifierDelegate delegate(view_, classifier);
+  PhishingClassifierDelegate* delegate =
+      new PhishingClassifierDelegate(view_, classifier);
   ASSERT_FALSE(classifier->is_ready());
 
   // Queue up a pending classification, cancel it, then queue up another one.
   LoadURL("http://host.com/");
   string16 page_text = ASCIIToUTF16("dummy");
-  delegate.CommittedLoadInFrame(GetMainFrame());
-  delegate.FinishedLoad(&page_text);
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
+  delegate->PageCaptured(page_text);
 
   LoadURL("http://host2.com/");
-  delegate.CommittedLoadInFrame(GetMainFrame());
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
   page_text = ASCIIToUTF16("dummy2");
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
 
   // Now set a scorer, which should cause a classifier to be created and
   // the classification to proceed.  Note that we need to reset |page_text|
-  // since it is modified by the call to FinishedLoad().
+  // since it is modified by the call to PageCaptured().
   page_text = ASCIIToUTF16("dummy2");
   EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
       WillOnce(DeleteArg<1>());
   MockScorer scorer;
-  delegate.SetPhishingScorer(&scorer);
+  delegate->SetPhishingScorer(&scorer);
   Mock::VerifyAndClearExpectations(classifier);
 
   // The delegate will cancel pending classification on destruction.
@@ -194,27 +199,28 @@ TEST_F(PhishingClassifierDelegateTest, PendingClassification_Ref) {
   // setting the scorer.
   MockPhishingClassifier* classifier =
       new StrictMock<MockPhishingClassifier>(view_);
-  PhishingClassifierDelegate delegate(view_, classifier);
+  PhishingClassifierDelegate* delegate =
+      new PhishingClassifierDelegate(view_, classifier);
   ASSERT_FALSE(classifier->is_ready());
 
   // Queue up a pending classification, cancel it, then queue up another one.
   LoadURL("http://host.com/");
-  delegate.CommittedLoadInFrame(GetMainFrame());
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
   string16 orig_page_text = ASCIIToUTF16("dummy");
   string16 page_text = orig_page_text;
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
 
   LoadURL("http://host.com/#foo");
   page_text = orig_page_text;
-  delegate.CommittedLoadInFrame(GetMainFrame());
-  delegate.FinishedLoad(&page_text);
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
+  delegate->PageCaptured(page_text);
 
   // Now set a scorer, which should cause a classifier to be created and
   // the classification to proceed.
   EXPECT_CALL(*classifier, BeginClassification(Pointee(orig_page_text), _)).
       WillOnce(DeleteArg<1>());
   MockScorer scorer;
-  delegate.SetPhishingScorer(&scorer);
+  delegate->SetPhishingScorer(&scorer);
   Mock::VerifyAndClearExpectations(classifier);
 
   // The delegate will cancel pending classification on destruction.
@@ -226,25 +232,27 @@ TEST_F(PhishingClassifierDelegateTest, DetectedPhishingSite) {
   // if a site comes back as phishy.
   MockPhishingClassifier* classifier =
       new StrictMock<MockPhishingClassifier>(view_);
-  PhishingClassifierDelegate delegate(view_, classifier);
+  PhishingClassifierDelegate* delegate =
+      new PhishingClassifierDelegate(view_, classifier);
   MockScorer scorer;
-  delegate.SetPhishingScorer(&scorer);
+  delegate->SetPhishingScorer(&scorer);
   ASSERT_TRUE(classifier->is_ready());
 
   // Start by loading a page to populate the delegate's state.
   responses_["http://host.com/"] = "<html><body>phish</body></html>";
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   LoadURL("http://host.com/");
   string16 page_text = ASCIIToUTF16("phish");
   EXPECT_CALL(*classifier, CancelPendingClassification());
-  delegate.CommittedLoadInFrame(GetMainFrame());
+  delegate->DidCommitProvisionalLoad(GetMainFrame(), true);
   Mock::VerifyAndClearExpectations(classifier);
   EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
       WillOnce(DeleteArg<1>());
-  delegate.FinishedLoad(&page_text);
+  delegate->PageCaptured(page_text);
   Mock::VerifyAndClearExpectations(classifier);
 
   // Now run the callback to simulate the classifier finishing.
-  RunClassificationDone(&delegate, true, 0.8);
+  RunClassificationDone(delegate, true, 0.8);
   EXPECT_TRUE(detected_phishing_site_);
   EXPECT_EQ(GURL("http://host.com/"), detected_url_);
   EXPECT_EQ(0.8, detected_score_);
diff --git a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc
index 3c577b8..74eb406 100644
--- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc
+++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc
@@ -146,11 +146,7 @@ void PhishingDOMFeatureExtractor::ExtractFeaturesWithTimeout() {
   if (!cur_frame_) {
     WebKit::WebView* web_view = render_view_->webview();
     if (!web_view) {
-      // When the WebView is going away, the render view should have called
-      // CancelPendingExtraction() which should have stopped any pending work,
-      // so this case should not happen.
-      NOTREACHED();
-      RunCallback(false);
+      RunCallback(false);  // The WebView is going away.
       return;
     }
     cur_frame_ = web_view->mainFrame();
diff --git a/chrome/renderer/safe_browsing/phishing_thumbnailer.cc b/chrome/renderer/safe_browsing/phishing_thumbnailer.cc
index 90cf2f7..de8c70b 100644
--- a/chrome/renderer/safe_browsing/phishing_thumbnailer.cc
+++ b/chrome/renderer/safe_browsing/phishing_thumbnailer.cc
@@ -28,8 +28,7 @@ SkBitmap GrabPhishingThumbnail(RenderView* render_view,
                                const gfx::Size& view_size,
                                const gfx::Size& thumbnail_size) {
   if (!render_view || !render_view->webview()) {
-    NOTREACHED();
-    return SkBitmap();
+    return SkBitmap();  // The WebView is going away.
   }
   WebView* view = render_view->webview();
   base::TimeTicks beginning_time = base::TimeTicks::Now();