Replace raw pointers with smart pointers in update_client::URLRequestPostInterceptor

This is a mechanical change. Understanding ownership of various objects such as interceptors
is difficult in the current implementation, and it is prone to memory leaks and
accessing dangling objects.

There is follow-up work to implement support for pausing such interceptors.

Bug: 828600
Change-Id: Ic88e76be4adc45e0f341f13ff888fd1612b157a6
Reviewed-on: https://chromium-review.googlesource.com/991471Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548094}

chrome/browser/policy/policy_browsertest.cc

@@ -4416,11 +4416,10 @@ class ComponentUpdaterPolicyTest : public PolicyTest {
   std::unique_ptr<update_client::URLRequestPostInterceptorFactory>
       interceptor_factory_;
 
-  // This member is owned by the |interceptor_factory_|.
-  update_client::URLRequestPostInterceptor* post_interceptor_ = nullptr;
+  scoped_refptr<update_client::URLRequestPostInterceptor> post_interceptor_;
 
   // This member is owned by g_browser_process;
-  component_updater::ComponentUpdateService* cus_;
+  component_updater::ComponentUpdateService* cus_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(ComponentUpdaterPolicyTest);
 };
@@ -4489,7 +4488,7 @@ void ComponentUpdaterPolicyTest::UpdateComponent(
     const update_client::CrxComponent& crx_component) {
   post_interceptor_->Reset();
   EXPECT_TRUE(post_interceptor_->ExpectRequest(
-      new update_client::PartialMatch("updatecheck"), 200));
+      std::make_unique<update_client::PartialMatch>("updatecheck")));
   EXPECT_TRUE(cus_->RegisterComponent(crx_component));
   cus_->GetOnDemandUpdater().OnDemandUpdate(
       component_id_, base::Bind(&ComponentUpdaterPolicyTest::OnDemandComplete,
@@ -4530,6 +4529,7 @@ void ComponentUpdaterPolicyTest::BeginTest() {
 }
 
 void ComponentUpdaterPolicyTest::EndTest() {
+  post_interceptor_ = nullptr;
   interceptor_factory_ = nullptr;
   cus_ = nullptr;
 

components/update_client/ping_manager_unittest.cc

@@ -106,7 +106,7 @@ scoped_refptr<UpdateContext> PingManagerTest::MakeMockUpdateContext() const {
 TEST_F(PingManagerTest, SendPing) {
   auto interceptor_factory =
       std::make_unique<InterceptorFactory>(base::ThreadTaskRunnerHandle::Get());
-  auto* interceptor = interceptor_factory->CreateInterceptor();
+  auto interceptor = interceptor_factory->CreateInterceptor();
   EXPECT_TRUE(interceptor);
 
   // Test eventresult="1" is sent for successful updates.
@@ -120,7 +120,7 @@ TEST_F(PingManagerTest, SendPing) {
     component.next_version_ = base::Version("2.0");
     component.AppendEvent(BuildUpdateCompleteEventElement(component));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 
@@ -153,7 +153,7 @@ TEST_F(PingManagerTest, SendPing) {
     component.next_version_ = base::Version("2.0");
     component.AppendEvent(BuildUpdateCompleteEventElement(component));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 
@@ -185,7 +185,7 @@ TEST_F(PingManagerTest, SendPing) {
     component.crx_diffurls_.push_back(GURL("http://host/path"));
     component.AppendEvent(BuildUpdateCompleteEventElement(component));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 
@@ -212,7 +212,7 @@ TEST_F(PingManagerTest, SendPing) {
 
     component.AppendEvent(BuildUpdateCompleteEventElement(component));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 
@@ -232,7 +232,7 @@ TEST_F(PingManagerTest, SendPing) {
     component.Uninstall(base::Version("1.2.3.4"), 0);
     component.AppendEvent(BuildUninstalledEventElement(component));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 
@@ -274,7 +274,7 @@ TEST_F(PingManagerTest, SendPing) {
     component.AppendEvent(
         BuildDownloadCompleteEventElement(component, download_metrics));
 
-    EXPECT_TRUE(interceptor->ExpectRequest(new AnyMatch));
+    EXPECT_TRUE(interceptor->ExpectRequest(std::make_unique<AnyMatch>()));
     ping_manager_->SendPing(component, MakePingCallback());
     RunThreads();
 

components/update_client/request_sender_unittest.cc

@@ -65,9 +65,8 @@ class RequestSenderTest : public testing::Test,
   std::unique_ptr<RequestSender> request_sender_;
   std::unique_ptr<InterceptorFactory> interceptor_factory_;
 
-  // Owned by the factory.
-  URLRequestPostInterceptor* post_interceptor_1_ = nullptr;
-  URLRequestPostInterceptor* post_interceptor_2_ = nullptr;
+  scoped_refptr<URLRequestPostInterceptor> post_interceptor_1_;
+  scoped_refptr<URLRequestPostInterceptor> post_interceptor_2_;
 
   int error_ = 0;
   std::string response_;
@@ -88,6 +87,8 @@ RequestSenderTest::~RequestSenderTest() {}
 
 void RequestSenderTest::SetUp() {
   config_ = base::MakeRefCounted<TestConfigurator>();
+  request_sender_ = std::make_unique<RequestSender>(config_);
+
   interceptor_factory_ =
       std::make_unique<InterceptorFactory>(base::ThreadTaskRunnerHandle::Get());
   post_interceptor_1_ =
@@ -96,8 +97,6 @@ void RequestSenderTest::SetUp() {
       interceptor_factory_->CreateInterceptorForPath(kUrlPath2);
   EXPECT_TRUE(post_interceptor_1_);
   EXPECT_TRUE(post_interceptor_2_);
-
-  request_sender_ = nullptr;
 }
 
 void RequestSenderTest::TearDown() {
@@ -137,15 +136,15 @@ void RequestSenderTest::RequestSenderComplete(int error,
 // Tests that when a request to the first url succeeds, the subsequent urls are
 // not tried.
 TEST_P(RequestSenderTest, RequestSendSuccess) {
-  EXPECT_TRUE(post_interceptor_1_->ExpectRequest(
-      new PartialMatch("test"), test_file("updatecheck_reply_1.xml")));
+  EXPECT_TRUE(
+      post_interceptor_1_->ExpectRequest(std::make_unique<PartialMatch>("test"),
+                                         test_file("updatecheck_reply_1.xml")));
 
-  const std::vector<GURL> urls = {GURL(kUrl1), GURL(kUrl2)};
   const bool is_foreground = GetParam();
-  request_sender_ = std::make_unique<RequestSender>(config_);
   request_sender_->Send(
-      urls, {{"X-Goog-Update-Interactivity", is_foreground ? "fg" : "bg"}},
-      "test", false,
+      {GURL(kUrl1), GURL(kUrl2)},
+      {{"X-Goog-Update-Interactivity", is_foreground ? "fg" : "bg"}}, "test",
+      false,
       base::BindOnce(&RequestSenderTest::RequestSenderComplete,
                      base::Unretained(this)));
   RunThreads();
@@ -177,21 +176,18 @@ TEST_P(RequestSenderTest, RequestSendSuccess) {
   std::string header;
   extra_request_headers.GetHeader("X-Goog-Update-Interactivity", &header);
   EXPECT_STREQ(is_foreground ? "fg" : "bg", header.c_str());
-
-  interceptor_factory_ = nullptr;
 }
 
 // Tests that the request succeeds using the second url after the first url
 // has failed.
 TEST_F(RequestSenderTest, RequestSendSuccessWithFallback) {
-  EXPECT_TRUE(
-      post_interceptor_1_->ExpectRequest(new PartialMatch("test"), 403));
-  EXPECT_TRUE(post_interceptor_2_->ExpectRequest(new PartialMatch("test")));
+  EXPECT_TRUE(post_interceptor_1_->ExpectRequest(
+      std::make_unique<PartialMatch>("test"), 403));
+  EXPECT_TRUE(post_interceptor_2_->ExpectRequest(
+      std::make_unique<PartialMatch>("test")));
 
-  const std::vector<GURL> urls = {GURL(kUrl1), GURL(kUrl2)};
-  request_sender_ = std::make_unique<RequestSender>(config_);
   request_sender_->Send(
-      urls, {}, "test", false,
+      {GURL(kUrl1), GURL(kUrl2)}, {}, "test", false,
       base::BindOnce(&RequestSenderTest::RequestSenderComplete,
                      base::Unretained(this)));
   RunThreads();
@@ -212,10 +208,10 @@ TEST_F(RequestSenderTest, RequestSendSuccessWithFallback) {
 
 // Tests that the request fails when both urls have failed.
 TEST_F(RequestSenderTest, RequestSendFailed) {
-  EXPECT_TRUE(
-      post_interceptor_1_->ExpectRequest(new PartialMatch("test"), 403));
-  EXPECT_TRUE(
-      post_interceptor_2_->ExpectRequest(new PartialMatch("test"), 403));
+  EXPECT_TRUE(post_interceptor_1_->ExpectRequest(
+      std::make_unique<PartialMatch>("test"), 403));
+  EXPECT_TRUE(post_interceptor_2_->ExpectRequest(
+      std::make_unique<PartialMatch>("test"), 403));
 
   const std::vector<GURL> urls = {GURL(kUrl1), GURL(kUrl2)};
   request_sender_ = std::make_unique<RequestSender>(config_);
@@ -254,8 +250,9 @@ TEST_F(RequestSenderTest, RequestSendFailedNoUrls) {
 
 // Tests that a CUP request fails if the response is not signed.
 TEST_F(RequestSenderTest, RequestSendCupError) {
-  EXPECT_TRUE(post_interceptor_1_->ExpectRequest(
-      new PartialMatch("test"), test_file("updatecheck_reply_1.xml")));
+  EXPECT_TRUE(
+      post_interceptor_1_->ExpectRequest(std::make_unique<PartialMatch>("test"),
+                                         test_file("updatecheck_reply_1.xml")));
 
   const std::vector<GURL> urls = {GURL(kUrl1)};
   request_sender_ = std::make_unique<RequestSender>(config_);

components/update_client/url_request_post_interceptor.cc

@@ -67,15 +67,6 @@ URLRequestPostInterceptor::URLRequestPostInterceptor(
 
 URLRequestPostInterceptor::~URLRequestPostInterceptor() {
   DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
-  ClearExpectations();
-}
-
-void URLRequestPostInterceptor::ClearExpectations() {
-  while (!expectations_.empty()) {
-    Expectation expectation(expectations_.front());
-    delete expectation.first;
-    expectations_.pop();
-  }
 }
 
 GURL URLRequestPostInterceptor::GetUrl() const {
@@ -83,29 +74,27 @@ GURL URLRequestPostInterceptor::GetUrl() const {
 }
 
 bool URLRequestPostInterceptor::ExpectRequest(
-    class RequestMatcher* request_matcher) {
-  expectations_.push(std::make_pair(request_matcher,
-                                    ExpectationResponse(kResponseCode200, "")));
-  return true;
+    std::unique_ptr<RequestMatcher> request_matcher) {
+  return ExpectRequest(std::move(request_matcher), kResponseCode200);
 }
 
 bool URLRequestPostInterceptor::ExpectRequest(
-    class RequestMatcher* request_matcher,
+    std::unique_ptr<RequestMatcher> request_matcher,
     int response_code) {
   expectations_.push(
-      std::make_pair(request_matcher, ExpectationResponse(response_code, "")));
+      {std::move(request_matcher), ExpectationResponse(response_code, "")});
   return true;
 }
 
 bool URLRequestPostInterceptor::ExpectRequest(
-    class RequestMatcher* request_matcher,
+    std::unique_ptr<RequestMatcher> request_matcher,
     const base::FilePath& filepath) {
   std::string response;
   if (filepath.empty() || !base::ReadFileToString(filepath, &response))
     return false;
 
-  expectations_.push(std::make_pair(
-      request_matcher, ExpectationResponse(kResponseCode200, response)));
+  expectations_.push({std::move(request_matcher),
+                      ExpectationResponse(kResponseCode200, response)});
   return true;
 }
 
@@ -146,7 +135,7 @@ void URLRequestPostInterceptor::Reset() {
   base::AutoLock auto_lock(interceptor_lock_);
   hit_count_ = 0;
   requests_.clear();
-  ClearExpectations();
+  base::queue<Expectation>().swap(expectations_);
 }
 
 class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
@@ -164,16 +153,15 @@ class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
 
   void Unregister() {
     DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
-    for (InterceptorMap::iterator it = interceptors_.begin();
-         it != interceptors_.end(); ++it)
-      delete (*it).second;
+    interceptors_.clear();
     net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(scheme_,
                                                                 hostname_);
   }
 
-  void OnCreateInterceptor(URLRequestPostInterceptor* interceptor) {
+  void OnCreateInterceptor(
+      scoped_refptr<URLRequestPostInterceptor> interceptor) {
     DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
-    DCHECK(interceptors_.find(interceptor->GetUrl()) == interceptors_.end());
+    DCHECK_EQ(0u, interceptors_.count(interceptor->GetUrl()));
 
     interceptors_.insert(std::make_pair(interceptor->GetUrl(), interceptor));
   }
@@ -197,7 +185,7 @@ class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
       url = url.ReplaceComponents(replacements);
     }
 
-    InterceptorMap::const_iterator it(interceptors_.find(url));
+    const auto it = interceptors_.find(url);
     if (it == interceptors_.end())
       return nullptr;
 
@@ -205,7 +193,7 @@ class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
     // check the existing expectations, and handle the matching case by
     // popping the expectation off the queue, counting the match, and
     // returning a mock object to serve the canned response.
-    URLRequestPostInterceptor* interceptor(it->second);
+    auto interceptor = it->second;
 
     const net::UploadDataStream* stream = request->get_upload();
     const net::UploadBytesElementReader* reader =
@@ -220,15 +208,12 @@ class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
           {request_body, request->extra_request_headers()});
       if (interceptor->expectations_.empty())
         return nullptr;
-      const URLRequestPostInterceptor::Expectation& expectation(
-          interceptor->expectations_.front());
+      const auto& expectation = interceptor->expectations_.front();
       if (expectation.first->Match(request_body)) {
         const int response_code(expectation.second.response_code);
         const std::string response_body(expectation.second.response_body);
-        delete expectation.first;
         interceptor->expectations_.pop();
         ++interceptor->hit_count_;
-
         return new URLRequestMockJob(request, network_delegate, response_code,
                                      response_body);
       }
@@ -237,7 +222,8 @@ class URLRequestPostInterceptor::Delegate : public net::URLRequestInterceptor {
     return nullptr;
   }
 
-  typedef std::map<GURL, URLRequestPostInterceptor*> InterceptorMap;
+  using InterceptorMap =
+      std::map<GURL, scoped_refptr<URLRequestPostInterceptor>>;
   InterceptorMap interceptors_;
 
   const std::string scheme_;
@@ -269,24 +255,19 @@ URLRequestPostInterceptorFactory::~URLRequestPostInterceptorFactory() {
                      base::Unretained(delegate_)));
 }
 
-URLRequestPostInterceptor* URLRequestPostInterceptorFactory::CreateInterceptor(
+scoped_refptr<URLRequestPostInterceptor>
+URLRequestPostInterceptorFactory::CreateInterceptor(
     const base::FilePath& filepath) {
   const GURL base_url(
       base::StringPrintf("%s://%s", scheme_.c_str(), hostname_.c_str()));
   GURL absolute_url(base_url.Resolve(filepath.MaybeAsASCII()));
-  URLRequestPostInterceptor* interceptor(
+  auto interceptor = scoped_refptr<URLRequestPostInterceptor>(
       new URLRequestPostInterceptor(absolute_url, io_task_runner_));
   bool res = io_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&URLRequestPostInterceptor::Delegate::OnCreateInterceptor,
-                     base::Unretained(delegate_),
-                     base::Unretained(interceptor)));
-  if (!res) {
-    delete interceptor;
-    return nullptr;
-  }
-
-  return interceptor;
+                     base::Unretained(delegate_), interceptor));
+  return res ? interceptor : nullptr;
 }
 
 bool PartialMatch::Match(const std::string& actual) const {
@@ -306,12 +287,13 @@ InterceptorFactory::InterceptorFactory(
 InterceptorFactory::~InterceptorFactory() {
 }
 
-URLRequestPostInterceptor* InterceptorFactory::CreateInterceptor() {
+scoped_refptr<URLRequestPostInterceptor>
+InterceptorFactory::CreateInterceptor() {
   return CreateInterceptorForPath(POST_INTERCEPT_PATH);
 }
 
-URLRequestPostInterceptor* InterceptorFactory::CreateInterceptorForPath(
-    const char* url_path) {
+scoped_refptr<URLRequestPostInterceptor>
+InterceptorFactory::CreateInterceptorForPath(const char* url_path) {
   return URLRequestPostInterceptorFactory::CreateInterceptor(
       base::FilePath::FromUTF8Unsafe(url_path));
 }

components/update_client/url_request_post_interceptor.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 #include <map>
+#include <memory>
 #include <string>
 #include <utility>
 #include <vector>
@@ -33,7 +34,8 @@ namespace update_client {
 // from a given file. The class maintains a queue of expectations, and returns
 // one and only one response for each request that matches the expectation.
 // Then, the expectation is removed from the queue.
-class URLRequestPostInterceptor {
+class URLRequestPostInterceptor
+    : public base::RefCountedThreadSafe<URLRequestPostInterceptor> {
  public:
   using InterceptedRequest = std::pair<std::string, net::HttpRequestHeaders>;
   // Allows a generic string maching interface when setting up expectations.
@@ -51,11 +53,11 @@ class URLRequestPostInterceptor {
   // the expectation is met. If no |file_path| is provided, then an empty
   // response body is served. If |response_code| is provided, then an empty
   // response body with that response code is returned.
-  // Returns |true| if the expectation was set. This class takes ownership of
-  // the |request_matcher| object.
-  bool ExpectRequest(class RequestMatcher* request_matcher);
-  bool ExpectRequest(class RequestMatcher* request_matcher, int response_code);
-  bool ExpectRequest(class RequestMatcher* request_matcher,
+  // Returns |true| if the expectation was set.
+  bool ExpectRequest(std::unique_ptr<RequestMatcher> request_matcher);
+  bool ExpectRequest(std::unique_ptr<RequestMatcher> request_matcher,
+                     int response_code);
+  bool ExpectRequest(std::unique_ptr<RequestMatcher> request_matcher,
                      const base::FilePath& filepath);
 
   // Returns how many requests have been intercepted and matched by
@@ -81,6 +83,7 @@ class URLRequestPostInterceptor {
 
  private:
   friend class URLRequestPostInterceptorFactory;
+  friend class base::RefCountedThreadSafe<URLRequestPostInterceptor>;
 
   static const int kResponseCode200 = 200;
 
@@ -90,7 +93,8 @@ class URLRequestPostInterceptor {
     const int response_code;
     const std::string response_body;
   };
-  typedef std::pair<const RequestMatcher*, ExpectationResponse> Expectation;
+  using Expectation =
+      std::pair<std::unique_ptr<RequestMatcher>, ExpectationResponse>;
 
   URLRequestPostInterceptor(
       const GURL& url,
@@ -125,10 +129,9 @@ class URLRequestPostInterceptorFactory {
       scoped_refptr<base::SequencedTaskRunner> io_task_runner);
   ~URLRequestPostInterceptorFactory();
 
-  // Creates an interceptor object for the specified url path. Returns NULL
-  // in case of errors or a valid interceptor object otherwise. The caller
-  // does not own the returned object.
-  URLRequestPostInterceptor* CreateInterceptor(const base::FilePath& filepath);
+  // Creates an interceptor object for the specified url path.
+  scoped_refptr<URLRequestPostInterceptor> CreateInterceptor(
+      const base::FilePath& filepath);
 
  private:
   const std::string scheme_;
@@ -151,10 +154,11 @@ class InterceptorFactory : public URLRequestPostInterceptorFactory {
   ~InterceptorFactory();
 
   // Creates an interceptor for the url path defined by POST_INTERCEPT_PATH.
-  URLRequestPostInterceptor* CreateInterceptor();
+  scoped_refptr<URLRequestPostInterceptor> CreateInterceptor();
 
   // Creates an interceptor for the given url path.
-  URLRequestPostInterceptor* CreateInterceptorForPath(const char* url_path);
+  scoped_refptr<URLRequestPostInterceptor> CreateInterceptorForPath(
+      const char* url_path);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(InterceptorFactory);