[sms] Refactor to not use BrowserMainLoop to retrieve SmsProvider

This changes the ownership of SmsProvider from BrowserMainLoop to
SmsFetcher. This infrastructure change that will help with
implementation of the autofill version of sms receiver.

Bug: 1026373
Change-Id: I0f50f90a3d4b7a72443b0a6639d7d12d25ffaeb0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1960068Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#724063}

content/browser/browser_main_loop.cc

@@ -87,7 +87,6 @@
 #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_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"
@@ -1613,16 +1612,4 @@ bool BrowserMainLoop::AudioServiceOutOfProcess() const {
          !GetContentClient()->browser()->OverridesAudioManager();
 }
 
-SmsProvider* BrowserMainLoop::GetSmsProvider() {
-  if (!sms_provider_) {
-    sms_provider_ = SmsProvider::Create();
-  }
-  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

@@ -89,7 +89,6 @@ class MediaKeysListenerManagerImpl;
 class MediaStreamManager;
 class SaveFileManager;
 class ScreenlockMonitor;
-class SmsProvider;
 class SpeechRecognitionManagerImpl;
 class StartupTaskRunner;
 class TracingControllerImpl;
@@ -225,9 +224,6 @@ class CONTENT_EXPORT BrowserMainLoop {
   }
 #endif
 
-  SmsProvider* GetSmsProvider();
-  void SetSmsProviderForTesting(std::unique_ptr<SmsProvider>);
-
   BrowserMainParts* parts() { return parts_.get(); }
 
  private:
@@ -375,8 +371,6 @@ class CONTENT_EXPORT BrowserMainLoop {
   // Must be deleted on the IO thread.
   std::unique_ptr<SpeechRecognitionManagerImpl> speech_recognition_manager_;
 
-  std::unique_ptr<SmsProvider> sms_provider_;
-
 #if defined(OS_WIN)
   std::unique_ptr<media::SystemMessageWindowWin> system_message_window_;
 #elif defined(OS_LINUX) && defined(USE_UDEV)

content/browser/sms/sms_fetcher_impl.cc

@@ -16,8 +16,9 @@ namespace content {
 
 const char kSmsFetcherImplKeyName[] = "sms_fetcher";
 
-SmsFetcherImpl::SmsFetcherImpl(BrowserContext* context, SmsProvider* provider)
-    : context_(context), provider_(provider) {
+SmsFetcherImpl::SmsFetcherImpl(BrowserContext* context,
+                               std::unique_ptr<SmsProvider> provider)
+    : context_(context), provider_(std::move(provider)) {
   if (provider_)
     provider_->AddObserver(this);
 }
@@ -30,8 +31,8 @@ SmsFetcherImpl::~SmsFetcherImpl() {
 // static
 SmsFetcher* SmsFetcher::Get(BrowserContext* context) {
   if (!context->GetUserData(kSmsFetcherImplKeyName)) {
-    auto fetcher = std::make_unique<SmsFetcherImpl>(
-        context, BrowserMainLoop::GetInstance()->GetSmsProvider());
+    auto fetcher =
+        std::make_unique<SmsFetcherImpl>(context, SmsProvider::Create());
     context->SetUserData(kSmsFetcherImplKeyName, std::move(fetcher));
   }
 
@@ -41,6 +42,7 @@ SmsFetcher* SmsFetcher::Get(BrowserContext* context) {
 
 void SmsFetcherImpl::Subscribe(const url::Origin& origin,
                                SmsQueue::Subscriber* subscriber) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   subscribers_.Push(origin, subscriber);
 
   // Fetches a remote SMS.
@@ -56,12 +58,14 @@ void SmsFetcherImpl::Subscribe(const url::Origin& origin,
 
 void SmsFetcherImpl::Unsubscribe(const url::Origin& origin,
                                  SmsQueue::Subscriber* subscriber) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   subscribers_.Remove(origin, subscriber);
 }
 
 bool SmsFetcherImpl::Notify(const url::Origin& origin,
                             const std::string& one_time_code,
                             const std::string& sms) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto* subscriber = subscribers_.Pop(origin);
 
   if (!subscriber)
@@ -73,6 +77,8 @@ bool SmsFetcherImpl::Notify(const url::Origin& origin,
 }
 
 void SmsFetcherImpl::OnRemote(base::Optional<std::string> sms) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!sms)
     return;
 
@@ -86,11 +92,20 @@ void SmsFetcherImpl::OnRemote(base::Optional<std::string> sms) {
 bool SmsFetcherImpl::OnReceive(const url::Origin& origin,
                                const std::string& one_time_code,
                                const std::string& sms) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return Notify(origin, one_time_code, sms);
 }
 
 bool SmsFetcherImpl::HasSubscribers() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return subscribers_.HasSubscribers();
 }
 
+void SmsFetcherImpl::SetSmsProviderForTesting(
+    std::unique_ptr<SmsProvider> provider) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  provider_ = std::move(provider);
+  provider_->AddObserver(this);
+}
+
 }  // namespace content

content/browser/sms/sms_fetcher_impl.h

@@ -5,7 +5,10 @@
 #ifndef CONTENT_BROWSER_SMS_SMS_FETCHER_IMPL_H_
 #define CONTENT_BROWSER_SMS_SMS_FETCHER_IMPL_H_
 
+#include <memory>
+
 #include "base/optional.h"
+#include "base/sequence_checker.h"
 #include "base/supports_user_data.h"
 #include "content/browser/sms/sms_provider.h"
 #include "content/browser/sms/sms_queue.h"
@@ -22,26 +25,30 @@ class BrowserContext;
 class SmsProvider;
 
 // SmsFetcherImpls coordinate between local and remote SMS providers as well as
-// between multiple origins. There is one SmsFetcherImpl per profile.
+// between multiple origins. There is one SmsFetcherImpl per profile. An
+// instance must only be used on the sequence it was created on.
 class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher,
                                       public base::SupportsUserData::Data,
                                       public SmsProvider::Observer {
  public:
-  SmsFetcherImpl(BrowserContext*, SmsProvider*);
+  SmsFetcherImpl(BrowserContext* context,
+                 std::unique_ptr<SmsProvider> provider);
   ~SmsFetcherImpl() override;
 
-  static SmsFetcher* Get(BrowserContext*);
+  static SmsFetcher* Get(BrowserContext* context);
 
   void Subscribe(const url::Origin& origin, Subscriber* subscriber) override;
   void Unsubscribe(const url::Origin& origin, Subscriber* subscriber) override;
 
   // content::SmsProvider::Observer:
-  bool OnReceive(const url::Origin&,
+  bool OnReceive(const url::Origin& origin,
                  const std::string& one_time_code,
                  const std::string& sms) override;
 
   bool HasSubscribers() override;
 
+  void SetSmsProviderForTesting(std::unique_ptr<SmsProvider> provider);
+
  private:
   void OnRemote(base::Optional<std::string> sms);
 
@@ -53,12 +60,12 @@ class CONTENT_EXPORT SmsFetcherImpl : public content::SmsFetcher,
   // the BrowserContext itself.
   BrowserContext* context_;
 
-  // |provider_| is safe because all instances of SmsProvider are owned
-  // by the BrowserMainLoop, which outlive instances of this class.
-  SmsProvider* provider_;
+  std::unique_ptr<SmsProvider> provider_;
 
   SmsQueue subscribers_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   base::WeakPtrFactory<SmsFetcherImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(SmsFetcherImpl);

content/browser/sms/sms_fetcher_impl_unittest.cc

@@ -15,7 +15,6 @@
 using ::testing::_;
 using ::testing::Invoke;
 using ::testing::NiceMock;
-using ::testing::Return;
 using ::testing::StrictMock;
 
 namespace content {
@@ -57,6 +56,7 @@ class SmsFetcherImplTest : public testing::Test {
 
   void SetUp() override {
     original_client_ = SetBrowserClientForTesting(&client_);
+    provider_ = new NiceMock<MockSmsProvider>();
   }
 
   void TearDown() override {
@@ -66,12 +66,12 @@ class SmsFetcherImplTest : public testing::Test {
 
  protected:
   MockContentBrowserClient* client() { return &client_; }
-  MockSmsProvider* provider() { return &provider_; }
+  MockSmsProvider* provider() { return provider_; }
 
  private:
   ContentBrowserClient* original_client_ = nullptr;
   NiceMock<MockContentBrowserClient> client_;
-  NiceMock<MockSmsProvider> provider_;
+  NiceMock<MockSmsProvider>* provider_;
 
   DISALLOW_COPY_AND_ASSIGN(SmsFetcherImplTest);
 };
@@ -80,7 +80,7 @@ class SmsFetcherImplTest : public testing::Test {
 
 TEST_F(SmsFetcherImplTest, ReceiveFromLocalSmsProvider) {
   StrictMock<MockSubscriber> subscriber;
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   EXPECT_CALL(*provider(), Retrieve()).WillOnce(Invoke([&]() {
     provider()->NotifyReceive(kTestOrigin, "123", "hello");
@@ -93,7 +93,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromLocalSmsProvider) {
 
 TEST_F(SmsFetcherImplTest, ReceiveFromRemoteProvider) {
   StrictMock<MockSubscriber> subscriber;
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   const std::string& sms = "For: https://a.com?otp=123";
 
@@ -111,7 +111,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromRemoteProvider) {
 
 TEST_F(SmsFetcherImplTest, RemoteProviderTimesOut) {
   StrictMock<MockSubscriber> subscriber;
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   EXPECT_CALL(*client(), FetchRemoteSms(_, _, _))
       .WillOnce(Invoke(
@@ -129,7 +129,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromOtherOrigin) {
   StrictMock<MockSubscriber> subscriber;
   const url::Origin origin = url::Origin::Create(GURL("https://a.com"));
 
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   EXPECT_CALL(*client(), FetchRemoteSms(_, _, _))
       .WillOnce(Invoke(
@@ -145,7 +145,7 @@ TEST_F(SmsFetcherImplTest, ReceiveFromOtherOrigin) {
 
 TEST_F(SmsFetcherImplTest, ReceiveFromBothProviders) {
   StrictMock<MockSubscriber> subscriber;
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   const std::string& sms = "hello \nFor: https://a.com?otp=123";
 
@@ -170,7 +170,7 @@ TEST_F(SmsFetcherImplTest, OneOriginTwoSubscribers) {
   StrictMock<MockSubscriber> subscriber1;
   StrictMock<MockSubscriber> subscriber2;
 
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
 
   fetcher.Subscribe(kTestOrigin, &subscriber1);
   fetcher.Subscribe(kTestOrigin, &subscriber2);
@@ -189,7 +189,7 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
   const url::Origin origin1 = url::Origin::Create(GURL("https://a.com"));
   const url::Origin origin2 = url::Origin::Create(GURL("https://b.com"));
 
-  SmsFetcherImpl fetcher(nullptr, provider());
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
   fetcher.Subscribe(origin1, &subscriber1);
   fetcher.Subscribe(origin2, &subscriber2);
 
@@ -200,4 +200,4 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
   provider()->NotifyReceive(origin1, "123", "foo");
 }
 
-}  // namespace content
\ No newline at end of file
+}  // namespace content

content/browser/sms/sms_service_unittest.cc

@@ -42,11 +42,9 @@ using blink::mojom::SmsReceiver;
 using blink::mojom::SmsStatus;
 using std::string;
 using ::testing::_;
-using ::testing::ByMove;
 using ::testing::Invoke;
 using ::testing::NiceMock;
 using ::testing::Return;
-using ::testing::StrictMock;
 using url::Origin;
 
 namespace content {
@@ -65,13 +63,17 @@ const char kTestUrl[] = "https://www.google.com";
 // control the testing environment.
 class Service {
  public:
-  Service(WebContents* web_contents, const Origin& origin)
-      : fetcher_(web_contents->GetBrowserContext(), &provider_) {
+  Service(WebContents* web_contents, const Origin& origin) {
+    auto provider = std::make_unique<NiceMock<MockSmsProvider>>();
+    provider_ = provider.get();
+    fetcher_ = static_cast<SmsFetcherImpl*>(
+        SmsFetcher::Get(web_contents->GetBrowserContext()));
+    fetcher_->SetSmsProviderForTesting(std::move(provider));
     WebContentsImpl* web_contents_impl =
         reinterpret_cast<WebContentsImpl*>(web_contents);
     web_contents_impl->SetDelegate(&delegate_);
     service_ = std::make_unique<SmsService>(
-        &fetcher_, origin, web_contents->GetMainFrame(),
+        fetcher_, origin, web_contents->GetMainFrame(),
         service_remote_.BindNewPipeAndPassReceiver());
   }
 
@@ -79,12 +81,11 @@ class Service {
       : Service(web_contents,
                 web_contents->GetMainFrame()->GetLastCommittedOrigin()) {}
 
-  NiceMock<MockSmsWebContentsDelegate>* delegate() { return &delegate_; }
-  NiceMock<MockSmsProvider>* provider() { return &provider_; }
-  SmsFetcher* fetcher() { return &fetcher_; }
+  NiceMock<MockSmsProvider>* provider() { return provider_; }
+  SmsFetcherImpl* fetcher() { return fetcher_; }
 
   void CreateSmsPrompt(RenderFrameHost* rfh) {
-    EXPECT_CALL(*delegate(), CreateSmsPrompt(rfh, _, _, _, _))
+    EXPECT_CALL(delegate_, CreateSmsPrompt(rfh, _, _, _, _))
         .WillOnce(Invoke([=](RenderFrameHost*, const Origin& origin,
                              const std::string&, base::OnceClosure on_confirm,
                              base::OnceClosure on_cancel) {
@@ -118,13 +119,13 @@ class Service {
   void AbortRequest() { service_remote_->Abort(); }
 
   void NotifyReceive(const GURL& url, const string& message) {
-    provider_.NotifyReceive(Origin::Create(url), "", message);
+    provider_->NotifyReceive(Origin::Create(url), "", message);
   }
 
  private:
   NiceMock<MockSmsWebContentsDelegate> delegate_;
-  NiceMock<MockSmsProvider> provider_;
-  SmsFetcherImpl fetcher_;
+  NiceMock<MockSmsProvider>* provider_;
+  SmsFetcherImpl* fetcher_ = nullptr;
   mojo::Remote<blink::mojom::SmsReceiver> service_remote_;
   std::unique_ptr<SmsService> service_;
   base::OnceClosure confirm_callback_;
@@ -404,15 +405,17 @@ TEST_F(SmsServiceTest, CleansUp) {
       reinterpret_cast<WebContentsImpl*>(web_contents());
   web_contents_impl->SetDelegate(&delegate);
 
-  NiceMock<MockSmsProvider> provider;
-  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(), &provider);
+  auto provider = std::make_unique<MockSmsProvider>();
+  MockSmsProvider* mock_provider_ptr = provider.get();
+  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(),
+                         std::move(provider));
   mojo::Remote<blink::mojom::SmsReceiver> service;
   SmsService::Create(&fetcher, main_rfh(),
                      service.BindNewPipeAndPassReceiver());
 
   base::RunLoop navigate;
 
-  EXPECT_CALL(provider, Retrieve()).WillOnce(Invoke([&navigate]() {
+  EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&navigate]() {
     navigate.Quit();
   }));
 
@@ -660,15 +663,17 @@ TEST_F(SmsServiceTest, RecordMetricsForNewPage) {
       reinterpret_cast<WebContentsImpl*>(web_contents());
   web_contents_impl->SetDelegate(&delegate);
 
-  NiceMock<MockSmsProvider> provider;
-  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(), &provider);
+  auto provider = std::make_unique<MockSmsProvider>();
+  MockSmsProvider* mock_provider_ptr = provider.get();
+  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(),
+                         std::move(provider));
   mojo::Remote<blink::mojom::SmsReceiver> service;
   SmsService::Create(&fetcher, main_rfh(),
                      service.BindNewPipeAndPassReceiver());
 
   base::RunLoop navigate;
 
-  EXPECT_CALL(provider, Retrieve()).WillOnce(Invoke([&navigate]() {
+  EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&navigate]() {
     navigate.Quit();
   }));
 
@@ -698,15 +703,17 @@ TEST_F(SmsServiceTest, RecordMetricsForSamePage) {
       reinterpret_cast<WebContentsImpl*>(web_contents());
   web_contents_impl->SetDelegate(&delegate);
 
-  NiceMock<MockSmsProvider> provider;
-  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(), &provider);
+  auto provider = std::make_unique<MockSmsProvider>();
+  MockSmsProvider* mock_provider_ptr = provider.get();
+  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(),
+                         std::move(provider));
   mojo::Remote<blink::mojom::SmsReceiver> service;
   SmsService::Create(&fetcher, main_rfh(),
                      service.BindNewPipeAndPassReceiver());
 
   base::RunLoop navigate;
 
-  EXPECT_CALL(provider, Retrieve()).WillOnce(Invoke([&navigate]() {
+  EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&navigate]() {
     navigate.Quit();
   }));
 
@@ -741,15 +748,17 @@ TEST_F(SmsServiceTest, RecordMetricsForExistingPage) {
       reinterpret_cast<WebContentsImpl*>(web_contents());
   web_contents_impl->SetDelegate(&delegate);
 
-  NiceMock<MockSmsProvider> provider;
-  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(), &provider);
+  auto provider = std::make_unique<MockSmsProvider>();
+  MockSmsProvider* mock_provider_ptr = provider.get();
+  SmsFetcherImpl fetcher(web_contents()->GetBrowserContext(),
+                         std::move(provider));
   mojo::Remote<blink::mojom::SmsReceiver> service;
   SmsService::Create(&fetcher, main_rfh(),
                      service.BindNewPipeAndPassReceiver());
 
   base::RunLoop navigate;
 
-  EXPECT_CALL(provider, Retrieve()).WillOnce(Invoke([&navigate]() {
+  EXPECT_CALL(*mock_provider_ptr, Retrieve()).WillOnce(Invoke([&navigate]() {
     navigate.Quit();
   }));
 

content/public/browser/sms_fetcher.h

@@ -5,6 +5,8 @@
 #ifndef CONTENT_PUBLIC_BROWSER_SMS_FETCHER_H_
 #define CONTENT_PUBLIC_BROWSER_SMS_FETCHER_H_
 
+#include <string>
+
 #include "base/observer_list_types.h"
 #include "content/common/content_export.h"