Introduce the PushSubscription.curve25519dh attribute (2/3)

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 7d550ac7b2f56dc51cd8fb52b3608beb1fdbbe17..87321de77294b2e2f3548f6e834e0561454b6290 100644
--- a/content/browser/push_messaging/push_messaging_message_filter.cc
+++ b/content/browser/push_messaging/push_messaging_message_filter.cc
@@ -64,6 +64,17 @@ void RecordGetRegistrationStatus(PushGetRegistrationStatus status) {
                             PUSH_GETREGISTRATION_STATUS_LAST + 1);
 }
 
+// Curries the |success| and |curve25519dh| parameters over to |callback| and
+// posts a task to invoke |callback| on the IO thread.
+void ForwardPublicEncryptionKeysToIOThreadProxy(

[johnme, 2015/07/09 16:02:53]

It's a bit unusual that Core::GetPublicEncryptionKeyOnUI gets called on UI
thread, but runs its callback on IO thread.

I've looked at the two callbacks (DidGetEncryptionKeys &
DidGetRegistrationKeys), and it seems both of them could actually be implemented
on the UI thread almost as easily (you can replace Send with the Core::Send
proxy; SendRegister{Success,Error} can be PostTask'd to; finally
RecordGetRegistrationStatus is safe enough to call from UI thread - just change
the DCHECK_CURRENTLY_ON).

Then you could remove ForwardPublicEncryptionKeysToIOThreadProxy. Alternatively,
if you prefer to keep the current threading, then document it clearly (rename
Core::GetPublicEncryptionKeyOnUI's callback parameter to io_thread_callback, and
add a comment in the method declaration).

[Peter Beverloo, 2015/07/09 18:54:17]

All the Record*Status have DCHECKS to be on the IO thread, so
DidGetRegistrationKeys would become messier w/ another PostTask.

I've updated the argument names and comment.

+    const PushMessagingService::PublicKeyCallback& callback,
+    bool success,
+    const std::vector<uint8_t>& curve25519dh) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(callback, success, curve25519dh));
+}
+
 // Concatenates the subscription id with the endpoint base to create a new
 // GURL object containing the endpoint unique to the subscription.
 GURL CreatePushEndpoint(const GURL& push_endpoint_base,
@@ -97,6 +108,12 @@ class PushMessagingMessageFilter::Core {
   // Called via PostTask from IO thread.
   void RegisterOnUI(const RegisterData& data, const std::string& sender_id);
 
+  // Called via PostTask from IO thread.
+  void GetPublicEncryptionKeyOnUI(

[johnme, 2015/07/09 16:02:53]

Please move this to the "Public helper methods on UI thread" section, since it's
used both for the Register flow and the GetRegistration flow.

[Peter Beverloo, 2015/07/09 18:54:17]

Done.

+      const GURL& origin,
+      int64_t service_worker_registration_id,
+      const PushMessagingService::PublicKeyCallback& callback);
+
   // Public Unregister methods on UI thread ------------------------------------
 
   // Called via PostTask from IO thread.
@@ -130,6 +147,7 @@ class PushMessagingMessageFilter::Core {
 
   void DidRegister(const RegisterData& data,
                    const std::string& push_registration_id,
+                   const std::vector<uint8_t>& curve25519dh,
                    PushRegistrationStatus status);
 
   // Private Unregister methods on UI thread -----------------------------------
@@ -312,8 +330,16 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status == SERVICE_WORKER_OK) {
-    SendRegisterSuccess(data, PUSH_REGISTRATION_STATUS_SUCCESS_FROM_CACHE,
-                        push_registration_id);
+    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::GetPublicEncryptionKeyOnUI,
+                   base::Unretained(ui_core_.get()), data.requesting_origin,
+                   data.service_worker_registration_id, callback));
     return;
   }
   // TODO(johnme): The spec allows the register algorithm to reject with an
@@ -335,6 +361,39 @@ void PushMessagingMessageFilter::DidCheckForExistingRegistration(
   }
 }
 
+void PushMessagingMessageFilter::Core::GetPublicEncryptionKeyOnUI(

[johnme, 2015/07/09 16:02:53]

Similarly, please move this to the "Helper methods on both IO and UI threads,
merged from PushMessagingMessageFilter and Core." section at the bottom of this
file.

[Peter Beverloo, 2015/07/09 18:54:17]

Done.

+    const GURL& origin,
+    int64_t service_worker_registration_id,
+    const PushMessagingService::PublicKeyCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  PushMessagingService* push_service = service();
+  if (push_service) {
+    push_service->GetPublicEncryptionKey(
+        origin, service_worker_registration_id,
+        base::Bind(&ForwardPublicEncryptionKeysToIOThreadProxy, callback));
+    return;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(callback, false /* success */, std::vector<uint8_t>()));
+}
+
+void PushMessagingMessageFilter::DidGetEncryptionKeys(
+    const RegisterData& data,
+    const std::string& push_registration_id,
+    bool success,
+    const std::vector<uint8_t>& curve25519dh) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!success) {
+    SendRegisterError(data, PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE);
+    return;
+  }
+
+  SendRegisterSuccess(data, PUSH_REGISTRATION_STATUS_SUCCESS_FROM_CACHE,
+                      push_registration_id, curve25519dh);
+}
+
 void PushMessagingMessageFilter::DidGetSenderIdFromStorage(
     const RegisterData& data,
     const std::string& sender_id,
@@ -409,13 +468,14 @@ void PushMessagingMessageFilter::Core::RegisterOnUI(
 void PushMessagingMessageFilter::Core::DidRegister(
     const RegisterData& data,
     const std::string& push_registration_id,
+    const std::vector<uint8_t>& curve25519dh,
     PushRegistrationStatus status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (status == PUSH_REGISTRATION_STATUS_SUCCESS_FROM_PUSH_SERVICE) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&PushMessagingMessageFilter::PersistRegistrationOnIO,
-                   io_parent_, data, push_registration_id));
+                   io_parent_, data, push_registration_id, curve25519dh));
   } else {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
@@ -426,7 +486,8 @@ void PushMessagingMessageFilter::Core::DidRegister(
 
 void PushMessagingMessageFilter::PersistRegistrationOnIO(
     const RegisterData& data,
-    const std::string& push_registration_id) {
+    const std::string& push_registration_id,
+    const std::vector<uint8_t>& curve25519dh) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   service_worker_context_->StoreRegistrationUserData(
       data.service_worker_registration_id,
@@ -435,18 +496,19 @@ void PushMessagingMessageFilter::PersistRegistrationOnIO(
       push_registration_id,
       base::Bind(&PushMessagingMessageFilter::DidPersistRegistrationOnIO,
                  weak_factory_io_to_io_.GetWeakPtr(),
-                 data, push_registration_id));
+                 data, push_registration_id, curve25519dh));
 }
 
 void PushMessagingMessageFilter::DidPersistRegistrationOnIO(
     const RegisterData& data,
     const std::string& push_registration_id,
+    const std::vector<uint8_t>& curve25519dh,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status == SERVICE_WORKER_OK) {
     SendRegisterSuccess(data,
                         PUSH_REGISTRATION_STATUS_SUCCESS_FROM_PUSH_SERVICE,
-                        push_registration_id);
+                        push_registration_id, curve25519dh);
   } else {
     // TODO(johnme): Unregister, so PushMessagingServiceImpl can decrease count.
     SendRegisterError(data, PUSH_REGISTRATION_STATUS_STORAGE_ERROR);
@@ -470,7 +532,8 @@ void PushMessagingMessageFilter::SendRegisterError(
 void PushMessagingMessageFilter::SendRegisterSuccess(
     const RegisterData& data,
     PushRegistrationStatus status,
-    const std::string& push_registration_id) {
+    const std::string& push_registration_id,
+    const std::vector<uint8_t>& curve25519dh) {
   // Only called from IO thread, but would be safe to call from UI thread.
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (push_endpoint_base_.is_empty()) {
@@ -484,11 +547,13 @@ void PushMessagingMessageFilter::SendRegisterSuccess(
     Send(new PushMessagingMsg_SubscribeFromDocumentSuccess(
         data.render_frame_id,
         data.request_id,
-        CreatePushEndpoint(push_endpoint_base_, push_registration_id)));
+        CreatePushEndpoint(push_endpoint_base_, push_registration_id),
+        curve25519dh));
   } else {
     Send(new PushMessagingMsg_SubscribeFromWorkerSuccess(
         data.request_id,
-        CreatePushEndpoint(push_endpoint_base_, push_registration_id)));
+        CreatePushEndpoint(push_endpoint_base_, push_registration_id),
+        curve25519dh));
   }
   RecordRegistrationStatus(status);
 }
@@ -712,6 +777,7 @@ void PushMessagingMessageFilter::DidUnregister(
 
 void PushMessagingMessageFilter::OnGetRegistration(
     int request_id,
+    const GURL& origin,
     int64_t service_worker_registration_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // TODO(johnme): Validate arguments?
@@ -719,18 +785,21 @@ void PushMessagingMessageFilter::OnGetRegistration(
       service_worker_registration_id,
       kPushRegistrationIdServiceWorkerKey,
       base::Bind(&PushMessagingMessageFilter::DidGetRegistration,
-                 weak_factory_io_to_io_.GetWeakPtr(), request_id));
+                 weak_factory_io_to_io_.GetWeakPtr(), request_id, origin,
+                 service_worker_registration_id));
 }
 
 void PushMessagingMessageFilter::DidGetRegistration(
     int request_id,
+    const GURL& origin,
+    int64_t service_worker_registration_id,
     const std::string& push_registration_id,
     ServiceWorkerStatusCode service_worker_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   PushGetRegistrationStatus get_status =
       PUSH_GETREGISTRATION_STATUS_STORAGE_ERROR;
   switch (service_worker_status) {
-    case SERVICE_WORKER_OK:
+    case SERVICE_WORKER_OK: {
       if (push_endpoint_base_.is_empty()) {
         // Return not found in incognito mode, so websites can't detect it.
         get_status =
@@ -740,17 +809,29 @@ void PushMessagingMessageFilter::DidGetRegistration(
         break;
       }
 
-      Send(new PushMessagingMsg_GetRegistrationSuccess(
-          request_id,
-          CreatePushEndpoint(push_endpoint_base_, push_registration_id)));
-      RecordGetRegistrationStatus(PUSH_GETREGISTRATION_STATUS_SUCCESS);
+      const GURL endpoint =
+          CreatePushEndpoint(push_endpoint_base_, push_registration_id);
+
+      auto callback =
+          base::Bind(&PushMessagingMessageFilter::DidGetRegistrationKeys,
+                     weak_factory_io_to_io_.GetWeakPtr(), request_id, endpoint);
+
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&Core::GetPublicEncryptionKeyOnUI,
+                     base::Unretained(ui_core_.get()), origin,
+                     service_worker_registration_id, callback));
+
       return;
-    case SERVICE_WORKER_ERROR_NOT_FOUND:
+    }
+    case SERVICE_WORKER_ERROR_NOT_FOUND: {
       get_status = PUSH_GETREGISTRATION_STATUS_REGISTRATION_NOT_FOUND;
       break;
-    case SERVICE_WORKER_ERROR_FAILED:
+    }
+    case SERVICE_WORKER_ERROR_FAILED: {
       get_status = PUSH_GETREGISTRATION_STATUS_STORAGE_ERROR;
       break;
+    }
     case SERVICE_WORKER_ERROR_ABORT:
     case SERVICE_WORKER_ERROR_START_WORKER_FAILED:
     case SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND:
@@ -766,16 +847,38 @@ void PushMessagingMessageFilter::DidGetRegistration(
     case SERVICE_WORKER_ERROR_SCRIPT_EVALUATE_FAILED:
     case SERVICE_WORKER_ERROR_DISK_CACHE:
     case SERVICE_WORKER_ERROR_REDUNDANT:
-    case SERVICE_WORKER_ERROR_MAX_VALUE:
+    case SERVICE_WORKER_ERROR_MAX_VALUE: {
       NOTREACHED() << "Got unexpected error code: " << service_worker_status
                    << " " << ServiceWorkerStatusToString(service_worker_status);
       get_status = PUSH_GETREGISTRATION_STATUS_STORAGE_ERROR;
       break;
+    }
   }
   Send(new PushMessagingMsg_GetRegistrationError(request_id, get_status));
   RecordGetRegistrationStatus(get_status);
 }
 
+void PushMessagingMessageFilter::DidGetRegistrationKeys(

[johnme, 2015/07/09 16:02:53]

Please add a DCHECK_CURRENTLY_ON.

[Peter Beverloo, 2015/07/09 18:54:17]

Done.

+    int request_id,
+    const GURL& endpoint,
+    bool success,
+    const std::vector<uint8_t>& curve25519dh) {
+  if (!success) {
+    PushGetRegistrationStatus status =
+        PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE;
+
+    Send(new PushMessagingMsg_GetRegistrationError(request_id, status));
+
+    RecordGetRegistrationStatus(status);
+    return;
+  }
+
+  Send(new PushMessagingMsg_GetRegistrationSuccess(request_id, endpoint,
+                                                   curve25519dh));
+
+  RecordGetRegistrationStatus(PUSH_GETREGISTRATION_STATUS_SUCCESS);
+}
+
 // GetPermission methods on both IO and UI threads, merged in order of use from
 // PushMessagingMessageFilter and Core.
 // -----------------------------------------------------------------------------