Stop passing the origin in the GetPermissionStatus() call for notifications

We can make the browser process authoritative by having the interface
request be routed through RendererInterfaceBinders.

A consequence of this is that we no longer will be able to establish a
connection after the context is destroyed. That's fine, but leads to a
minor change in behaviour: when calling Notification.permission from
a detached context, it will now always return "denied".

Other features tend to reject the promise in this situation, but we
don't have one of those. Throwing when accessing the property will
definitely have (bad) side effects.

Bug: 595685
Change-Id: I51b988cedabd0689bb118a3d841a6864eb64a24a
Reviewed-on: https://chromium-review.googlesource.com/791550
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519820}

content/browser/notifications/blink_notification_service_impl.cc

@@ -28,10 +28,12 @@ BlinkNotificationServiceImpl::BlinkNotificationServiceImpl(
     PlatformNotificationContextImpl* notification_context,
     ResourceContext* resource_context,
     int render_process_id,
+    const url::Origin& origin,
     mojo::InterfaceRequest<blink::mojom::NotificationService> request)
     : notification_context_(notification_context),
       resource_context_(resource_context),
       render_process_id_(render_process_id),
+      origin_(origin),
       binding_(this, std::move(request)) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(notification_context_);
@@ -47,17 +49,15 @@ BlinkNotificationServiceImpl::~BlinkNotificationServiceImpl() {
 }
 
 void BlinkNotificationServiceImpl::GetPermissionStatus(
-    const std::string& origin,
     GetPermissionStatusCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   if (!Service()) {
     std::move(callback).Run(blink::mojom::PermissionStatus::DENIED);
     return;
   }
 
   blink::mojom::PermissionStatus permission_status =
-      Service()->CheckPermissionOnIOThread(resource_context_, GURL(origin),
+      Service()->CheckPermissionOnIOThread(resource_context_, origin_.GetURL(),
                                            render_process_id_);
 
   std::move(callback).Run(permission_status);

content/browser/notifications/blink_notification_service_impl.h

@@ -9,6 +9,7 @@
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "third_party/WebKit/public/platform/modules/notifications/notification_service.mojom.h"
+#include "url/origin.h"
 
 namespace content {
 
@@ -24,12 +25,12 @@ class BlinkNotificationServiceImpl : public blink::mojom::NotificationService {
       PlatformNotificationContextImpl* notification_context,
       ResourceContext* resource_context,
       int render_process_id,
+      const url::Origin& origin,
       mojo::InterfaceRequest<blink::mojom::NotificationService> request);
   ~BlinkNotificationServiceImpl() override;
 
   // blink::mojom::NotificationService implementation.
-  void GetPermissionStatus(const std::string& origin,
-                           GetPermissionStatusCallback callback) override;
+  void GetPermissionStatus(GetPermissionStatusCallback callback) override;
 
  private:
   // Called when an error is detected on binding_.
@@ -42,6 +43,9 @@ class BlinkNotificationServiceImpl : public blink::mojom::NotificationService {
 
   int render_process_id_;
 
+  // The origin that this notification service is communicating with.
+  url::Origin origin_;
+
   mojo::Binding<blink::mojom::NotificationService> binding_;
 
   DISALLOW_COPY_AND_ASSIGN(BlinkNotificationServiceImpl);

content/browser/notifications/platform_notification_context_impl.cc

@@ -125,22 +125,25 @@ void PlatformNotificationContextImpl::ShutdownOnIO() {
 
 void PlatformNotificationContextImpl::CreateService(
     int render_process_id,
+    const url::Origin& origin,
     blink::mojom::NotificationServiceRequest request) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::BindOnce(&PlatformNotificationContextImpl::CreateServiceOnIO, this,
-                     render_process_id, browser_context_->GetResourceContext(),
+                     render_process_id, origin,
+                     browser_context_->GetResourceContext(),
                      base::Passed(&request)));
 }
 
 void PlatformNotificationContextImpl::CreateServiceOnIO(
     int render_process_id,
+    const url::Origin& origin,
     ResourceContext* resource_context,
     mojo::InterfaceRequest<blink::mojom::NotificationService> request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   services_.push_back(std::make_unique<BlinkNotificationServiceImpl>(
-      this, resource_context, render_process_id, std::move(request)));
+      this, resource_context, render_process_id, origin, std::move(request)));
 }
 
 void PlatformNotificationContextImpl::RemoveService(

content/browser/notifications/platform_notification_context_impl.h

@@ -29,6 +29,10 @@ namespace base {
 class SequencedTaskRunner;
 }
 
+namespace url {
+class Origin;
+}
+
 namespace content {
 
 class BlinkNotificationServiceImpl;
@@ -64,6 +68,7 @@ class CONTENT_EXPORT PlatformNotificationContextImpl
   // be called on the UI thread, although the service will be created on and
   // bound to the IO thread.
   void CreateService(int render_process_id,
+                     const url::Origin& origin,
                      blink::mojom::NotificationServiceRequest request);
 
   // Removes |service| from the list of owned services, for example because the
@@ -109,6 +114,7 @@ class CONTENT_EXPORT PlatformNotificationContextImpl
   void ShutdownOnIO();
   void CreateServiceOnIO(
       int render_process_id,
+      const url::Origin& origin,
       ResourceContext* resource_context,
       mojo::InterfaceRequest<blink::mojom::NotificationService> request);
 

content/browser/renderer_host/render_process_host_impl.cc

@@ -103,7 +103,6 @@
 #include "content/browser/mus_util.h"
 #include "content/browser/net/reporting_service_proxy.h"
 #include "content/browser/notifications/notification_message_filter.h"
-#include "content/browser/notifications/platform_notification_context_impl.h"
 #include "content/browser/payments/payment_manager.h"
 #include "content/browser/permissions/permission_service_context.h"
 #include "content/browser/permissions/permission_service_impl.h"
@@ -1827,12 +1826,6 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
       base::Bind(&BackgroundSyncContext::CreateService,
                  base::Unretained(
                      storage_partition_impl_->GetBackgroundSyncContext())));
-  AddUIThreadInterface(
-      registry.get(),
-      base::Bind(&PlatformNotificationContextImpl::CreateService,
-                 base::Unretained(
-                     storage_partition_impl_->GetPlatformNotificationContext()),
-                 GetID()));
   AddUIThreadInterface(
       registry.get(),
       base::Bind(&RenderProcessHostImpl::CreateStoragePartitionService,

content/browser/renderer_interface_binders.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "content/browser/background_fetch/background_fetch_service_impl.h"
 #include "content/browser/dedicated_worker/dedicated_worker_host.h"
+#include "content/browser/notifications/platform_notification_context_impl.h"
 #include "content/browser/payments/payment_manager.h"
 #include "content/browser/permissions/permission_service_context.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
@@ -27,6 +28,7 @@
 #include "services/shape_detection/public/interfaces/constants.mojom.h"
 #include "services/shape_detection/public/interfaces/facedetection_provider.mojom.h"
 #include "services/shape_detection/public/interfaces/textdetection.mojom.h"
+#include "third_party/WebKit/public/platform/modules/notifications/notification_service.mojom.h"
 #include "url/origin.h"
 
 namespace content {
@@ -123,6 +125,13 @@ void RendererInterfaceBinders::InitializeParameterizedBinderRegistry() {
       }));
   parameterized_binder_registry_.AddInterface(
       base::Bind(&CreateDedicatedWorkerHostFactory));
+  parameterized_binder_registry_.AddInterface(
+      base::Bind([](blink::mojom::NotificationServiceRequest request,
+                    RenderProcessHost* host, const url::Origin& origin) {
+        static_cast<StoragePartitionImpl*>(host->GetStoragePartition())
+            ->GetPlatformNotificationContext()
+            ->CreateService(host->GetID(), origin, std::move(request));
+      }));
 }
 
 RendererInterfaceBinders& GetRendererInterfaceBinders() {

content/public/app/mojo/content_browser_manifest.json

@@ -127,6 +127,7 @@
           "blink::mojom::InsecureInputService",
           "blink::mojom::KeyboardLockService",
           "blink::mojom::MediaSessionService",
+          "blink::mojom::NotificationService",
           "blink::mojom::PermissionService",
           "blink::mojom::PresentationService",
           "blink::mojom::TextSuggestionHost",
@@ -175,6 +176,7 @@
     "navigation:dedicated_worker": {
       "provides": {
         "renderer": [
+          "blink::mojom::NotificationService",
           "blink::mojom::PermissionService",
           "payments::mojom::PaymentManager",
           "shape_detection::mojom::BarcodeDetection",
@@ -186,6 +188,7 @@
     "navigation:service_worker": {
       "provides": {
         "renderer": [
+          "blink::mojom::NotificationService",
           "blink::mojom::PermissionService",
           "blink::mojom::WebSocket",
           "payments::mojom::PaymentManager",
@@ -198,6 +201,7 @@
     "navigation:shared_worker": {
       "provides": {
         "renderer": [
+          "blink::mojom::NotificationService",
           "blink::mojom::PermissionService",
           "blink::mojom::WebSocket",
           "payments::mojom::PaymentManager",

third_party/WebKit/LayoutTests/http/tests/notifications/request-permission-detached-context.html

@@ -26,7 +26,7 @@
                   win.close();
 
               } else if (event.data == 'closed') {
-                  assert_equals(notificationObj.permission, 'granted');
+                  assert_equals(notificationObj.permission, 'denied');
                   notificationObj.requestPermission(function () {});
 
                   test.done();

third_party/WebKit/Source/modules/notifications/Notification.cpp

@@ -172,8 +172,7 @@ void Notification::PrepareShow() {
     return;
   }
 
-  if (NotificationManager::From(GetExecutionContext())
-          ->GetPermissionStatus(GetExecutionContext()) !=
+  if (NotificationManager::From(GetExecutionContext())->GetPermissionStatus() !=
       mojom::blink::PermissionStatus::GRANTED) {
     DispatchErrorEvent();
     return;
@@ -380,7 +379,7 @@ String Notification::permission(ExecutionContext* context) {
     return PermissionString(mojom::blink::PermissionStatus::DENIED);
 
   mojom::blink::PermissionStatus status =
-      NotificationManager::From(context)->GetPermissionStatus(context);
+      NotificationManager::From(context)->GetPermissionStatus();
 
   // Permission can only be requested from top-level frames and same-origin
   // iframes. This should be reflected in calls getting permission status.

third_party/WebKit/Source/modules/notifications/NotificationManager.cpp

@@ -17,6 +17,7 @@
 #include "public/platform/Platform.h"
 #include "public/platform/modules/permissions/permission.mojom-blink.h"
 #include "public/platform/modules/permissions/permission_status.mojom-blink.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
 
 namespace blink {
 
@@ -29,7 +30,7 @@ NotificationManager* NotificationManager::From(
   NotificationManager* manager = static_cast<NotificationManager*>(
       Supplement<ExecutionContext>::From(execution_context, SupplementName()));
   if (!manager) {
-    manager = new NotificationManager();
+    manager = new NotificationManager(*execution_context);
     Supplement<ExecutionContext>::ProvideTo(*execution_context,
                                             SupplementName(), manager);
   }
@@ -42,21 +43,17 @@ const char* NotificationManager::SupplementName() {
   return "NotificationManager";
 }
 
-NotificationManager::NotificationManager() {}
+NotificationManager::NotificationManager(ExecutionContext& execution_context)
+    : Supplement<ExecutionContext>(execution_context) {}
 
 NotificationManager::~NotificationManager() {}
 
-mojom::blink::PermissionStatus NotificationManager::GetPermissionStatus(
-    ExecutionContext* execution_context) {
-  if (!notification_service_) {
-    Platform::Current()->GetInterfaceProvider()->GetInterface(
-        mojo::MakeRequest(&notification_service_));
-  }
+mojom::blink::PermissionStatus NotificationManager::GetPermissionStatus() {
+  if (GetSupplementable()->IsContextDestroyed())
+    return mojom::blink::PermissionStatus::DENIED;
 
   mojom::blink::PermissionStatus permission_status;
-  if (!notification_service_->GetPermissionStatus(
-          execution_context->GetSecurityOrigin()->ToString(),
-          &permission_status)) {
+  if (!GetNotificationService()->GetPermissionStatus(&permission_status)) {
     NOTREACHED();
     return mojom::blink::PermissionStatus::DENIED;
   }
@@ -104,10 +101,29 @@ void NotificationManager::OnPermissionRequestComplete(
   resolver->Resolve(status_string);
 }
 
+void NotificationManager::OnNotificationServiceConnectionError() {
+  notification_service_.reset();
+}
+
 void NotificationManager::OnPermissionServiceConnectionError() {
   permission_service_.reset();
 }
 
+const mojom::blink::NotificationServicePtr&
+NotificationManager::GetNotificationService() {
+  if (!notification_service_) {
+    if (auto* provider = GetSupplementable()->GetInterfaceProvider()) {
+      provider->GetInterface(mojo::MakeRequest(&notification_service_));
+
+      notification_service_.set_connection_error_handler(ConvertToBaseCallback(
+          WTF::Bind(&NotificationManager::OnNotificationServiceConnectionError,
+                    WrapWeakPersistent(this))));
+    }
+  }
+
+  return notification_service_;
+}
+
 void NotificationManager::Trace(blink::Visitor* visitor) {
   Supplement<ExecutionContext>::Trace(visitor);
 }

third_party/WebKit/Source/modules/notifications/NotificationManager.h

@@ -36,7 +36,7 @@ class NotificationManager final
 
   // Returns the notification permission status of the current origin. This
   // method is synchronous to support the Notification.permission getter.
-  mojom::blink::PermissionStatus GetPermissionStatus(ExecutionContext*);
+  mojom::blink::PermissionStatus GetPermissionStatus();
 
   ScriptPromise RequestPermission(
       ScriptState*,
@@ -45,11 +45,17 @@ class NotificationManager final
   virtual void Trace(blink::Visitor*);
 
  private:
-  NotificationManager();
+  explicit NotificationManager(ExecutionContext&);
+
+  // Returns an initialized NotificationServicePtr. A connection will be
+  // established the first time this method is called.
+  const mojom::blink::NotificationServicePtr& GetNotificationService();
 
   void OnPermissionRequestComplete(ScriptPromiseResolver*,
                                    NotificationPermissionCallback*,
                                    mojom::blink::PermissionStatus);
+
+  void OnNotificationServiceConnectionError();
   void OnPermissionServiceConnectionError();
 
   mojom::blink::NotificationServicePtr notification_service_;

third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp

@@ -79,8 +79,7 @@ ScriptPromise ServiceWorkerRegistrationNotifications::showNotification(
 
   // If permission for notification's origin is not "granted", reject the
   // promise with a TypeError exception, and terminate these substeps.
-  if (NotificationManager::From(execution_context)
-          ->GetPermissionStatus(execution_context) !=
+  if (NotificationManager::From(execution_context)->GetPermissionStatus() !=
       mojom::blink::PermissionStatus::GRANTED)
     return ScriptPromise::Reject(
         script_state,

third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

@@ -9,7 +9,8 @@ import "third_party/WebKit/public/platform/modules/permissions/permission_status
 // Service through which Blink can request notifications to be shown, closed or
 // retrieved from the embedder.
 interface NotificationService {
-  // Synchronously retrieves the permission status for |origin|. Required to
-  // be synchronous due to the Notification.permission JavaScript getter.
-  [Sync] GetPermissionStatus(string origin) => (PermissionStatus status);
+  // Synchronously retrieves the permission status for the origin associated
+  // with the interface connection. Required to be synchronous due to the
+  // Notification.permission JavaScript getter.
+  [Sync] GetPermissionStatus() => (PermissionStatus status);
 };