SMS API: Handle SMS timeout in the SmsReceiverImpl

Currently SMS API listens to timeout events from the JNI android
SmsRetriever API. This CL updated SmsReceiverImpl to handle timeout
using a OneShotTimer per request so that the API uses the timeout
value that the user sets up.

Bug: 969130
Change-Id: Ie99c2e1650a5689a2dbd65c0708324b71df97639
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1682329
Commit-Queue: Jun Cai <juncai@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676557}

content/browser/sms/sms_provider.cc

@@ -54,10 +54,6 @@ void SmsProvider::NotifyReceive(const url::Origin& origin,
   }
 }
 
-void SmsProvider::NotifyTimeout() {
-  observers_.begin()->OnTimeout();
-}
-
 bool SmsProvider::HasObservers() {
   return observers_.might_have_observers();
 }

content/browser/sms/sms_provider.h

@@ -28,7 +28,6 @@ class CONTENT_EXPORT SmsProvider {
     // Receive an |sms| from an origin. Return true if the message is
     // handled, which stops its propagation to other observers.
     virtual bool OnReceive(const url::Origin&, const std::string& sms) = 0;
-    virtual void OnTimeout() = 0;
   };
 
   SmsProvider();
@@ -44,7 +43,6 @@ class CONTENT_EXPORT SmsProvider {
   void RemoveObserver(const Observer*);
   void NotifyReceive(const url::Origin&, const std::string& sms);
   void NotifyReceive(const std::string& sms);
-  void NotifyTimeout();
   bool HasObservers();
 
  private:

content/browser/sms/sms_provider_android.cc

@@ -48,7 +48,6 @@ void SmsProviderAndroid::OnReceive(
 void SmsProviderAndroid::OnTimeout(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj) {
-  NotifyTimeout();
 }
 
 }  // namespace content

content/browser/sms/sms_receiver_impl.cc

@@ -21,48 +21,70 @@ 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_);
-  Push(std::move(callback));
-}
 
-void SmsReceiverImpl::Push(ReceiveCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (callbacks_.empty())
+  if (requests_.empty())
     sms_provider_->AddObserver(this);
 
-  callbacks_.push(std::move(callback));
+  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();
 }
 
-blink::mojom::SmsReceiver::ReceiveCallback SmsReceiverImpl::Pop() {
-  DCHECK(!callbacks_.empty()) << "Unexpected SMS received";
-
-  ReceiveCallback callback = std::move(callbacks_.front());
-  callbacks_.pop();
-
-  if (callbacks_.empty())
-    sms_provider_->RemoveObserver(this);
-
-  return callback;
-}
-
 bool SmsReceiverImpl::OnReceive(const url::Origin& origin,
                                 const std::string& sms) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (origin_ != origin)
     return false;
 
-  Pop().Run(blink::mojom::SmsStatus::kSuccess, sms);
+  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() {
+void SmsReceiverImpl::OnTimeout(Request* request) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  Pop().Run(blink::mojom::SmsStatus::kTimeout, base::nullopt);
+  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

@@ -5,13 +5,14 @@
 #ifndef CONTENT_BROWSER_SMS_SMS_RECEIVER_IMPL_H_
 #define CONTENT_BROWSER_SMS_SMS_RECEIVER_IMPL_H_
 
+#include <list>
 #include <memory>
-#include <queue>
 
 #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"
@@ -25,23 +26,33 @@ class CONTENT_EXPORT SmsReceiverImpl : public blink::mojom::SmsReceiver,
   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;
-  void OnTimeout() override;
 
   // blink::mojom::SmsReceiver:
   void Receive(base::TimeDelta timeout, ReceiveCallback) override;
 
  private:
-  // Manages the queue of callbacks.
-  void Push(ReceiveCallback);
-  ReceiveCallback Pop();
+  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_;
-  std::queue<ReceiveCallback> callbacks_;
+
+  using SmsRequestList = std::list<std::unique_ptr<Request>>;
+  SmsRequestList requests_;
 
   SEQUENCE_CHECKER(sequence_checker_);
   DISALLOW_COPY_AND_ASSIGN(SmsReceiverImpl);

content/browser/sms/sms_service_impl_unittest.cc

@@ -299,12 +299,8 @@ TEST_F(SmsServiceImplTest, Timeout) {
 
   base::RunLoop loop;
 
-  EXPECT_CALL(*mock, Retrieve()).WillOnce(Invoke([&mock]() {
-    mock->NotifyTimeout();
-  }));
-
   service_ptr->Receive(
-      base::TimeDelta::FromSeconds(10),
+      base::TimeDelta::FromSeconds(0),
       base::BindLambdaForTesting([&](blink::mojom::SmsStatus status,
                                      const base::Optional<std::string>& sms) {
         EXPECT_EQ(blink::mojom::SmsStatus::kTimeout, status);
@@ -340,7 +336,7 @@ TEST_F(SmsServiceImplTest, TimeoutTwoOrigins) {
       .WillOnce(Invoke([&listen]() { listen.Quit(); }));
 
   service_ptr1->Receive(
-      base::TimeDelta::FromSeconds(10),
+      base::TimeDelta::FromSeconds(0),
       base::BindLambdaForTesting([&](blink::mojom::SmsStatus status,
                                      const base::Optional<std::string>& sms) {
         sms_status1 = status;
@@ -359,9 +355,8 @@ TEST_F(SmsServiceImplTest, TimeoutTwoOrigins) {
 
   listen.Run();
 
-  // Timesout the first request.
-
-  mock->NotifyTimeout();
+  // The first request immediately times out because it uses TimeDelta of 0
+  // seconds.
 
   sms_loop1.Run();
 
@@ -377,4 +372,66 @@ TEST_F(SmsServiceImplTest, TimeoutTwoOrigins) {
   EXPECT_EQ(blink::mojom::SmsStatus::kSuccess, sms_status2);
 }
 
+TEST_F(SmsServiceImplTest, SecondRequestTimesOutEarlierThanFirstRequest) {
+  auto impl = std::make_unique<SmsServiceImpl>();
+  auto* mock = new NiceMock<MockSmsProvider>();
+
+  impl->SetSmsProviderForTest(base::WrapUnique(mock));
+
+  blink::mojom::SmsReceiverPtr service_ptr1;
+  GURL url1("http://a.com");
+  impl->Bind(mojo::MakeRequest(&service_ptr1), url::Origin::Create(url1));
+
+  blink::mojom::SmsReceiverPtr service_ptr2;
+  GURL url2("http://b.com");
+  impl->Bind(mojo::MakeRequest(&service_ptr2), url::Origin::Create(url2));
+
+  blink::mojom::SmsStatus sms_status1;
+  base::Optional<std::string> response1;
+  blink::mojom::SmsStatus sms_status2;
+  base::Optional<std::string> response2;
+
+  base::RunLoop listen, sms_loop1, sms_loop2;
+
+  EXPECT_CALL(*mock, Retrieve())
+      .WillOnce(testing::Return())
+      .WillOnce(Invoke([&listen]() { listen.Quit(); }));
+
+  service_ptr1->Receive(
+      base::TimeDelta::FromSeconds(10),
+      base::BindLambdaForTesting([&](blink::mojom::SmsStatus status,
+                                     const base::Optional<std::string>& sms) {
+        sms_status1 = status;
+        response1 = sms;
+        sms_loop1.Quit();
+      }));
+
+  service_ptr2->Receive(
+      base::TimeDelta::FromSeconds(0),
+      base::BindLambdaForTesting([&](blink::mojom::SmsStatus status,
+                                     const base::Optional<std::string>& sms) {
+        sms_status2 = status;
+        response2 = sms;
+        sms_loop2.Quit();
+      }));
+
+  listen.Run();
+
+  // The second request immediately times out because it uses TimeDelta of 0
+  // seconds.
+
+  sms_loop2.Run();
+
+  EXPECT_EQ(blink::mojom::SmsStatus::kTimeout, sms_status2);
+
+  // Delivers the first SMS.
+
+  mock->NotifyReceive(url::Origin::Create(url1), "first");
+
+  sms_loop1.Run();
+
+  EXPECT_EQ("first", response1.value());
+  EXPECT_EQ(blink::mojom::SmsStatus::kSuccess, sms_status1);
+}
+
 }  // namespace content