Change the phishing classifier to look at the transition type to determine history navigations.

Previously, this code was looking at page ids, and making some incorrect assumptions about them for cases such as location.replace().  Also, we'll no longer start classification if a back/forward navigation is followed by an in-page navigation.  To get the tests working properly for the new code, RenderViewFakeResourcesTest needs to simulate an actual back/forward a little more closely, by sending a ViewMsg_FrameNavigate.

BUG=80345
TEST=PhishingClassifierDelegateTest.Navigation

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

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

5 files changed, 132 insertions, 34 deletions

diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
index 43bba8d..e246ec3 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
@@ -94,8 +94,8 @@ PhishingClassifierDelegate::PhishingClassifierDelegate(
     RenderView* render_view,
     PhishingClassifier* classifier)
     : RenderViewObserver(render_view),
-      last_finished_load_id_(-1),
-      last_page_id_sent_to_classifier_(-1) {
+      last_main_frame_transition_(PageTransition::LINK),
+      have_page_text_(false) {
   g_delegates.Get().insert(this);
   if (!classifier) {
     classifier = new PhishingClassifier(render_view,
@@ -147,6 +147,9 @@ void PhishingClassifierDelegate::DidCommitProvisionalLoad(
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.CanceledForInPageNavigation", 1);
   }
   CancelPendingClassification();
+  if (frame == render_view()->webview()->mainFrame()) {
+    last_main_frame_transition_ = state->transition_type();
+  }
 }
 
 void PhishingClassifierDelegate::PageCaptured(string16* page_text,
@@ -154,9 +157,9 @@ void PhishingClassifierDelegate::PageCaptured(string16* page_text,
   if (preliminary_capture) {
     return;
   }
-  last_finished_load_id_ = render_view()->page_id();
   last_finished_load_url_ = GetToplevelUrl();
   classifier_page_text_.swap(*page_text);
+  have_page_text_ = true;
   MaybeStartClassification();
 }
 
@@ -165,6 +168,7 @@ void PhishingClassifierDelegate::CancelPendingClassification() {
     classifier_->CancelPendingClassification();
   }
   classifier_page_text_.clear();
+  have_page_text_ = false;
 }
 
 bool PhishingClassifierDelegate::OnMessageReceived(
@@ -215,30 +219,30 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
     return;
   }
 
-  if (last_finished_load_id_ <= last_page_id_sent_to_classifier_) {
-    // Skip loads from session history navigation.
-    VLOG(2) << "Not starting classification, last finished load id is "
-            << last_finished_load_id_ << " but we have classified up to "
-            << "load id " << last_page_id_sent_to_classifier_;
+  if (last_main_frame_transition_ & PageTransition::FORWARD_BACK) {
+    // Skip loads from session history navigation.  However, update the
+    // last URL sent to the classifier, so that we'll properly detect
+    // in-page navigations.
+    VLOG(2) << "Not starting classification for back/forward navigation";
+    last_url_sent_to_classifier_ = last_finished_load_url_;
     classifier_page_text_.clear();  // we won't need this.
+    have_page_text_ = false;
     return;
   }
 
-  if (last_finished_load_id_ != render_view()->page_id()) {
-    VLOG(2) << "Render view page has changed, not starting classification";
-    classifier_page_text_.clear();  // we won't need this.
-    return;
-  }
-  // If the page id is unchanged, the toplevel URL should also be unchanged.
   GURL stripped_last_load_url(StripRef(last_finished_load_url_));
-  DCHECK_EQ(StripRef(GetToplevelUrl()), stripped_last_load_url);
-
   if (stripped_last_load_url == StripRef(last_url_sent_to_classifier_)) {
     // We've already classified this toplevel URL, so this was likely an
     // in-page navigation or a subframe navigation.  The browser should not
     // send a StartPhishingDetection IPC in this case.
     VLOG(2) << "Toplevel URL is unchanged, not starting classification.";
     classifier_page_text_.clear();  // we won't need this.
+    have_page_text_ = false;
+    return;
+  }
+
+  if (!have_page_text_) {
+    VLOG(2) << "Not starting classification, there is no page text ready.";
     return;
   }
 
@@ -257,7 +261,6 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
 
   VLOG(2) << "Starting classification for " << last_finished_load_url_;
   last_url_sent_to_classifier_ = last_finished_load_url_;
-  last_page_id_sent_to_classifier_ = last_finished_load_id_;
   classifier_->BeginClassification(
       &classifier_page_text_,
       NewCallback(this, &PhishingClassifierDelegate::ClassificationDone));
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate.h b/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
index e6eb0f2..c86be3f 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate.h
@@ -10,6 +10,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
+#include "content/common/page_transition_types.h"
 #include "content/renderer/render_view_observer.h"
 #include "content/renderer/render_process_observer.h"
 #include "googleurl/src/gurl.h"
@@ -40,7 +41,7 @@ class PhishingClassifierDelegate : public RenderViewObserver {
   void SetPhishingScorer(const safe_browsing::Scorer* scorer);
 
   // Called by the RenderView once a page has finished loading.  Updates the
-  // last-loaded URL and page id, then starts classification if all other
+  // last-loaded URL and page text, then starts classification if all other
   // conditions are met (see MaybeStartClassification for details).
   // We ignore preliminary captures, since these happen before the page has
   // finished loading.
@@ -89,16 +90,20 @@ class PhishingClassifierDelegate : public RenderViewObserver {
   // with the ref stripped.
   GURL last_url_received_from_browser_;
 
-  // The last URL and page id that have finished loading in the RenderView.
-  // These correspond to the text in classifier_page_text_.
+  // The last top-level URL that has finished loading in the RenderView.
+  // This corresponds to the text in classifier_page_text_.
   GURL last_finished_load_url_;
-  int32 last_finished_load_id_;
 
-  // The URL and page id of the last load that we actually started
-  // classification on.  This is used to suppress phishing classification on
-  // subframe navigation and back and forward navigations in history.
+  // The transition type for the last load in the main frame.  We use this
+  // to exclude back/forward loads from classification.  Note that this is
+  // set in DidCommitProvisionalLoad(); the transition is reset after this
+  // call in the RenderView, so we need to save off the value.
+  PageTransition::Type last_main_frame_transition_;
+
+  // The URL of the last load that we actually started classification on.
+  // This is used to suppress phishing classification on subframe navigation
+  // and back and forward navigations in history.
   GURL last_url_sent_to_classifier_;
-  int32 last_page_id_sent_to_classifier_;
 
   // The page text that will be analyzed by the phishing classifier.  This is
   // set by OnNavigate and cleared when the classifier finishes.  Note that if
@@ -107,6 +112,11 @@ class PhishingClassifierDelegate : public RenderViewObserver {
   // these conditions are met.
   string16 classifier_page_text_;
 
+  // Tracks whether we have stored anything in classifier_page_text_ for the
+  // most recent load.  We use this to distinguish empty text from cases where
+  // PageCaptured has not been called.
+  bool have_page_text_;
+
   DISALLOW_COPY_AND_ASSIGN(PhishingClassifierDelegate);
 };
 
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
index f422309..04819bf 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
@@ -19,6 +19,7 @@
 #include "googleurl/src/gurl.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebHistoryItem.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURL.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLRequest.h"
 
@@ -130,9 +131,9 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   page_text = ASCIIToUTF16("dummy");
   delegate->PageCaptured(&page_text, false);
 
-  // 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.
+  // Navigating in a subframe will not change the toplevel URL.  However, this
+  // should cancel pending classification since the page content is changing.
+  // Currently, we do not start a new classification after subframe loads.
   EXPECT_CALL(*classifier, CancelPendingClassification());
   GetMainFrame()->firstChild()->loadRequest(
       WebKit::WebURLRequest(GURL("http://sub2.com/")));
@@ -142,11 +143,11 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   page_text = ASCIIToUTF16("dummy");
   delegate->PageCaptured(&page_text, false);
 
-  // Scrolling to an anchor will increment the page id, but should not
-  // not trigger a reclassification.  Currently, a pending classification will
-  // be canceled, but see the TODO in phishing_classifier_delegate.cc.
+  // Scrolling to an anchor works similarly to a subframe navigation, but
+  // see the TODO in PhishingClassifierDelegate::DidCommitProvisionalLoad.
   EXPECT_CALL(*classifier, CancelPendingClassification());
   LoadURL("http://host.com/#foo");
+  Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
   page_text = ASCIIToUTF16("dummy");
   delegate->PageCaptured(&page_text, false);
@@ -166,6 +167,23 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   // Note: in practice, the browser will not send a StartPhishingDetection IPC
   // in this case.  However, we want to make sure that the delegate behaves
   // correctly regardless.
+  WebKit::WebHistoryItem forward_item = GetMainFrame()->currentHistoryItem();
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  GoBack();
+  Mock::VerifyAndClearExpectations(classifier);
+  page_text = ASCIIToUTF16("dummy");
+  OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
+  delegate->PageCaptured(&page_text, false);
+
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  GoForward(forward_item);
+  Mock::VerifyAndClearExpectations(classifier);
+  page_text = ASCIIToUTF16("dummy2");
+  OnStartPhishingDetection(delegate, GURL("http://host2.com/"));
+  delegate->PageCaptured(&page_text, false);
+
+  // Now go back again and scroll to a different anchor.
+  // No classification should happen.
   EXPECT_CALL(*classifier, CancelPendingClassification());
   GoBack();
   Mock::VerifyAndClearExpectations(classifier);
@@ -173,6 +191,13 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
   delegate->PageCaptured(&page_text, false);
 
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  LoadURL("http://host.com/#foo2");
+  Mock::VerifyAndClearExpectations(classifier);
+  OnStartPhishingDetection(delegate, GURL("http://host.com/#foo2"));
+  page_text = ASCIIToUTF16("dummy");
+  delegate->PageCaptured(&page_text, false);
+
   // The delegate will cancel pending classification on destruction.
   EXPECT_CALL(*classifier, CancelPendingClassification());
 }
@@ -272,6 +297,7 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
   EXPECT_CALL(*classifier, CancelPendingClassification());
   responses_["http://host2.com/"] = "<html><body>phish</body></html>";
   LoadURL("http://host2.com/");
+  Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("phish");
   delegate->PageCaptured(&page_text, false);
 
@@ -281,6 +307,27 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host2.com/"));
 
+  // In this test, the original page is a redirect, which we do not get a
+  // StartPhishingDetection IPC for.  We use location.replace() to load a
+  // new page while reusing the original session history entry, and check that
+  // classification begins correctly for the landing page.
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  responses_["http://host4.com/"] = "<html><body>abc</body></html>";
+  LoadURL("http://host4.com/");
+  Mock::VerifyAndClearExpectations(classifier);
+  page_text = ASCIIToUTF16("abc");
+  delegate->PageCaptured(&page_text, false);
+  responses_["http://host4.com/redir"] = "<html><body>123</body></html>";
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  LoadURL("javascript:location.replace(\'redir\');");
+  Mock::VerifyAndClearExpectations(classifier);
+  OnStartPhishingDetection(delegate, GURL("http://host4.com/redir"));
+  page_text = ASCIIToUTF16("123");
+  EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _))
+      .WillOnce(DeleteArg<1>());
+  delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
+
   // The delegate will cancel pending classification on destruction.
   EXPECT_CALL(*classifier, CancelPendingClassification());
 }
diff --git a/chrome/renderer/safe_browsing/render_view_fake_resources_test.cc b/chrome/renderer/safe_browsing/render_view_fake_resources_test.cc
index 2fabe50..91ea764 100644
--- a/chrome/renderer/safe_browsing/render_view_fake_resources_test.cc
+++ b/chrome/renderer/safe_browsing/render_view_fake_resources_test.cc
@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/process.h"
 #include "base/shared_memory.h"
+#include "base/time.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/renderer/mock_render_process.h"
 #include "content/common/dom_storage_common.h"
@@ -17,6 +18,7 @@
 #include "content/common/resource_response.h"
 #include "content/common/sandbox_init_wrapper.h"
 #include "content/common/view_messages.h"
+#include "content/renderer/navigation_state.h"
 #include "content/renderer/render_thread.h"
 #include "content/renderer/render_view.h"
 #include "content/renderer/renderer_main_platform_delegate.h"
@@ -29,6 +31,7 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLRequest.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
+#include "webkit/glue/glue_serialize.h"
 #include "webkit/glue/webkit_glue.h"
 
 namespace safe_browsing {
@@ -129,9 +132,12 @@ void RenderViewFakeResourcesTest::LoadURLWithPost(const std::string& url) {
 }
 
 void RenderViewFakeResourcesTest::GoBack() {
-  WebKit::WebFrame* frame = GetMainFrame();
-  frame->loadHistoryItem(frame->previousHistoryItem());
-  message_loop_.Run();
+  GoToOffset(-1, GetMainFrame()->previousHistoryItem());
+}
+
+void RenderViewFakeResourcesTest::GoForward(
+    const WebKit::WebHistoryItem& history_item) {
+  GoToOffset(1, history_item);
 }
 
 void RenderViewFakeResourcesTest::OnDidStopLoading() {
@@ -189,4 +195,26 @@ void RenderViewFakeResourcesTest::OnRenderViewReady() {
   message_loop_.Quit();
 }
 
+void RenderViewFakeResourcesTest::GoToOffset(
+    int offset,
+    const WebKit::WebHistoryItem& history_item) {
+  NavigationState* state = NavigationState::FromDataSource(
+      GetMainFrame()->dataSource());
+
+  ViewMsg_Navigate_Params params;
+  params.page_id = view_->page_id() + offset;
+  params.pending_history_list_offset =
+      state->pending_history_list_offset() + offset;
+  params.current_history_list_offset = state->pending_history_list_offset();
+  params.current_history_list_length = (view_->historyBackListCount() +
+                                        view_->historyForwardListCount() + 1);
+  params.url = GURL(history_item.urlString());
+  params.transition = PageTransition::FORWARD_BACK;
+  params.state = webkit_glue::HistoryItemToString(history_item);
+  params.navigation_type = ViewMsg_Navigate_Type::NORMAL;
+  params.request_time = base::Time::Now();
+  channel_->Send(new ViewMsg_Navigate(view_->routing_id(), params));
+  message_loop_.Run();
+}
+
 }  // namespace safe_browsing
diff --git a/chrome/renderer/safe_browsing/render_view_fake_resources_test.h b/chrome/renderer/safe_browsing/render_view_fake_resources_test.h
index 4f44a514..ec4fd60 100644
--- a/chrome/renderer/safe_browsing/render_view_fake_resources_test.h
+++ b/chrome/renderer/safe_browsing/render_view_fake_resources_test.h
@@ -63,6 +63,7 @@ struct ResourceHostMsg_Request;
 
 namespace WebKit {
 class WebFrame;
+class WebHistoryItem;
 }
 
 namespace safe_browsing {
@@ -98,6 +99,12 @@ class RenderViewFakeResourcesTest : public ::testing::Test,
   // Navigates the main frame back in session history.
   void GoBack();
 
+  // Navigates the main frame forward in session history.  Note that for
+  // forward navigations, the caller needs to capture the WebHistoryItem
+  // for the page to go forward to (before going back) and pass it to
+  // this method.  The WebHistoryItem is available from the WebFrame.
+  void GoForward(const WebKit::WebHistoryItem& history_item);
+
   // Returns the main WebFrame for our RenderView.
   WebKit::WebFrame* GetMainFrame();
 
@@ -137,6 +144,9 @@ class RenderViewFakeResourcesTest : public ::testing::Test,
   std::map<std::string, std::string> responses_;
 
  private:
+  // A helper for GoBack and GoForward.
+  void GoToOffset(int offset, const WebKit::WebHistoryItem& history_item);
+
   DISALLOW_COPY_AND_ASSIGN(RenderViewFakeResourcesTest);
 };