[sms] Make the lifecycle of SmsServices be managed by FrameServiceBase

In the current architecture, SmsService is owned by SmsKeyedService which
is owned by Profiles. SmsReceiverImpl is owned by SmsServiceImpl and the
StrongBindingSet, which has no connection (from a memory perspective), to
RenderFrameHosts.

There are no guarantees that the mojo clients are going to outlive the
RenderFrameHosts, so we run into possible cases where SmsReceiverImpl is
handling requests after the RenderFrameHost was destructed (@see bugs).

To address this problem, and make the architecture consistent with other
comparable features, this cl makes the SmsService a subclass of
FrameServiceBase, which is responsible for managing its lifecycle and
memory based on events dispatched by the lifecycle of the RenderFrameHost.

We keep the SmsProvider owned by the BrowserMainLoop, which is effectively
a singleton. To facilitate BrowserTest testing, we expose a
BrowserMainLoop::SetSmsProviderForTesting static method.

Design Doc: https://docs.google.com/document/d/1dB5UM9x8Ap2-bs6Xn0KnbC_B1KNLIUv4W05MunuXYh0/edit#

Bug: 979265,982370,979418
Change-Id: Ic2b0a9b35fa1f5a3e28c84d77d78c22d41b09ad0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1691683Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Jun Cai <juncai@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Sam Goto <goto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678558}

content/browser/BUILD.gn

@@ -1826,8 +1826,6 @@ jumbo_source_set("browser") {
     "sms/sms_parser.h",
     "sms/sms_provider.cc",
     "sms/sms_provider.h",
-    "sms/sms_receiver_impl.cc",
-    "sms/sms_receiver_impl.h",
     "sms/sms_service.cc",
     "sms/sms_service.h",
     "speech/speech_recognition_dispatcher_host.cc",

content/browser/browser_main_loop.cc

@@ -86,7 +86,7 @@
 #include "content/browser/scheduler/responsiveness/watcher.h"
 #include "content/browser/screenlock_monitor/screenlock_monitor.h"
 #include "content/browser/screenlock_monitor/screenlock_monitor_device_source.h"
-#include "content/browser/sms/sms_service.h"
+#include "content/browser/sms/sms_provider.h"
 #include "content/browser/speech/speech_recognition_manager_impl.h"
 #include "content/browser/startup_data_impl.h"
 #include "content/browser/startup_task_runner.h"
@@ -1611,11 +1611,16 @@ bool BrowserMainLoop::AudioServiceOutOfProcess() const {
          !GetContentClient()->browser()->OverridesAudioManager();
 }
 
-SmsService* BrowserMainLoop::GetSmsService() {
-  if (!sms_service_) {
-    sms_service_ = std::make_unique<SmsService>();
+SmsProvider* BrowserMainLoop::GetSmsProvider() {
+  if (!sms_provider_) {
+    sms_provider_ = SmsProvider::Create();
   }
-  return sms_service_.get();
+  return sms_provider_.get();
+}
+
+void BrowserMainLoop::SetSmsProviderForTesting(
+    std::unique_ptr<SmsProvider> provider) {
+  sms_provider_ = std::move(provider);
 }
 
 }  // namespace content

content/browser/browser_main_loop.h

@@ -93,7 +93,7 @@ class MediaStreamManager;
 class ResourceDispatcherHostImpl;
 class SaveFileManager;
 class ScreenlockMonitor;
-class SmsService;
+class SmsProvider;
 class SpeechRecognitionManagerImpl;
 class StartupTaskRunner;
 class TracingControllerImpl;
@@ -229,7 +229,8 @@ class CONTENT_EXPORT BrowserMainLoop {
   }
 #endif
 
-  SmsService* GetSmsService();
+  SmsProvider* GetSmsProvider();
+  void SetSmsProviderForTesting(std::unique_ptr<SmsProvider>);
 
   BrowserMainParts* parts() { return parts_.get(); }
 
@@ -379,7 +380,7 @@ class CONTENT_EXPORT BrowserMainLoop {
   // Must be deleted on the IO thread.
   std::unique_ptr<SpeechRecognitionManagerImpl> speech_recognition_manager_;
 
-  std::unique_ptr<SmsService> sms_service_;
+  std::unique_ptr<SmsProvider> sms_provider_;
 
 #if defined(OS_WIN)
   std::unique_ptr<media::SystemMessageWindowWin> system_message_window_;

content/browser/frame_host/render_frame_host_impl.cc

@@ -6157,8 +6157,8 @@ void RenderFrameHostImpl::BindSmsReceiverRequest(
     mojo::ReportBadMessage("Must be in top-level browser context.");
     return;
   }
-  SmsService* sms_service = BrowserMainLoop::GetInstance()->GetSmsService();
-  sms_service->Bind(std::move(request), GetLastCommittedOrigin());
+  auto* provider = BrowserMainLoop::GetInstance()->GetSmsProvider();
+  SmsService::Create(provider, this, std::move(request));
 }
 
 void RenderFrameHostImpl::GetInterface(

content/browser/sms/sms_browsertest.cc

@@ -50,10 +50,9 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Receive) {
   GURL url = GetTestUrl(nullptr, "simple_page.html");
   NavigateToURL(shell(), url);
 
-  auto* mock = new NiceMock<MockSmsProvider>();
-
-  auto* sms_service = BrowserMainLoop::GetInstance()->GetSmsService();
-  sms_service->SetSmsProviderForTest(base::WrapUnique(mock));
+  auto* provider = new NiceMock<MockSmsProvider>();
+  BrowserMainLoop::GetInstance()->SetSmsProviderForTesting(
+      base::WrapUnique(provider));
 
   // Test that SMS content can be retrieved after navigator.sms.receive().
   std::string script = R"(
@@ -63,8 +62,8 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Receive) {
     }) ();
   )";
 
-  EXPECT_CALL(*mock, Retrieve()).WillOnce(Invoke([&mock, &url]() {
-    mock->NotifyReceive(url::Origin::Create(url), "hello");
+  EXPECT_CALL(*provider, Retrieve()).WillOnce(Invoke([&provider, &url]() {
+    provider->NotifyReceive(url::Origin::Create(url), "hello");
   }));
 
   EXPECT_EQ("hello", EvalJs(shell(), script));
@@ -74,10 +73,9 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, ReceiveMultiple) {
   GURL url = GetTestUrl(nullptr, "simple_page.html");
   NavigateToURL(shell(), url);
 
-  auto* mock = new NiceMock<MockSmsProvider>();
-
-  auto* sms_service = BrowserMainLoop::GetInstance()->GetSmsService();
-  sms_service->SetSmsProviderForTest(base::WrapUnique(mock));
+  auto* provider = new NiceMock<MockSmsProvider>();
+  BrowserMainLoop::GetInstance()->SetSmsProviderForTesting(
+      base::WrapUnique(provider));
 
   // Test that SMS content can retrieve multiple messages.
   std::string script = R"(
@@ -92,12 +90,12 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, ReceiveMultiple) {
     }) ();
   )";
 
-  EXPECT_CALL(*mock, Retrieve())
-      .WillOnce(Invoke([&mock, &url]() {
-        mock->NotifyReceive(url::Origin::Create(url), "hello1");
+  EXPECT_CALL(*provider, Retrieve())
+      .WillOnce(Invoke([&provider, &url]() {
+        provider->NotifyReceive(url::Origin::Create(url), "hello1");
       }))
-      .WillOnce(Invoke([&mock, &url]() {
-        mock->NotifyReceive(url::Origin::Create(url), "hello2");
+      .WillOnce(Invoke([&provider, &url]() {
+        provider->NotifyReceive(url::Origin::Create(url), "hello2");
       }));
 
   base::ListValue result = EvalJs(shell(), script).ExtractList();

content/browser/sms/sms_receiver_impl.cc

-// Copyright 2019 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include <iterator>
-#include <queue>
-#include <utility>
-
-#include "content/browser/sms/sms_receiver_impl.h"
-
-#include "base/bind.h"
-#include "base/callback_helpers.h"
-
-namespace content {
-
-SmsReceiverImpl::SmsReceiverImpl(SmsProvider* sms_provider,
-                                 const url::Origin& origin)
-    : sms_provider_(sms_provider), origin_(origin) {}
-
-SmsReceiverImpl::~SmsReceiverImpl() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-}
-
-SmsReceiverImpl::Request::Request(ReceiveCallback callback)
-    : callback(std::move(callback)) {}
-
-SmsReceiverImpl::Request::~Request() = default;
-
-void SmsReceiverImpl::Receive(base::TimeDelta timeout,
-                              ReceiveCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (requests_.empty())
-    sms_provider_->AddObserver(this);
-
-  auto request = std::make_unique<Request>(std::move(callback));
-  // The |timer| is owned by |request|, and |request| is owned by |this|, so it
-  // is safe to hold raw pointers to |this| and |request| here in the callback.
-  request->timer.Start(FROM_HERE, timeout,
-                       base::BindOnce(&SmsReceiverImpl::OnTimeout,
-                                      base::Unretained(this), request.get()));
-  requests_.push_back(std::move(request));
-  sms_provider_->Retrieve();
-}
-
-bool SmsReceiverImpl::OnReceive(const url::Origin& origin,
-                                const std::string& sms) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (origin_ != origin)
-    return false;
-
-  return Pop(sms);
-}
-
-bool SmsReceiverImpl::Pop(const std::string& sms) {
-  DCHECK(!requests_.empty());
-
-  DCHECK(requests_.front()->timer.IsRunning());
-
-  requests_.front()->timer.Stop();
-  std::move(requests_.front()->callback)
-      .Run(blink::mojom::SmsStatus::kSuccess, sms);
-  requests_.pop_front();
-
-  if (requests_.empty())
-    sms_provider_->RemoveObserver(this);
-
-  return true;
-}
-
-void SmsReceiverImpl::OnTimeout(Request* request) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  DCHECK(!request->timer.IsRunning());
-
-  std::move(request->callback)
-      .Run(blink::mojom::SmsStatus::kTimeout, base::nullopt);
-
-  // Remove the request from the list.
-  for (auto iter = requests_.begin(); iter != requests_.end(); ++iter) {
-    if ((*iter).get() == request) {
-      requests_.erase(iter);
-      if (requests_.empty())
-        sms_provider_->RemoveObserver(this);
-      return;
-    }
-  }
-}
-
-}  // namespace content

content/browser/sms/sms_receiver_impl.h

-// Copyright 2019 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CONTENT_BROWSER_SMS_SMS_RECEIVER_IMPL_H_
-#define CONTENT_BROWSER_SMS_SMS_RECEIVER_IMPL_H_
-
-#include <list>
-#include <memory>
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "base/sequence_checker.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
-#include "content/browser/sms/sms_provider.h"
-#include "content/common/content_export.h"
-#include "third_party/blink/public/mojom/sms/sms_receiver.mojom.h"
-#include "url/origin.h"
-
-namespace content {
-
-class CONTENT_EXPORT SmsReceiverImpl : public blink::mojom::SmsReceiver,
-                                       public content::SmsProvider::Observer {
- public:
-  SmsReceiverImpl(SmsProvider*, const url::Origin&);
-  ~SmsReceiverImpl() override;
-
-  struct Request {
-    explicit Request(ReceiveCallback callback);
-    ~Request();
-
-    base::OneShotTimer timer;
-    ReceiveCallback callback;
-
-    DISALLOW_COPY_AND_ASSIGN(Request);
-  };
-
-  // content::SmsProvider::Observer:
-  bool OnReceive(const url::Origin&, const std::string& message) override;
-
-  // blink::mojom::SmsReceiver:
-  void Receive(base::TimeDelta timeout, ReceiveCallback) override;
-
- private:
-  bool Pop(const std::string& sms);
-  void OnTimeout(Request* request);
-
-  // |sms_provider_| is safe because all instances of SmsReceiverImpl are owned
-  // by SmsServiceImpl through a StrongBindingSet.
-  SmsProvider* sms_provider_;
-  const url::Origin origin_;
-
-  using SmsRequestList = std::list<std::unique_ptr<Request>>;
-  SmsRequestList requests_;
-
-  SEQUENCE_CHECKER(sequence_checker_);
-  DISALLOW_COPY_AND_ASSIGN(SmsReceiverImpl);
-};
-
-}  // namespace content
-
-#endif  // CONTENT_BROWSER_SMS_SMS_RECEIVER_IMPL_H_

content/browser/sms/sms_service.cc

@@ -4,33 +4,114 @@
 
 #include "content/browser/sms/sms_service.h"
 
+#include <iterator>
+#include <queue>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
-#include "content/browser/sms/sms_receiver_impl.h"
+#include "base/optional.h"
+
+using blink::mojom::SmsStatus;
 
 namespace content {
 
-SmsService::SmsService() : sms_provider_(SmsProvider::Create()) {}
+SmsService::SmsService(SmsProvider* provider,
+                       const url::Origin& origin,
+                       RenderFrameHost* host,
+                       blink::mojom::SmsReceiverRequest request)
+    : FrameServiceBase(host, std::move(request)),
+      sms_provider_(provider),
+      origin_(origin) {}
+
+SmsService::SmsService(SmsProvider* provider,
+                       RenderFrameHost* host,
+                       blink::mojom::SmsReceiverRequest request)
+    : SmsService(provider,
+                 host->GetLastCommittedOrigin(),
+                 host,
+                 std::move(request)) {}
 
 SmsService::~SmsService() {
+  while (!requests_.empty()) {
+    Pop(SmsStatus::kTimeout, base::nullopt);
+  }
+}
+
+SmsService::Request::Request(ReceiveCallback callback)
+    : callback(std::move(callback)) {}
+
+SmsService::Request::~Request() = default;
+
+// static
+void SmsService::Create(SmsProvider* provider,
+                        RenderFrameHost* host,
+                        blink::mojom::SmsReceiverRequest request) {
+  DCHECK(host);
+
+  // SmsService owns itself. It will self-destruct when a mojo interface
+  // error occurs, the render frame host is deleted, or the render frame host
+  // navigates to a new document.
+  new SmsService(provider, host, std::move(request));
+}
+
+void SmsService::Receive(base::TimeDelta timeout, ReceiveCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (requests_.empty())
+    sms_provider_->AddObserver(this);
+
+  auto request = std::make_unique<Request>(std::move(callback));
+  // The |timer| is owned by |request|, and |request| is owned by |this|, so it
+  // is safe to hold raw pointers to |this| and |request| here in the callback.
+  request->timer.Start(FROM_HERE, timeout,
+                       base::BindOnce(&SmsService::OnTimeout,
+                                      base::Unretained(this), request.get()));
+  requests_.push_back(std::move(request));
+  sms_provider_->Retrieve();
 }
 
-void SmsService::Bind(blink::mojom::SmsReceiverRequest request,
-                      const url::Origin& origin) {
+bool SmsService::OnReceive(const url::Origin& origin, const std::string& sms) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (origin_ != origin)
+    return false;
+
+  return Pop(SmsStatus::kSuccess, sms);
+}
+
+bool SmsService::Pop(blink::mojom::SmsStatus status,
+                     base::Optional<std::string> sms) {
+  DCHECK(!requests_.empty());
+
+  DCHECK(requests_.front()->timer.IsRunning());
 
-  bindings_.AddBinding(
-      std::make_unique<SmsReceiverImpl>(sms_provider_.get(), origin),
-      std::move(request));
+  requests_.front()->timer.Stop();
+  std::move(requests_.front()->callback).Run(status, sms);
+  requests_.pop_front();
+
+  if (requests_.empty())
+    sms_provider_->RemoveObserver(this);
+
+  return true;
 }
 
-void SmsService::SetSmsProviderForTest(
-    std::unique_ptr<SmsProvider> sms_provider) {
+void SmsService::OnTimeout(Request* request) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  sms_provider_ = std::move(sms_provider);
+
+  DCHECK(!request->timer.IsRunning());
+
+  std::move(request->callback)
+      .Run(blink::mojom::SmsStatus::kTimeout, base::nullopt);
+
+  // Remove the request from the list.
+  for (auto iter = requests_.begin(); iter != requests_.end(); ++iter) {
+    if ((*iter).get() == request) {
+      requests_.erase(iter);
+      if (requests_.empty())
+        sms_provider_->RemoveObserver(this);
+      return;
+    }
+  }
 }
 
 }  // namespace content

content/browser/sms/sms_service.h

@@ -5,45 +5,76 @@
 #ifndef CONTENT_BROWSER_SMS_SMS_SERVICE_H_
 #define CONTENT_BROWSER_SMS_SMS_SERVICE_H_
 
-#include <map>
+#include <list>
 #include <memory>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "content/browser/sms/sms_provider.h"
 #include "content/common/content_export.h"
-#include "mojo/public/cpp/bindings/strong_binding_set.h"
+#include "content/public/browser/frame_service_base.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "third_party/blink/public/mojom/sms/sms_receiver.mojom.h"
-
-namespace url {
-class Origin;
-}
+#include "url/origin.h"
 
 namespace content {
 
-// The SmsService is responsible for binding the incoming mojo connections
-// from the renderer process and their corresponding SmsReceiverImpl.
-// SmsService is owned by BrowserMainLoop, making it effectively a singleton,
-// which allows it to serialize and manage a global queue of incoming SMS
-// messsages.
-class CONTENT_EXPORT SmsService {
+class RenderFrameHost;
+
+// SmsService handles mojo connections from the renderer, observing the incoming
+// SMS messages from an SmsProvider.
+// In practice, it is owned and managed by a RenderFrameHost. It accomplishes
+// that via subclassing FrameServiceBase, which observes the lifecycle of a
+// RenderFrameHost and manages it own memory.
+// Create() creates a self-managed instance of SmsService and binds it to the
+// request.
+class CONTENT_EXPORT SmsService
+    : public FrameServiceBase<blink::mojom::SmsReceiver>,
+      public content::SmsProvider::Observer {
  public:
-  SmsService();
-  ~SmsService();
+  static void Create(SmsProvider*,
+                     RenderFrameHost*,
+                     blink::mojom::SmsReceiverRequest);
+
+  SmsService(SmsProvider*, RenderFrameHost*, blink::mojom::SmsReceiverRequest);
+  SmsService(SmsProvider*,
+             const url::Origin&,
+             RenderFrameHost*,
+             blink::mojom::SmsReceiverRequest);
+  ~SmsService() override;
+
+  struct Request {
+    explicit Request(ReceiveCallback callback);
+    ~Request();
 
-  void Bind(blink::mojom::SmsReceiverRequest, const url::Origin&);
+    base::OneShotTimer timer;
+    ReceiveCallback callback;
 
-  // Testing helpers.
-  void SetSmsProviderForTest(std::unique_ptr<SmsProvider>);
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
+
+  // content::SmsProvider::Observer:
+  bool OnReceive(const url::Origin&, const std::string& message) override;
+
+  // blink::mojom::SmsReceiver:
+  void Receive(base::TimeDelta timeout, ReceiveCallback) override;
 
  private:
-  std::unique_ptr<SmsProvider> sms_provider_;
+  bool Pop(blink::mojom::SmsStatus, base::Optional<std::string>);
+  void OnTimeout(Request* request);
+  // |sms_provider_| is safe because all instances of SmsProvider are owned
+  // by a SmsKeyedService, which is owned by a Profile, which transitively
+  // owns SmsServices.
+  SmsProvider* sms_provider_;
+  const url::Origin origin_;
 
-  // Registered clients.
-  mojo::StrongBindingSet<blink::mojom::SmsReceiver> bindings_;
-  SEQUENCE_CHECKER(sequence_checker_);
+  using SmsRequestList = std::list<std::unique_ptr<Request>>;
+  SmsRequestList requests_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
   DISALLOW_COPY_AND_ASSIGN(SmsService);
 };