Handle push resubscribes with no sender info (avoids DCHECK)

content/browser/push_messaging/push_messaging_message_filter.cc

Index: content/browser/push_messaging/push_messaging_message_filter.cc
diff --git a/content/browser/push_messaging/push_messaging_message_filter.cc b/content/browser/push_messaging/push_messaging_message_filter.cc
index fd768eb7ff6aca39eade34157eab148b28d50fd8..47d1ce1cec146fb959b01e69cd6ae7f469c7a1b8 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -284,31 +284,54 @@ void PushMessagingMessageFilter::OnSubscribe(
 
   service_worker_context_->GetRegistrationUserData(
       data.service_worker_registration_id,
-      {kPushRegistrationIdServiceWorkerKey},
+      {kPushRegistrationIdServiceWorkerKey, kPushSenderIdServiceWorkerKey},
       base::Bind(&PushMessagingMessageFilter::DidCheckForExistingRegistration,
                  weak_factory_io_to_io_.GetWeakPtr(), data));
 }
 
 void PushMessagingMessageFilter::DidCheckForExistingRegistration(
     const RegisterData& data,
-    const std::vector<std::string>& push_registration_id,
+    const std::vector<std::string>& push_registration_id_and_sender_id,

[harkness, 2016/11/02 18:48:12]

As we discussed in person, I think the code would be a lot clearer if you only
do the no registration check here, and move all the sender id checking into
DidGetSenderIdFromStorage. 

[awdf, 2016/11/03 14:10:11]

Ok yeah I think you're right and it would look simpler, it just means I'll have
to pass an extra bool to DidGetSenderIdFromStorage
-shouldRegisterNewSubscription?- so it knows whether to register a new
subscription or just check encryption info + return success as it's doing here
when one already exists.

[awdf, 2016/11/03 14:21:21]

And also push_registration_id needs to be passed through for this case but not
the other. Hm I could pass it as a nullable string and check for its existence
instead of having a separate bool to say whether to register, what do you think?

[harkness, 2016/11/03 15:09:27]

Yeah, I like the nullable string. It's already a string coming from the
GetRegistrationUserData call.

     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status == SERVICE_WORKER_OK) {
-    // TODO(johnme): Check that stored sender ID equals data.options.sender_info
-    // and throw an exception if they don't match.
-    DCHECK_EQ(1u, push_registration_id.size());
-    auto callback = base::Bind(
-        &PushMessagingMessageFilter::DidGetEncryptionKeys,
-        weak_factory_io_to_io_.GetWeakPtr(), data, push_registration_id[0]);
-
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&Core::GetEncryptionInfoOnUI,
-                   base::Unretained(ui_core_.get()), data.requesting_origin,
-                   data.service_worker_registration_id,
-                   data.options.sender_info, callback));
-    return;
+    DCHECK_EQ(2u, push_registration_id_and_sender_id.size());
+    const auto& push_registration_id = push_registration_id_and_sender_id[0];
+    const auto& stored_sender_id = push_registration_id_and_sender_id[1];
+    if (!data.options.sender_info.empty()) {
+      // TODO(crbug.com/638924): Check that stored sender ID equals
+      // data.options.sender_info and throw an exception if they don't match.
+      auto callback = base::Bind(

[harkness, 2016/11/02 18:48:12]

Unless I'm overlooking something, you can refactor this to make it much simpler
if you move the callback definition and the PostTask to after the if/else and
just have the if set a local to data.options.sender_info or stored_sender_id.

const auto& sender_id = push_registration_id_and_sender_id;
if (...) {
  sender_id = data.options.sender_info;
} else {
  // check things
}

callback = ...
BrowserThread::PostTask(... sender_id ...);
return;
  

[awdf, 2016/11/03 14:10:11]

Done. Agree it's much nicer not to repeat the identical callback and
near-identical postTask.

+          &PushMessagingMessageFilter::DidGetEncryptionKeys,
+          weak_factory_io_to_io_.GetWeakPtr(), data, push_registration_id);
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&Core::GetEncryptionInfoOnUI,
+                     base::Unretained(ui_core_.get()), data.requesting_origin,
+                     data.service_worker_registration_id,
+                     data.options.sender_info, callback));
+      return;
+    } else {
+      // Only allow subscribes with no sender info if the stored id is numeric,
+      // in order to support the legacy way of subscribing from a service
+      // worker (first subscribe from the document using a gcm_sender_id set
+      // in the manifest, then subscribe from the service worker with no key).
+      if (base::ContainsOnlyChars(stored_sender_id, "0123456789")) {
+        auto callback = base::Bind(
+            &PushMessagingMessageFilter::DidGetEncryptionKeys,
+            weak_factory_io_to_io_.GetWeakPtr(), data, push_registration_id);
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(&Core::GetEncryptionInfoOnUI,
+                       base::Unretained(ui_core_.get()), data.requesting_origin,
+                       data.service_worker_registration_id, stored_sender_id,
+                       callback));
+        return;
+      } else {
+        SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
+        return;
+      }
+    }
   }
   // TODO(johnme): The spec allows the register algorithm to reject with an
   // AbortError when accessing storage fails. Perhaps we should do that if
@@ -320,6 +343,8 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
                             base::Bind(&Core::RegisterOnUI,
                                        base::Unretained(ui_core_.get()), data));
   } else {
+    // There is no existing registration and the sender_info passed in was
+    // empty, but perhaps there is a stored sender id we can use.
     service_worker_context_->GetRegistrationUserData(
         data.service_worker_registration_id, {kPushSenderIdServiceWorkerKey},
         base::Bind(&PushMessagingMessageFilter::DidGetSenderIdFromStorage,
@@ -354,8 +379,20 @@ void PushMessagingMessageFilter::DidGetSenderIdFromStorage(
     return;
   }
   DCHECK_EQ(1u, sender_id.size());
+  const auto& stored_sender_id = sender_id[0];
+  bool stored_sender_id_is_numeric =
+      base::ContainsOnlyChars(stored_sender_id, "0123456789");
+  if (data.options.sender_info.empty() && !stored_sender_id_is_numeric) {
+    // Reject subscribes with no sender info, unless the stored id is numeric,

[awdf, 2016/11/02 15:03:00]

Hm, actually not sure I need any of this stuff. It depends if my new rules for
when you're allowed to not provide a key should just apply to resubscribes
(subscribe(), subscribe()) or also apply if you do subscribe(), unsubscribe(),
subscribe(). (Right now they apply in both causing a test failure for the latter
situation from PMBrowserTest.Subscribeworker)..

[awdf, 2016/11/03 14:10:11]

(Leaving it in and fixing up that test instead as it makes sense to apply the
rules consistently).

[harkness, 2016/11/03 15:09:27]

I think it's ok to leave this in for now, but long term we want to get down to a
single place this is in the code, which will be addressed by the refactor from
line 294.

+    // which is allowed in order to support the legacy way of subscribing from
+    // a service worker (first subscribe from the document using a gcm_sender_id
+    // set in the manifest, and then subscribe from the service worker with
+    // no key).
+    SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
+    return;
+  }
   RegisterData mutated_data = data;
-  mutated_data.options.sender_info = sender_id[0];
+  mutated_data.options.sender_info = stored_sender_id;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&Core::RegisterOnUI, base::Unretained(ui_core_.get()),