Have the phishing classifier guard against extra PageCaptured calls.

Currently, the classifier assumes that any PageCaptured call will be preceded by a DidCommitProvisionalLoad.  This seems to not always be the case, and is causing occasional crashes.  Cancel classification in this situation so that we don't swap out the page text from under the term feature extractor.

BUG=80325
TEST=PhishingClassifierDelegateTest.DuplicatePageCapture

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

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

2 files changed, 100 insertions, 18 deletions

diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
index e246ec3..c58f061 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate.cc
@@ -157,6 +157,12 @@ void PhishingClassifierDelegate::PageCaptured(string16* page_text,
   if (preliminary_capture) {
     return;
   }
+  // Make sure there's no classification in progress.  We don't want to swap
+  // out the page text string from underneath the term feature extractor.
+  //
+  // Note: Currently, if the url hasn't changed, we won't restart
+  // classification in this case.  We may want to adjust this.
+  CancelPendingClassification();
   last_finished_load_url_ = GetToplevelUrl();
   classifier_page_text_.swap(*page_text);
   have_page_text_ = true;
diff --git a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
index 04819bf..fd1693a 100644
--- a/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
+++ b/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
@@ -25,6 +25,7 @@
 
 using ::testing::_;
 using ::testing::DeleteArg;
+using ::testing::InSequence;
 using ::testing::Mock;
 using ::testing::Pointee;
 using ::testing::StrictMock;
@@ -115,10 +116,14 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/"));
   string16 page_text = ASCIIToUTF16("dummy");
-  EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
-      WillOnce(DeleteArg<1>());
-  delegate->PageCaptured(&page_text, false);
-  Mock::VerifyAndClearExpectations(classifier);
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
+        WillOnce(DeleteArg<1>());
+    delegate->PageCaptured(&page_text, false);
+    Mock::VerifyAndClearExpectations(classifier);
+  }
 
   // Reloading the same page should not trigger a reclassification.
   // However, it will cancel any pending classification since the
@@ -129,7 +134,9 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/"));
   page_text = ASCIIToUTF16("dummy");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   // Navigating in a subframe will not change the toplevel URL.  However, this
   // should cancel pending classification since the page content is changing.
@@ -141,7 +148,9 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/"));
   page_text = ASCIIToUTF16("dummy");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   // Scrolling to an anchor works similarly to a subframe navigation, but
   // see the TODO in PhishingClassifierDelegate::DidCommitProvisionalLoad.
@@ -150,18 +159,24 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
   page_text = ASCIIToUTF16("dummy");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   // Now load a new toplevel page, which should trigger another classification.
   EXPECT_CALL(*classifier, CancelPendingClassification());
   LoadURL("http://host2.com/");
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("dummy2");
-  EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
-      WillOnce(DeleteArg<1>());
   OnStartPhishingDetection(delegate, GURL("http://host2.com/"));
-  delegate->PageCaptured(&page_text, false);
-  Mock::VerifyAndClearExpectations(classifier);
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
+        WillOnce(DeleteArg<1>());
+    delegate->PageCaptured(&page_text, false);
+    Mock::VerifyAndClearExpectations(classifier);
+  }
 
   // No classification should happen on back/forward navigation.
   // Note: in practice, the browser will not send a StartPhishingDetection IPC
@@ -173,14 +188,18 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("dummy");
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   EXPECT_CALL(*classifier, CancelPendingClassification());
   GoForward(forward_item);
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("dummy2");
   OnStartPhishingDetection(delegate, GURL("http://host2.com/"));
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   // Now go back again and scroll to a different anchor.
   // No classification should happen.
@@ -189,14 +208,18 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("dummy");
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo"));
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   EXPECT_CALL(*classifier, CancelPendingClassification());
   LoadURL("http://host.com/#foo2");
   Mock::VerifyAndClearExpectations(classifier);
   OnStartPhishingDetection(delegate, GURL("http://host.com/#foo2"));
   page_text = ASCIIToUTF16("dummy");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   // The delegate will cancel pending classification on destruction.
   EXPECT_CALL(*classifier, CancelPendingClassification());
@@ -283,7 +306,9 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
   LoadURL("http://host.com/");
   Mock::VerifyAndClearExpectations(classifier);
   string16 page_text = ASCIIToUTF16("phish");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
   // Now simulate the StartPhishingDetection IPC.  We expect classification
   // to begin.
   page_text = ASCIIToUTF16("phish");
@@ -299,7 +324,9 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
   LoadURL("http://host2.com/");
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("phish");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
 
   EXPECT_CALL(*classifier, CancelPendingClassification());
   responses_["http://host3.com/"] = "<html><body>phish</body></html>";
@@ -316,17 +343,23 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
   LoadURL("http://host4.com/");
   Mock::VerifyAndClearExpectations(classifier);
   page_text = ASCIIToUTF16("abc");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
+  Mock::VerifyAndClearExpectations(classifier);
   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);
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    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());
@@ -352,8 +385,47 @@ TEST_F(PhishingClassifierDelegateTest, IgnorePreliminaryCapture) {
 
   // Once the non-preliminary capture happens, classification should begin.
   page_text = ASCIIToUTF16("phish");
-  EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
-      WillOnce(DeleteArg<1>());
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    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());
+}
+
+TEST_F(PhishingClassifierDelegateTest, DuplicatePageCapture) {
+  // Tests that a second PageCaptured notification causes classification to
+  // be cancelled.
+  MockPhishingClassifier* classifier =
+      new StrictMock<MockPhishingClassifier>(view_);
+  PhishingClassifierDelegate* delegate =
+      new PhishingClassifierDelegate(view_, classifier);
+  MockScorer scorer;
+  delegate->SetPhishingScorer(&scorer);
+  ASSERT_TRUE(classifier->is_ready());
+
+  EXPECT_CALL(*classifier, CancelPendingClassification());
+  responses_["http://host.com/"] = "<html><body>phish</body></html>";
+  LoadURL("http://host.com/");
+  Mock::VerifyAndClearExpectations(classifier);
+  OnStartPhishingDetection(delegate, GURL("http://host.com/"));
+  string16 page_text = ASCIIToUTF16("phish");
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
+        WillOnce(DeleteArg<1>());
+    delegate->PageCaptured(&page_text, false);
+    Mock::VerifyAndClearExpectations(classifier);
+  }
+
+  page_text = ASCIIToUTF16("phish");
+  EXPECT_CALL(*classifier, CancelPendingClassification());
   delegate->PageCaptured(&page_text, false);
   Mock::VerifyAndClearExpectations(classifier);
 
@@ -378,11 +450,15 @@ TEST_F(PhishingClassifierDelegateTest, DetectedPhishingSite) {
   LoadURL("http://host.com/#a");
   Mock::VerifyAndClearExpectations(classifier);
   string16 page_text = ASCIIToUTF16("phish");
-  EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
-      WillOnce(DeleteArg<1>());
   OnStartPhishingDetection(delegate, GURL("http://host.com/#a"));
-  delegate->PageCaptured(&page_text, false);
-  Mock::VerifyAndClearExpectations(classifier);
+  {
+    InSequence s;
+    EXPECT_CALL(*classifier, CancelPendingClassification());
+    EXPECT_CALL(*classifier, BeginClassification(Pointee(page_text), _)).
+        WillOnce(DeleteArg<1>());
+    delegate->PageCaptured(&page_text, false);
+    Mock::VerifyAndClearExpectations(classifier);
+  }
 
   // Now run the callback to simulate the classifier finishing.
   ClientPhishingRequest verdict;