[sms] Allow multiple one-time-code forms on a single page

This changes allows a page to have multiple input forms with
autocomplete='one-time-code'. If subscribe has already been called for
an SmsClient, it will silently ignore. Without this change, the page
will crash upon trying to Subscribe again because each SmsClient is only
allowed to be subscribed once in the SmsQueue[1]. This crash is also
reproducible by reloading the page with the autocomplete input.

[1] https://cs.chromium.org/chromium/src/content/browser/sms/sms_queue.cc?sq=package:chromium&g=0&l=16

Bug: 1037914
Change-Id: I130aeda2b8c9df2328393ec6b42a7cae608ef07f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1981266Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729999}

chrome/browser/ui/autofill/sms_client_impl.cc

@@ -19,7 +19,6 @@ SmsClientImpl::~SmsClientImpl() {
 }
 
 void SmsClientImpl::Subscribe() {
-  // TODO(goto): Deal with corner cases like multiple forms in the DOM.
   fetcher_->Subscribe(origin_, this);
 }
 

content/browser/sms/sms_fetcher_impl.cc

@@ -10,7 +10,6 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
-#include "url/origin.h"
 
 namespace content {
 
@@ -43,6 +42,10 @@ SmsFetcher* SmsFetcher::Get(BrowserContext* context) {
 void SmsFetcherImpl::Subscribe(const url::Origin& origin,
                                SmsQueue::Subscriber* subscriber) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (subscribers_.HasSubscriber(origin, subscriber))
+    return;
+
   subscribers_.Push(origin, subscriber);
 
   // Fetches a remote SMS.

content/browser/sms/sms_fetcher_impl.h

@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_SMS_SMS_FETCHER_IMPL_H_
 
 #include <memory>
+#include <string>
 
 #include "base/optional.h"
 #include "base/sequence_checker.h"
@@ -14,10 +15,7 @@
 #include "content/browser/sms/sms_queue.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/sms_fetcher.h"
-
-namespace url {
-class Origin;
-}
+#include "url/origin.h"
 
 namespace content {
 

content/browser/sms/sms_fetcher_impl_unittest.cc

@@ -200,4 +200,18 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
   provider()->NotifyReceive(origin1, "123", "foo");
 }
 
+TEST_F(SmsFetcherImplTest, SubscribeIsIdempotent) {
+  StrictMock<MockSubscriber> subscriber;
+
+  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
+  fetcher.Subscribe(kTestOrigin, &subscriber);
+  fetcher.Subscribe(kTestOrigin, &subscriber);
+
+  EXPECT_TRUE(fetcher.HasSubscribers());
+
+  fetcher.Unsubscribe(kTestOrigin, &subscriber);
+
+  EXPECT_FALSE(fetcher.HasSubscribers());
+}
+
 }  // namespace content

content/browser/sms/sms_queue.cc

@@ -42,4 +42,10 @@ bool SmsQueue::HasSubscribers() {
   return !subscribers_.empty();
 }
 
+bool SmsQueue::HasSubscriber(const url::Origin& origin,
+                             const Subscriber* subscriber) {
+  return (subscribers_.find(origin) != subscribers_.end()) &&
+         subscribers_[origin].HasObserver(subscriber);
+}
+
 }  // namespace content

content/browser/sms/sms_queue.h

@@ -26,6 +26,7 @@ class CONTENT_EXPORT SmsQueue {
   Subscriber* Pop(const url::Origin& origin);
   void Remove(const url::Origin& origin, Subscriber* subscriber);
   bool HasSubscribers();
+  bool HasSubscriber(const url::Origin& origin, const Subscriber* subscriber);
 
  private:
   std::map<url::Origin, base::ObserverList<Subscriber>> subscribers_;

content/public/browser/sms_fetcher.h

@@ -34,6 +34,7 @@ class SmsFetcher {
                            const std::string& sms) = 0;
   };
 
+  // Idempotent function that subscribes to incoming SMSes from SmsProvider.
   virtual void Subscribe(const url::Origin& origin, Subscriber* subscriber) = 0;
   virtual void Unsubscribe(const url::Origin& origin,
                            Subscriber* subscriber) = 0;