Push API: Validate storage before returning cached subscriptions

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 9820f4f151d95be74ca3f6e4ad5b6fc6a80158dc..da84a15df75ef5428f31f76f472390acd2ecb37a 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -51,37 +51,30 @@ const char kIncognitoPushUnsupportedMessage[] =
     "feature-detect this, since incognito mode needs to be undetectable by "
     "websites.";
 
-// These UMA methods are only called from IO thread, but it would be acceptable
-// (even though slightly racy) to call them from UI thread as well, see
+// These UMA methods are called from the IO and/or UI threads. Racey but ok, see
 // https://groups.google.com/a/chromium.org/d/msg/chromium-dev/FNzZRJtN2aw/Aw0CWAXJJ1kJ
 void RecordRegistrationStatus(PushRegistrationStatus status) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
+         BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("PushMessaging.RegistrationStatus", status,
                             PUSH_REGISTRATION_STATUS_LAST + 1);
 }
-
 void RecordUnregistrationStatus(PushUnregistrationStatus status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   UMA_HISTOGRAM_ENUMERATION("PushMessaging.UnregistrationStatus", status,
                             PUSH_UNREGISTRATION_STATUS_LAST + 1);
 }
-
 void RecordGetRegistrationStatus(PushGetRegistrationStatus status) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
+         BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("PushMessaging.GetRegistrationStatus", status,
                             PUSH_GETREGISTRATION_STATUS_LAST + 1);
 }
 
-// Curries the |success| and |p256dh| parameters over to |callback| and
-// posts a task to invoke |callback| on the IO thread.
-void ForwardEncryptionInfoToIOThreadProxy(
-    const PushMessagingService::EncryptionInfoCallback& callback,
-    bool success,
-    const std::vector<uint8_t>& p256dh,
-    const std::vector<uint8_t>& auth) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(callback, success, p256dh, auth));
+void UnregisterCallbackToClosure(const base::Closure& closure,
+                                 PushUnregistrationStatus status) {
+  DCHECK(!closure.is_null());
+  closure.Run();
 }
 
 // Returns whether |sender_info| contains a valid application server key, that
@@ -130,6 +123,14 @@ class PushMessagingMessageFilter::Core {
 
   // Public Register methods on UI thread --------------------------------------
 
+  // Callback called on UI thread.
+  void SubscribeDidGetInfoOnUI(const RegisterData& data,
+                               const std::string& push_subscription_id,
+                               const std::string& sender_id,
+                               bool is_valid,
+                               const std::vector<uint8_t>& p256dh,
+                               const std::vector<uint8_t>& auth);
+
   // Called via PostTask from IO thread.
   void RegisterOnUI(const RegisterData& data);
 
@@ -141,6 +142,24 @@ class PushMessagingMessageFilter::Core {
                              const GURL& requesting_origin,
                              const std::string& sender_id);
 
+  // Public GetSubscription methods on UI thread -------------------------------
+
+  // Callback called on UI thread.
+  void GetSubscriptionDidGetInfoOnUI(int request_id,
+                                     const GURL& origin,
+                                     int64_t service_worker_registration_id,
+                                     const GURL& endpoint,
+                                     const std::string& sender_info,
+                                     bool is_valid,
+                                     const std::vector<uint8_t>& p256dh,
+                                     const std::vector<uint8_t>& auth);
+
+  // Callback called on UI thread.
+  void GetSubscriptionDidUnsubscribe(
+      int request_id,
+      PushGetRegistrationStatus get_status,
+      PushUnregistrationStatus unsubscribe_status);
+
   // Public GetPermission methods on UI thread ---------------------------------
 
   // Called via PostTask from IO thread.
@@ -150,13 +169,13 @@ class PushMessagingMessageFilter::Core {
 
   // Public helper methods on UI thread ----------------------------------------
 
-  // Called via PostTask from IO thread. The |io_thread_callback| callback
-  // will be invoked on the IO thread.
-  void GetEncryptionInfoOnUI(
+  // Called via PostTask from IO thread. |callback| will be run on UI thread.
+  void GetSubscriptionInfoOnUI(
       const GURL& origin,
       int64_t service_worker_registration_id,
       const std::string& sender_id,
-      const PushMessagingService::EncryptionInfoCallback& io_thread_callback);
+      const std::string& push_subscription_id,
+      const PushMessagingService::SubscriptionInfoCallback& callback);
 
   // Called (directly) from both the UI and IO threads.
   bool is_incognito() const { return is_incognito_; }
@@ -164,6 +183,9 @@ class PushMessagingMessageFilter::Core {
   // Returns a push messaging service. May return null.
   PushMessagingService* service();
 
+  // Returns a weak ptr. Must only be called from the outer class's constructor.
+  base::WeakPtr<Core> GetWeakPtrFromIOParentConstructor();
+
  private:
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class base::DeleteHelper<Core>;
@@ -239,6 +261,7 @@ PushMessagingMessageFilter::PushMessagingMessageFilter(
   // constructor finishes.
   ui_core_.reset(
       new Core(weak_factory_io_to_io_.GetWeakPtr(), render_process_id));
+  ui_core_weak_ptr_ = ui_core_->GetWeakPtrFromIOParentConstructor();
 
   PushMessagingService* service = ui_core_->service();
   service_available_ = !!service;
@@ -316,7 +339,7 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status == SERVICE_WORKER_OK) {
     DCHECK_EQ(2u, push_registration_id_and_sender_id.size());
-    const auto& push_registration_id = push_registration_id_and_sender_id[0];
+    const auto& push_subscription_id = push_registration_id_and_sender_id[0];
     const auto& stored_sender_id = push_registration_id_and_sender_id[1];
     std::string fixed_sender_id =
         FixSenderInfo(data.options.sender_info, stored_sender_id);
@@ -328,15 +351,14 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
       SendSubscriptionError(data, PUSH_REGISTRATION_STATUS_SENDER_ID_MISMATCH);
       return;
     }
-    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::Bind(&Core::GetSubscriptionInfoOnUI,
                    base::Unretained(ui_core_.get()), data.requesting_origin,
                    data.service_worker_registration_id, fixed_sender_id,
-                   callback));
+                   push_subscription_id,
+                   base::Bind(&Core::SubscribeDidGetInfoOnUI, ui_core_weak_ptr_,
+                              data, push_subscription_id, fixed_sender_id)));
     return;
   }
   // TODO(johnme): The spec allows the register algorithm to reject with an
@@ -358,21 +380,46 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
   }
 }
 
-void PushMessagingMessageFilter::DidGetEncryptionKeys(
+void PushMessagingMessageFilter::Core::SubscribeDidGetInfoOnUI(
     const RegisterData& data,
-    const std::string& push_registration_id,
-    bool success,
+    const std::string& push_subscription_id,
+    const std::string& sender_id,
+    bool is_valid,
     const std::vector<uint8_t>& p256dh,
     const std::vector<uint8_t>& auth) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (!success) {
-    SendSubscriptionError(
-        data, PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE);
-    return;
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (is_valid) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&PushMessagingMessageFilter::SendSubscriptionSuccess,
+                   io_parent_, data,
+                   PUSH_REGISTRATION_STATUS_SUCCESS_FROM_CACHE,
+                   push_subscription_id, p256dh, auth));
+  } else {
+    // Uh-oh! Although there was a cached subscription in the Service Worker
+    // database, it did not have matching counterparts in the
+    // PushMessagingAppIdentifier map and/or GCM Store. Unsubscribe and
+    // re-subscribe to fix this inconsistency.
+
+    // Consider this subscription attempt to have failed. The re-subscribe will
+    // be logged to UMA as a separate subscription attempt.
+    RecordRegistrationStatus(PUSH_REGISTRATION_STATUS_STORAGE_CORRUPT);
+
+    PushMessagingService* push_service = service();
+    DCHECK(push_service);  // NOT_FOUND response can only come from service.

[Peter Beverloo, 2017/03/20 23:50:13]

Core::GetSubscriptionInfoOnUI specifically returns is_valid==false for cases
where there isn't a Push Service. That seems to be in conflict with this
comment?

[johnme, 2017/03/30 18:36:38]

Hmm yeah. In practice it shouldn't be possible to have a stored push
subscription in a profile that doesn't have a push service though. I kept the
DCHECK (well, NOTREACHED), but added a fallback for release builds that returns
PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE, in case some refactoring or
edge case (kill switch?) makes that stop being true.

+    push_service->Unsubscribe(
+        PUSH_UNREGISTRATION_REASON_SUBSCRIBE_STORAGE_CORRUPT,
+        data.requesting_origin, data.service_worker_registration_id, sender_id,
+        base::Bind(&UnregisterCallbackToClosure,
+                   base::Bind(IgnoreResult(&BrowserThread::PostTask),
+                              BrowserThread::IO, FROM_HERE,
+                              base::Bind(&PushMessagingMessageFilter::
+                                             DidCheckForExistingRegistration,
+                                         io_parent_, data,
+                                         // push_registration_id_and_sender_id
+                                         std::vector<std::string>(),
+                                         SERVICE_WORKER_ERROR_NOT_FOUND))));

[Peter Beverloo, 2017/03/20 23:50:13]

This is deep. Like, literally. Can we maybe make it slightly less deep by using
anonymous functions to bridge?

[johnme, 2017/03/30 18:36:38]

Done.

   }
-
-  SendSubscriptionSuccess(data, PUSH_REGISTRATION_STATUS_SUCCESS_FROM_CACHE,
-                          push_registration_id, p256dh, auth);
 }
 
 void PushMessagingMessageFilter::DidGetSenderIdFromStorage(
@@ -635,7 +682,8 @@ void PushMessagingMessageFilter::Core::UnregisterFromService(
   }
 
   push_service->Unsubscribe(
-      requesting_origin, service_worker_registration_id, sender_id,
+      PUSH_UNREGISTRATION_REASON_JAVASCRIPT_API, requesting_origin,
+      service_worker_registration_id, sender_id,
       base::Bind(&Core::DidUnregisterFromService,
                  weak_factory_ui_to_ui_.GetWeakPtr(), request_id,
                  service_worker_registration_id));
@@ -709,6 +757,9 @@ void PushMessagingMessageFilter::DidGetSubscription(
   switch (service_worker_status) {
     case SERVICE_WORKER_OK: {
       DCHECK_EQ(2u, push_subscription_id_and_sender_info.size());
+      const std::string& push_subscription_id =
+          push_subscription_id_and_sender_info[0];
+      const std::string& sender_info = push_subscription_id_and_sender_info[1];
 
       if (!service_available_) {
         // Return not found in incognito mode, so websites can't detect it.
@@ -724,22 +775,20 @@ void PushMessagingMessageFilter::DidGetSubscription(
               service_worker_registration_id);
       const GURL origin = registration->pattern().GetOrigin();
 
-      const bool uses_standard_protocol =
-          IsApplicationServerKey(push_subscription_id_and_sender_info[1]);
-      const GURL endpoint = CreateEndpoint(
-          uses_standard_protocol, push_subscription_id_and_sender_info[0]);
-
-      auto callback =
-          base::Bind(&PushMessagingMessageFilter::DidGetSubscriptionKeys,
-                     weak_factory_io_to_io_.GetWeakPtr(), request_id, endpoint,
-                     push_subscription_id_and_sender_info[1]);
+      const bool uses_standard_protocol = IsApplicationServerKey(sender_info);
+      const GURL endpoint =
+          CreateEndpoint(uses_standard_protocol, push_subscription_id);
 
       BrowserThread::PostTask(
           BrowserThread::UI, FROM_HERE,
-          base::Bind(&Core::GetEncryptionInfoOnUI,
+          base::Bind(&Core::GetSubscriptionInfoOnUI,
                      base::Unretained(ui_core_.get()), origin,
-                     service_worker_registration_id,
-                     push_subscription_id_and_sender_info[1], callback));
+                     service_worker_registration_id, sender_info,
+                     push_subscription_id,
+                     base::Bind(&Core::GetSubscriptionDidGetInfoOnUI,
+                                ui_core_weak_ptr_, request_id, origin,
+                                service_worker_registration_id, endpoint,
+                                sender_info)));
 
       return;
     }
@@ -778,35 +827,55 @@ void PushMessagingMessageFilter::DidGetSubscription(
   RecordGetRegistrationStatus(get_status);
 }
 
-void PushMessagingMessageFilter::DidGetSubscriptionKeys(
+void PushMessagingMessageFilter::Core::GetSubscriptionDidGetInfoOnUI(
     int request_id,
+    const GURL& origin,
+    int64_t service_worker_registration_id,
     const GURL& endpoint,
     const std::string& sender_info,
-    bool success,
+    bool is_valid,
     const std::vector<uint8_t>& p256dh,
     const std::vector<uint8_t>& auth) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (!success) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (is_valid) {
+    PushSubscriptionOptions options;
+    // Chrome rejects subscription requests with userVisibleOnly false, so it
+    // must have been true. TODO(harkness): If Chrome starts accepting silent
+    // push subscriptions with userVisibleOnly false, the bool will need to be
+    // stored.
+    options.user_visible_only = true;
+    options.sender_info = sender_info;
+
+    Send(new PushMessagingMsg_GetSubscriptionSuccess(request_id, endpoint,
+                                                     options, p256dh, auth));
+
+    RecordGetRegistrationStatus(PUSH_GETREGISTRATION_STATUS_SUCCESS);
+  } else {
+    // Uh-oh! Although there was a cached subscription in the Service Worker
+    // database, it did not have matching counterparts in the
+    // PushMessagingAppIdentifier map and/or GCM Store. Unsubscribe to fix this
+    // inconsistency.
     PushGetRegistrationStatus status =
-        PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE;
+        PUSH_GETREGISTRATION_STATUS_STORAGE_CORRUPT;
 
-    Send(new PushMessagingMsg_GetSubscriptionError(request_id, status));
+    PushMessagingService* push_service = service();
+    DCHECK(push_service);  // NOT_FOUND response can only come from service.
+    push_service->Unsubscribe(
+        PUSH_UNREGISTRATION_REASON_GET_SUBSCRIPTION_STORAGE_CORRUPT, origin,
+        service_worker_registration_id, sender_info,
+        base::Bind(&Core::GetSubscriptionDidUnsubscribe,
+                   weak_factory_ui_to_ui_.GetWeakPtr(), request_id, status));
 
     RecordGetRegistrationStatus(status);
-    return;
   }
+}
 
-  PushSubscriptionOptions options;
-  // Chrome rejects subscription requests with userVisibleOnly false, so it must
-  // have been true. TODO(harkness): If Chrome starts accepting silent push
-  // subscriptions with userVisibleOnly false, the bool will need to be stored.
-  options.user_visible_only = true;
-  options.sender_info = sender_info;
-
-  Send(new PushMessagingMsg_GetSubscriptionSuccess(request_id, endpoint,
-                                                   options, p256dh, auth));
-
-  RecordGetRegistrationStatus(PUSH_GETREGISTRATION_STATUS_SUCCESS);
+void PushMessagingMessageFilter::Core::GetSubscriptionDidUnsubscribe(
+    int request_id,
+    PushGetRegistrationStatus get_status,
+    PushUnregistrationStatus unsubscribe_status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  Send(new PushMessagingMsg_GetSubscriptionError(request_id, get_status));
 }
 
 // GetPermission methods on both IO and UI threads, merged in order of use from
@@ -864,24 +933,22 @@ void PushMessagingMessageFilter::Core::GetPermissionStatusOnUI(
 // PushMessagingMessageFilter and Core.
 // -----------------------------------------------------------------------------
 
-void PushMessagingMessageFilter::Core::GetEncryptionInfoOnUI(
+void PushMessagingMessageFilter::Core::GetSubscriptionInfoOnUI(
     const GURL& origin,
     int64_t service_worker_registration_id,
     const std::string& sender_id,
-    const PushMessagingService::EncryptionInfoCallback& io_thread_callback) {
+    const std::string& push_subscription_id,
+    const PushMessagingService::SubscriptionInfoCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   PushMessagingService* push_service = service();
-  if (push_service) {
-    push_service->GetEncryptionInfo(
-        origin, service_worker_registration_id, sender_id,
-        base::Bind(&ForwardEncryptionInfoToIOThreadProxy, io_thread_callback));
+  if (!push_service) {
+    callback.Run(false /* is_valid */, std::vector<uint8_t>() /* p256dh */,
+                 std::vector<uint8_t>() /* auth */);
     return;
   }
 
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(io_thread_callback, false /* success */,
-                                     std::vector<uint8_t>() /* p256dh */,
-                                     std::vector<uint8_t>() /* auth */));
+  push_service->GetSubscriptionInfo(origin, service_worker_registration_id,
+                                    sender_id, push_subscription_id, callback);
 }
 
 void PushMessagingMessageFilter::Core::Send(IPC::Message* message) {
@@ -914,4 +981,10 @@ PushMessagingService* PushMessagingMessageFilter::Core::service() {
              : nullptr;
 }
 
+base::WeakPtr<PushMessagingMessageFilter::Core>
+PushMessagingMessageFilter::Core::GetWeakPtrFromIOParentConstructor() {

[Peter Beverloo, 2017/03/20 23:50:13]

Would you mind summarising in two sentences or so why it's important to have
this method and cache the WeakPtr?

[johnme, 2017/03/30 18:36:38]

weak_factory_ui_to_ui_ should not be public, as it's only safe to use on the UI
thread, and the outer class lives on the IO thread. The constructor of the outer
class exceptionally runs on the UI thread, so can safely grab a weak ptr, hence
this public wrapper method that DCHECKs it's called on UI thread.

I added the following comment to ui_core_weak_ptr_ which hopefully the purpose
clearer:
  // Can be used on the IO thread as the |this| parameter when binding a
  // callback that will be called on the UI thread (an IO -> UI -> UI chain).

And clarified the comment on GetWeakPtrFromIOParentConstructor:
  // Returns a weak ptr. Must only be called on the UI thread (and hence can
  // only be called from the outer class's constructor).

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  return weak_factory_ui_to_ui_.GetWeakPtr();
+}
+
 }  // namespace content