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..8de0e03e3152c626248fc622b7b7ed1fbed51d6b 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -294,37 +294,29 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
     const std::vector<std::string>& push_registration_id,
     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;
-  }
   // TODO(johnme): The spec allows the register algorithm to reject with an
   // AbortError when accessing storage fails. Perhaps we should do that if
   // service_worker_status != SERVICE_WORKER_ERROR_NOT_FOUND instead of
   // attempting to do a fresh registration?
   // https://w3c.github.io/push-api/#widl-PushRegistrationManager-register-Promise-PushRegistration
-  if (!data.options.sender_info.empty()) {
+  if (service_worker_status != SERVICE_WORKER_OK &&

[johnme, 2016/11/04 14:46:12]

(This patch feels a little awkward, because when there was a stored existing
push registration ID and data.options.sender_info was not empty,
DidGetSenderIdFromStorage doesn't currently do anything with the sender ID
fetched from storage. But I get that it'll make more sense once
https://codereview.chromium.org/2436393002 lands on top, so that's fine.)

[awdf, 2016/11/04 16:21:36]

Acknowledged.

+      !data.options.sender_info.empty()) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::Bind(&Core::RegisterOnUI,
                                        base::Unretained(ui_core_.get()), data));
-  } else {
-    service_worker_context_->GetRegistrationUserData(
-        data.service_worker_registration_id, {kPushSenderIdServiceWorkerKey},
-        base::Bind(&PushMessagingMessageFilter::DidGetSenderIdFromStorage,
-                   weak_factory_io_to_io_.GetWeakPtr(), data));
+    return;
+  }
+  if (service_worker_status == SERVICE_WORKER_OK) {

[johnme, 2016/11/04 14:46:12]

Nit: it'd be a little clearer to do both at the same time:

std::string stored_push_registration_id;
if (service_worker_status == SERVICE_WORKER_OK) {
  DCHECK_EQ(1u, push_registration_id.size());
  stored_push_registration_id = push_registration_id[0];
}

[awdf, 2016/11/04 16:21:36]

Done.

+    DCHECK_EQ(1u, push_registration_id.size());
   }
+  service_worker_context_->GetRegistrationUserData(
+      data.service_worker_registration_id, {kPushSenderIdServiceWorkerKey},
+      base::Bind(&PushMessagingMessageFilter::DidGetSenderIdFromStorage,
+                 weak_factory_io_to_io_.GetWeakPtr(),
+                 service_worker_status == SERVICE_WORKER_OK
+                     ? push_registration_id[0]
+                     : std::string(),
+                 data));
 }
 
 void PushMessagingMessageFilter::DidGetEncryptionKeys(

[johnme, 2016/11/04 14:46:12]

Nit: Please move this method below DidGetSenderIdFromStorage (in both .h and
.cc) to reflect the new order in which they are called.

[awdf, 2016/11/04 16:21:36]

Done.

@@ -345,6 +337,7 @@ void PushMessagingMessageFilter::DidGetEncryptionKeys(
 }
 
 void PushMessagingMessageFilter::DidGetSenderIdFromStorage(

[johnme, 2016/11/04 14:46:12]

This method got a little harder to understand, since it now has two roles. How
about adding a comment to the top along the lines of

"The stored sender ID was fetched:
- because sender_info was omitted, so consider using the stored sender
- and/or because there was a pre-existing registration, so sender_info should
match the stored sender"

and splitting the code up with nested ifs and returns:


if (data.options.sender_info.empty()) {
  if (!stored_sender_id_is_numeric) {
    SendSubscriptionError(...
    return;
  }
  if (push_registration_id.empty()) {
    PostTask(RegisterOnUI...
    return;
  }
}
DCHECK(!push_registration_id.empty());
// TODO(awdf): Check...
PostTask(GetEncryptionInfoOnUI...


wdyt?

[awdf, 2016/11/04 16:21:36]

Agree it could use a comment explaining why we're here, but I've phrased it
slightly differently and re-arranged the code to match. Wdyt?

+    const std::string& push_registration_id,
     const RegisterData& data,
     const std::vector<std::string>& sender_id,
     ServiceWorkerStatusCode service_worker_status) {
@@ -354,12 +347,39 @@ void PushMessagingMessageFilter::DidGetSenderIdFromStorage(
     return;
   }
   DCHECK_EQ(1u, sender_id.size());
-  RegisterData mutated_data = data;
-  mutated_data.options.sender_info = sender_id[0];
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&Core::RegisterOnUI, base::Unretained(ui_core_.get()),
-                 mutated_data));
+  bool stored_sender_id_is_numeric =

[johnme, 2016/11/04 14:46:12]

Nit: s/is_numeric/is_gcm_project_number/

[awdf, 2016/11/04 16:21:36]

Done.

+      base::ContainsOnlyChars(sender_id[0], "0123456789");
+  if (data.options.sender_info.empty() && !stored_sender_id_is_numeric) {
+    // Reject subscribes with no sender info, unless the stored id is numeric,

[johnme, 2016/11/04 14:46:12]

I know this comment is already quite long, but could you add "even if there is a
stored push_registration_id" after the first comma?

[awdf, 2016/11/04 16:21:36]

Done.

+    // 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);
+  } else if (push_registration_id.empty()) {
+    // No existing registration, so register anew with the stored sender id.
+    RegisterData mutated_data = data;
+    mutated_data.options.sender_info = sender_id[0];

[johnme, 2016/11/04 14:46:12]

Please add a DCHECK(data.options.sender_info.empty()) to document the fact that
it's known to already be blank in this case. Unless you do the nested ifs thing
I suggest above, in which case this'll be unnecessary.

[awdf, 2016/11/04 16:21:36]

Done.

+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&Core::RegisterOnUI, base::Unretained(ui_core_.get()),
+                   mutated_data));
+  } else {
+    // There is already a registration - use it.
+    // TODO(crbug.com/638924): Check that stored sender ID equals

[johnme, 2016/11/04 14:46:12]

Nit: chromium style says to put the username that knows about the issue in the
TODO brackets (so one of us). The bug number can be part of the TODO's
description instead.

[awdf, 2016/11/04 16:21:36]

Done.

+    // data.options.sender_info and throw an exception if they don't match.
+    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,
+                   data.options.sender_info.empty() ? sender_id[0]
+                                                    : data.options.sender_info,
+                   callback));
+  }
 }
 
 void PushMessagingMessageFilter::Core::RegisterOnUI(