Pass in ClientPhishingRequest as a unique_ptr.

Previously, the ClientPhishingRequest was passed
as a raw pointer into SendClientPhishingRequest
and then converted back to a unique_ptr when
passed into StartClientPhishingRequest. When
using a raw pointer,it has to be made sure
to be deleted otherwise there is a memory leak.
To make this easier to maintain, it is switched
to a unique_ptr.

Bug: 1122226
Change-Id: Ia344ea26661eec202cd27224889ba398f8264835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379252Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Bettina Dea <bdea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807150}

chrome/browser/safe_browsing/client_side_detection_host.cc

@@ -567,8 +567,7 @@ void ClientSideDetectionHost::FeatureExtractionDone(
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
   // Send ping even if the browser feature extraction failed.
   csd_service_->SendClientReportPhishingRequest(
-      request.release(),  // The service takes ownership of the request object.
-      IsExtendedReportingEnabled(*profile->GetPrefs()),
+      std::move(request), IsExtendedReportingEnabled(*profile->GetPrefs()),
       IsEnhancedProtectionEnabled(*profile->GetPrefs()), callback);
 }
 

chrome/browser/safe_browsing/client_side_detection_host_browsertest.cc

@@ -29,15 +29,11 @@ class FakeClientSideDetectionService : public ClientSideDetectionService {
   FakeClientSideDetectionService() : ClientSideDetectionService(nullptr) {}
 
   void SendClientReportPhishingRequest(
-      ClientPhishingRequest* verdict,
+      std::unique_ptr<ClientPhishingRequest> verdict,
       bool is_extended_reporting,
       bool is_enhanced_protection,
       const ClientReportPhishingRequestCallback& callback) override {
     saved_request_ = *verdict;
-    // TODO(drubery): This can be removed if SendClientReportPhishingRequest
-    // takes a unique_ptr<ClientPhishingRequest>, while also providing better
-    // guarantees about memory safety.
-    delete verdict;
     saved_callback_ = callback;
     request_callback_.Run();
   }

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

@@ -96,9 +96,9 @@ namespace {
 // buffer strings because the BrowserFeatureExtractor might add features to the
 // verdict object before calling SendClientReportPhishingRequest.
 MATCHER_P(PartiallyEqualVerdict, other, "") {
-  return (other.url() == arg.url() &&
-          other.client_score() == arg.client_score() &&
-          other.is_phishing() == arg.is_phishing());
+  return (other.url() == arg->url() &&
+          other.client_score() == arg->client_score() &&
+          other.is_phishing() == arg->is_phishing());
 }
 
 // Test that the callback is nullptr when the verdict is not phishing.
@@ -129,7 +129,7 @@ class MockClientSideDetectionService : public ClientSideDetectionService {
   ~MockClientSideDetectionService() override {}
 
   MOCK_METHOD4(SendClientReportPhishingRequest,
-               void(ClientPhishingRequest*,
+               void(std::unique_ptr<ClientPhishingRequest>,
                     bool,
                     bool,
                     const ClientReportPhishingRequestCallback&));
@@ -498,10 +498,10 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneNotPhishing) {
         request->CopyFrom(verdict);
         std::move(callback).Run(true, std::move(request));
       }));
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(
-                  Pointee(PartiallyEqualVerdict(verdict)), _, _, _))
-      .WillOnce(DoAll(DeleteArg<0>(), SaveArg<3>(&cb)));
+
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
+                                 PartiallyEqualVerdict(verdict), _, _, _))
+      .WillOnce(SaveArg<3>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   ASSERT_FALSE(cb.is_null());
@@ -533,10 +533,9 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneDisabled) {
         request->CopyFrom(verdict);
         std::move(callback).Run(true, std::move(request));
       }));
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(
-                  Pointee(PartiallyEqualVerdict(verdict)), _, _, _))
-      .WillOnce(DoAll(DeleteArg<0>(), SaveArg<3>(&cb)));
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
+                                 PartiallyEqualVerdict(verdict), _, _, _))
+      .WillOnce(SaveArg<3>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   ASSERT_FALSE(cb.is_null());
@@ -569,10 +568,9 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneShowInterstitial) {
         request->CopyFrom(verdict);
         std::move(callback).Run(true, std::move(request));
       }));
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(
-                  Pointee(PartiallyEqualVerdict(verdict)), _, _, _))
-      .WillOnce(DoAll(DeleteArg<0>(), SaveArg<3>(&cb)));
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
+                                 PartiallyEqualVerdict(verdict), _, _, _))
+      .WillOnce(SaveArg<3>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
@@ -623,10 +621,9 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneMultiplePings) {
         request->CopyFrom(verdict);
         std::move(callback).Run(true, std::move(request));
       }));
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(
-                  Pointee(PartiallyEqualVerdict(verdict)), _, _, _))
-      .WillOnce(DoAll(DeleteArg<0>(), SaveArg<3>(&cb)));
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
+                                 PartiallyEqualVerdict(verdict), _, _, _))
+      .WillOnce(SaveArg<3>(&cb));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
@@ -648,16 +645,14 @@ TEST_F(ClientSideDetectionHostTest, PhishingDetectionDoneMultiplePings) {
   ClientSideDetectionService::ClientReportPhishingRequestCallback cb_other;
   verdict.set_url(other_phishing_url.spec());
   verdict.set_client_score(0.8f);
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(
-                  Pointee(PartiallyEqualVerdict(verdict)), _, _, _))
-      .WillOnce(
-          DoAll(DeleteArg<0>(), SaveArg<3>(&cb_other), QuitUIMessageLoop()));
+  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
+                                 PartiallyEqualVerdict(verdict), _, _, _))
+      .WillOnce(SaveArg<3>(&cb_other));
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(other_phishing_url);
   SetRedirectChain(redirect_chain);
   PhishingDetectionDone(verdict.SerializeAsString());
-  base::RunLoop().Run();
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
   EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
   ASSERT_FALSE(cb_other.is_null());
@@ -721,10 +716,10 @@ TEST_F(ClientSideDetectionHostTest,
   WaitAndCheckPreClassificationChecks();
   SetUnsafeSubResourceForCurrent(true /* expect_unsafe_resource */);
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 Pointee(PartiallyEqualVerdict(verdict)), _, _,
-                                 CallbackIsNull()))
-      .WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
+                                              _, CallbackIsNull()))
+      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
@@ -761,10 +756,10 @@ TEST_F(ClientSideDetectionHostTest,
   NavigateWithSBHitAndCommit(url);
   WaitAndCheckPreClassificationChecks();
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 Pointee(PartiallyEqualVerdict(verdict)), _, _,
-                                 CallbackIsNull()))
-      .WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
+                                              _, CallbackIsNull()))
+      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
@@ -820,10 +815,10 @@ TEST_F(
   // Even though we have a pending navigation, the DidShowSBInterstitial check
   // should apply to the committed navigation, so we should get a report even
   // though the verdict has is_phishing = false.
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 Pointee(PartiallyEqualVerdict(verdict)), _, _,
-                                 CallbackIsNull()))
-      .WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
+                                              _, CallbackIsNull()))
+      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
@@ -1165,10 +1160,10 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectionDuration) {
   const base::TimeDelta duration = base::TimeDelta::FromMilliseconds(10);
   AdvanceTimeTickClock(duration);
 
-  EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(
-                                 Pointee(PartiallyEqualVerdict(verdict)), _, _,
-                                 CallbackIsNull()))
-      .WillOnce(DoAll(DeleteArg<0>(), QuitUIMessageLoop()));
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
+                                              _, CallbackIsNull()))
+      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
@@ -1211,7 +1206,7 @@ TEST_F(ClientSideDetectionHostTest, ClearsScreenshotData) {
   verdict.set_screenshot_phash("screenshot_phash");
   verdict.set_phash_dimension_size(48);
 
-  ClientPhishingRequest* request;
+  ClientPhishingRequest request;
 
   EXPECT_CALL(*mock_extractor, ExtractFeatures(_, _, _))
       .WillOnce(Invoke([&](const BrowseInfo* into,
@@ -1220,13 +1215,12 @@ TEST_F(ClientSideDetectionHostTest, ClearsScreenshotData) {
         std::move(callback).Run(true, std::move(request));
       }));
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-      .WillOnce(SaveArg<0>(&request));
+      .WillOnce(testing::SaveArgPointee<0>(&request));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
-  EXPECT_FALSE(request->has_phash_dimension_size());
-  EXPECT_FALSE(request->has_screenshot_phash());
-  EXPECT_FALSE(request->has_screenshot_digest());
-  delete request;
+  EXPECT_FALSE(request.has_phash_dimension_size());
+  EXPECT_FALSE(request.has_screenshot_phash());
+  EXPECT_FALSE(request.has_screenshot_digest());
 }
 
 TEST_F(ClientSideDetectionHostTest, AllowsScreenshotDataForSBER) {
@@ -1245,7 +1239,7 @@ TEST_F(ClientSideDetectionHostTest, AllowsScreenshotDataForSBER) {
   verdict.set_screenshot_phash("screenshot_phash");
   verdict.set_phash_dimension_size(48);
 
-  ClientPhishingRequest* request;
+  ClientPhishingRequest request;
 
   EXPECT_CALL(*mock_extractor, ExtractFeatures(_, _, _))
       .WillOnce(Invoke([&](const BrowseInfo* into,
@@ -1254,13 +1248,12 @@ TEST_F(ClientSideDetectionHostTest, AllowsScreenshotDataForSBER) {
         std::move(callback).Run(true, std::move(request));
       }));
   EXPECT_CALL(*csd_service_, SendClientReportPhishingRequest(_, _, _, _))
-      .WillOnce(SaveArg<0>(&request));
+      .WillOnce(testing::SaveArgPointee<0>(&request));
   PhishingDetectionDone(verdict.SerializeAsString());
   EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
-  EXPECT_TRUE(request->has_phash_dimension_size());
-  EXPECT_TRUE(request->has_screenshot_phash());
-  EXPECT_TRUE(request->has_screenshot_digest());
-  delete request;
+  EXPECT_TRUE(request.has_phash_dimension_size());
+  EXPECT_TRUE(request.has_screenshot_phash());
+  EXPECT_TRUE(request.has_screenshot_digest());
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/client_side_detection_service.cc

@@ -154,7 +154,7 @@ void ClientSideDetectionService::OnPrefsUpdated() {
 }
 
 void ClientSideDetectionService::SendClientReportPhishingRequest(
-    ClientPhishingRequest* verdict,
+    std::unique_ptr<ClientPhishingRequest> verdict,
     bool is_extended_reporting,
     bool is_enhanced_reporting,
     const ClientReportPhishingRequestCallback& callback) {
@@ -163,7 +163,7 @@ void ClientSideDetectionService::SendClientReportPhishingRequest(
       FROM_HERE,
       base::BindOnce(
           &ClientSideDetectionService::StartClientReportPhishingRequest,
-          weak_factory_.GetWeakPtr(), verdict, is_extended_reporting,
+          weak_factory_.GetWeakPtr(), std::move(verdict), is_extended_reporting,
           is_enhanced_reporting, callback));
 }
 
@@ -213,12 +213,11 @@ void ClientSideDetectionService::SendModelToRenderers() {
 }
 
 void ClientSideDetectionService::StartClientReportPhishingRequest(
-    ClientPhishingRequest* verdict,
+    std::unique_ptr<ClientPhishingRequest> request,
     bool is_extended_reporting,
     bool is_enhanced_reporting,
     const ClientReportPhishingRequestCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  std::unique_ptr<ClientPhishingRequest> request(verdict);
 
   if (!enabled_) {
     if (!callback.is_null())

chrome/browser/safe_browsing/client_side_detection_service.h

@@ -82,7 +82,7 @@ class ClientSideDetectionService : public KeyedService {
   // SendClientReportPhishingRequest() was called.  You may set |callback| to
   // NULL if you don't care about the server verdict.
   virtual void SendClientReportPhishingRequest(
-      ClientPhishingRequest* verdict,
+      std::unique_ptr<ClientPhishingRequest> verdict,
       bool is_extended_reporting,
       bool is_enhanced_protection,
       const ClientReportPhishingRequestCallback& callback);
@@ -167,7 +167,7 @@ class ClientSideDetectionService : public KeyedService {
   // Starts sending the request to the client-side detection frontends.
   // This method takes ownership of both pointers.
   void StartClientReportPhishingRequest(
-      ClientPhishingRequest* verdict,
+      std::unique_ptr<ClientPhishingRequest> request,
       bool is_extended_reporting,
       bool is_enhanced_protection,
       const ClientReportPhishingRequestCallback& callback);

chrome/browser/safe_browsing/client_side_detection_service_unittest.cc

@@ -86,13 +86,15 @@ class ClientSideDetectionServiceTest : public testing::Test {
                                        float score,
                                        bool is_extended_reporting,
                                        bool is_enhanced_reporting) {
-    ClientPhishingRequest* request = new ClientPhishingRequest();
+    std::unique_ptr<ClientPhishingRequest> request =
+        std::make_unique<ClientPhishingRequest>(ClientPhishingRequest());
     request->set_url(phishing_url.spec());
     request->set_client_score(score);
     request->set_is_phishing(true);  // client thinks the URL is phishing.
+
     base::RunLoop run_loop;
     csd_service_->SendClientReportPhishingRequest(
-        request, is_extended_reporting, is_enhanced_reporting,
+        std::move(request), is_extended_reporting, is_enhanced_reporting,
         base::Bind(&ClientSideDetectionServiceTest::SendRequestDone,
                    base::Unretained(this), run_loop.QuitWhenIdleClosure()));
     phishing_url_ = phishing_url;