Calling PushManager.subscribe() should always hit the GCM Driver

The PushMessagingManager currently maintains its own subscription cache
in the Service Worker database, keeping track of the subscriptions it
thinks are valid. This means that the PushMessagingManager assumes that
the subscription is valid.

There are cases where the underlying push service, in our case Google
Cloud Messaging implemented through our GCM Driver, invalidates a
subscription. We need to find out about that when it happens, so change
the PushMessagingManager to *always* attempt to create a subscription,
upon which we rely on the push service client to return the same
information given the same input.

The GCM Driver has its own cache, so in the vast majority of cases these
calls won't hit the server. They will, however, on occasion.

Bug: 799483
Change-Id: I715c6f2eb8296b4512b6b7e9e31734ce9f254744
Reviewed-on: https://chromium-review.googlesource.com/932401
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539473}

chrome/browser/push_messaging/push_messaging_browsertest.cc

@@ -596,6 +596,37 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
             script_result);
 }
 
+IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeWithInvalidation) {
+  std::string token1, token2, token3;
+
+  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token1));
+  ASSERT_FALSE(token1.empty());
+
+  // Repeated calls to |subscribe()| should yield the same token.
+  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token2));
+  ASSERT_EQ(token1, token2);
+
+  PushMessagingAppIdentifier app_identifier =
+      PushMessagingAppIdentifier::FindByServiceWorker(
+          GetBrowser()->profile(), https_server()->GetURL("/").GetOrigin(),
+          0LL /* service_worker_registration_id */);
+
+  ASSERT_FALSE(app_identifier.is_null());
+  EXPECT_EQ(app_identifier.app_id(), gcm_driver_->last_gettoken_app_id());
+
+  // Delete the InstanceID. This captures two scenarios: either the database was
+  // corrupted, or the subscription was invalidated by the server.
+  ASSERT_NO_FATAL_FAILURE(
+      DeleteInstanceIDAsIfGCMStoreReset(app_identifier.app_id()));
+
+  EXPECT_EQ(app_identifier.app_id(), gcm_driver_->last_deletetoken_app_id());
+
+  // Repeated calls to |subscribe()| will now (silently) result in a new token.
+  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token3));
+  ASSERT_FALSE(token3.empty());
+  EXPECT_NE(token1, token3);
+}
+
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribeWorker) {
   std::string script_result;
 
@@ -990,10 +1021,6 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribePersisted) {
   // Now test that the Service Worker registration IDs and push subscription IDs
   // generated above were persisted to SW storage, by checking that they are
   // unchanged despite requesting them in a different order.
-  // TODO(johnme): Ideally we would restart the browser at this point to check
-  // they were persisted to disk, but that's not currently possible since the
-  // test server uses random port numbers for each test (even PRE_Foo and Foo),
-  // so we wouldn't be able to load the test pages with the same origin.
 
   LoadTestPage("/push_messaging/subscope1/test.html");
   std::string token4;
@@ -1005,13 +1032,13 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, SubscribePersisted) {
   std::string token5;
   ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token5));
   EXPECT_EQ(token2, token5);
-  EXPECT_EQ(sw1_identifier.app_id(), gcm_driver_->last_gettoken_app_id());
+  EXPECT_EQ(sw2_identifier.app_id(), gcm_driver_->last_gettoken_app_id());
 
   LoadTestPage();
   std::string token6;
   ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token6));
   EXPECT_EQ(token1, token6);
-  EXPECT_EQ(sw1_identifier.app_id(), gcm_driver_->last_gettoken_app_id());
+  EXPECT_EQ(sw0_identifier.app_id(), gcm_driver_->last_gettoken_app_id());
 }
 
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, AppHandlerOnlyIfSubscribed) {
@@ -1945,53 +1972,6 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   EXPECT_TRUE(app_identifier3.is_null());
 }
 
-IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest, InvalidSubscribeUnsubscribes) {
-  std::string script_result;
-
-  std::string token1;
-  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token1));
-
-  GURL origin = https_server()->GetURL("/").GetOrigin();
-  PushMessagingAppIdentifier app_identifier1 =
-      PushMessagingAppIdentifier::FindByServiceWorker(
-          GetBrowser()->profile(), origin,
-          0LL /* service_worker_registration_id */);
-  ASSERT_FALSE(app_identifier1.is_null());
-
-  ASSERT_NO_FATAL_FAILURE(
-      DeleteInstanceIDAsIfGCMStoreReset(app_identifier1.app_id()));
-
-  // Push messaging should not yet be aware of the InstanceID being deleted.
-  histogram_tester_.ExpectTotalCount("PushMessaging.UnregistrationReason", 0);
-  // We should still be able to look up the app id.
-  PushMessagingAppIdentifier app_identifier2 =
-      PushMessagingAppIdentifier::FindByServiceWorker(
-          GetBrowser()->profile(), origin,
-          0LL /* service_worker_registration_id */);
-  EXPECT_FALSE(app_identifier2.is_null());
-  EXPECT_EQ(app_identifier1.app_id(), app_identifier2.app_id());
-
-  // Now call PushManager.subscribe() again. It should succeed, but with a
-  // *different* token, indicating that it unsubscribed and re-subscribed.
-  std::string token2;
-  ASSERT_NO_FATAL_FAILURE(SubscribeSuccessfully(true /* use_key */, &token2));
-  EXPECT_NE(token1, token2);
-
-  // This should have unsubscribed the original push subscription.
-  histogram_tester_.ExpectUniqueSample(
-      "PushMessaging.UnregistrationReason",
-      static_cast<int>(
-          content::mojom::PushUnregistrationReason::SUBSCRIBE_STORAGE_CORRUPT),
-      1);
-  // Looking up the app id should return a different id.
-  PushMessagingAppIdentifier app_identifier3 =
-      PushMessagingAppIdentifier::FindByServiceWorker(
-          GetBrowser()->profile(), origin,
-          0LL /* service_worker_registration_id */);
-  EXPECT_FALSE(app_identifier3.is_null());
-  EXPECT_NE(app_identifier2.app_id(), app_identifier3.app_id());
-}
-
 IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
                        GlobalResetPushPermissionUnsubscribes) {
   std::string script_result;

chrome/browser/push_messaging/push_messaging_service_impl.cc

@@ -466,8 +466,15 @@ void PushMessagingServiceImpl::SubscribeFromDocument(
     bool user_gesture,
     const RegisterCallback& callback) {
   PushMessagingAppIdentifier app_identifier =
-      PushMessagingAppIdentifier::Generate(requesting_origin,
-                                           service_worker_registration_id);
+      PushMessagingAppIdentifier::FindByServiceWorker(
+          profile_, requesting_origin, service_worker_registration_id);
+
+  // If there is no existing app identifier for the given Service Worker,
+  // generate a new one. This will create a new subscription on the server.
+  if (app_identifier.is_null()) {
+    app_identifier = PushMessagingAppIdentifier::Generate(
+        requesting_origin, service_worker_registration_id);
+  }
 
   if (push_subscription_count_ + pending_push_subscription_count_ >=
       kMaxRegistrations) {
@@ -507,8 +514,15 @@ void PushMessagingServiceImpl::SubscribeFromWorker(
     const content::PushSubscriptionOptions& options,
     const RegisterCallback& register_callback) {
   PushMessagingAppIdentifier app_identifier =
-      PushMessagingAppIdentifier::Generate(requesting_origin,
-                                           service_worker_registration_id);
+      PushMessagingAppIdentifier::FindByServiceWorker(
+          profile_, requesting_origin, service_worker_registration_id);
+
+  // If there is no existing app identifier for the given Service Worker,
+  // generate a new one. This will create a new subscription on the server.
+  if (app_identifier.is_null()) {
+    app_identifier = PushMessagingAppIdentifier::Generate(
+        requesting_origin, service_worker_registration_id);
+  }
 
   if (push_subscription_count_ + pending_push_subscription_count_ >=
       kMaxRegistrations) {

components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc

@@ -9,6 +9,7 @@
 #include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/gcm_driver/gcm_client.h"
 
@@ -93,8 +94,22 @@ void FakeGCMDriverForInstanceID::DeleteToken(
     const std::string& authorized_entity,
     const std::string& scope,
     const DeleteTokenCallback& callback) {
-  std::string key = app_id + authorized_entity + scope;
-  tokens_.erase(key);
+  std::string key_prefix = app_id;
+
+  // Calls to InstanceID::DeleteID() will end up deleting the token for a given
+  // |app_id| with both |authorized_entity| and |scope| set to "*", meaning that
+  // all data has to be deleted. Do a prefix search to emulate this behaviour.
+  if (authorized_entity != "*")
+    key_prefix += authorized_entity;
+  if (scope != "*")
+    key_prefix += scope;
+
+  for (auto iter = tokens_.begin(); iter != tokens_.end();) {
+    if (base::StartsWith(iter->first, key_prefix, base::CompareCase::SENSITIVE))
+      iter = tokens_.erase(iter);
+    else
+      iter++;
+  }
 
   last_deletetoken_app_id_ = app_id;
 

components/gcm_driver/registration_info.cc

@@ -119,7 +119,7 @@ bool GCMRegistrationInfo::Deserialize(const std::string& serialized_key,
     return false;
   // Note that it's valid for pos_hash to be std::string::npos.
   size_t pos_hash = serialized_value.find(kSerializedValidationTimeSeparator);
-  bool has_timestamp = (pos_hash != std::string::npos);
+  bool has_timestamp = pos_hash != std::string::npos;
 
   std::string senders = serialized_value.substr(0, pos_equals);
   std::string registration_id_str, last_validated_str;