Safe Browsing Bad IP feature extraction should wait until all frames

have loaded an not just stop after the first frame finished.

BUG=352782

Review URL: https://codereview.chromium.org/216553009

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

3 files changed, 16 insertions, 23 deletions

diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index c2830fc..b223f54 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -313,7 +313,7 @@ ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
       classification_request_(NULL),
       should_extract_malware_features_(true),
       should_classify_for_malware_(false),
-      onload_complete_(false),
+      pageload_complete_(false),
       weak_factory_(this),
       unsafe_unique_page_id_(-1) {
   DCHECK(tab);
@@ -389,7 +389,7 @@ void ClientSideDetectionHost::DidNavigateMainFrame(
 
   should_extract_malware_features_ = true;
   should_classify_for_malware_ = false;
-  onload_complete_ = false;
+  pageload_complete_ = false;
 
   // Check whether we can cassify the current URL for phishing or malware.
   classification_request_ = new ShouldClassifyUrlRequest(
@@ -504,19 +504,12 @@ void ClientSideDetectionHost::OnMalwarePreClassificationDone(
   MaybeStartMalwareFeatureExtraction();
 }
 
-void ClientSideDetectionHost::DocumentOnLoadCompletedInMainFrame(
-    int32 page_id) {
+void ClientSideDetectionHost::DidStopLoading(content::RenderViewHost* rvh) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!csd_service_ || !browse_info_.get())
     return;
-  DVLOG(2) << "Main frame onload hander called.";
-  if (browse_info_->page_id != page_id) {
-    // Something weird is happening here.  The BrowseInfo page ID
-    // should always be the same as the most recent load.
-    UMA_HISTOGRAM_BOOLEAN("SBClientMalware.UnexpectedPageId", 1);
-    return;
-  }
-  onload_complete_ = true;
+  DVLOG(2) << "Page finished loading.";
+  pageload_complete_ = true;
   MaybeStartMalwareFeatureExtraction();
 }
 
@@ -524,7 +517,7 @@ void ClientSideDetectionHost::MaybeStartMalwareFeatureExtraction() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (csd_service_ && browse_info_.get() &&
       should_classify_for_malware_ &&
-      onload_complete_) {
+      pageload_complete_) {
     scoped_ptr<ClientMalwareRequest> malware_request(
         new ClientMalwareRequest);
     // Start browser-side malware feature extraction.  Once we're done it will
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.h b/chrome/browser/safe_browsing/client_side_detection_host.h
index 1492f13..1db9f29 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.h
+++ b/chrome/browser/safe_browsing/client_side_detection_host.h
@@ -125,9 +125,9 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // Inherited from WebContentsObserver.  This is called once the onload handler
-  // is called.
-  virtual void DocumentOnLoadCompletedInMainFrame(int32 page_id) OVERRIDE;
+  // Inherited from WebContentsObserver.  This is called once the page is
+  // done loading.
+  virtual void DidStopLoading(content::RenderViewHost* rvh) OVERRIDE;
 
   // Returns true if the user has seen a regular SafeBrowsing
   // interstitial for the current page.  This is only true if the user has
@@ -169,7 +169,7 @@ class ClientSideDetectionHost : public content::WebContentsObserver,
 
   bool should_extract_malware_features_;
   bool should_classify_for_malware_;
-  bool onload_complete_;
+  bool pageload_complete_;
 
   base::WeakPtrFactory<ClientSideDetectionHost> weak_factory_;
 
diff --git a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
index 784182b..132bb84 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
@@ -218,8 +218,8 @@ class ClientSideDetectionHostTest : public ChromeRenderViewHostTestHarness {
     csd_host_->set_client_side_detection_service(csd_service_.get());
     csd_host_->set_safe_browsing_managers(ui_manager_.get(),
                                           database_manager_.get());
-    // We need to create this here since we don't call
-    // DidNavigateMainFramePostCommit in this test.
+    // We need to create this here since we don't call DidStopLanding in
+    // this test.
     csd_host_->browse_info_.reset(new BrowseInfo);
   }
 
@@ -246,8 +246,8 @@ class ClientSideDetectionHostTest : public ChromeRenderViewHostTestHarness {
     csd_host_->OnPhishingDetectionDone(verdict_str);
   }
 
-  void DocumentOnLoadCompletedInMainFrame(int32 page_id) {
-    csd_host_->DocumentOnLoadCompletedInMainFrame(page_id);
+  void DidStopLoading() {
+    csd_host_->DidStopLoading(pending_rvh());
   }
 
   void UpdateIPUrlMap(const std::string& ip, const std::string& host) {
@@ -767,7 +767,7 @@ TEST_F(ClientSideDetectionHostTest,
 }
 
 TEST_F(ClientSideDetectionHostTest,
-       DocumentOnLoadCompletedInMainFrameShowMalwareInterstitial) {
+       DidStopLoadingShowMalwareInterstitial) {
   // Case 9: client thinks the page match malware IP and so does the server.
   // We show an sub-resource malware interstitial.
   MockBrowserFeatureExtractor* mock_extractor =
@@ -798,7 +798,7 @@ TEST_F(ClientSideDetectionHostTest,
               SendClientReportMalwareRequest(
                   Pointee(PartiallyEqualMalwareVerdict(malware_verdict)), _))
       .WillOnce(DoAll(DeleteArg<0>(), SaveArg<1>(&cb)));
-  DocumentOnLoadCompletedInMainFrame(GetBrowseInfo()->page_id);
+  DidStopLoading();
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
   ASSERT_FALSE(cb.is_null());