Push API: Grace - allow one in ten pushes to show no notification.

For developers who have opted in to showing a user-visible UI change on
every push (in exchange for showing the user a less scary permission
prompt), we currently[1] enforce that each push shows a notification.

This patch allows one in ten pushes to show no notification, so
occasional bugs in developer push handlers don't needlessly spam the
user.

Specifically, it keeps track of whether the last (up to) 10 push
messages showed a notification (ignoring pushes didn't show a
notification but were exempt, e.g. due to https://crrev.com/866443003)
and only shows a forced notification if one of those (up to) 10 also
failed to show a notification.

[1]: (since https://crrev.com/842233003)

BUG=437277

Review URL: https://codereview.chromium.org/883743002

Cr-Commit-Position: refs/heads/master@{#314851}

chrome/browser/services/gcm/push_messaging_browsertest.cc

@@ -499,39 +499,34 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
       browser()->tab_strip_model()->GetActiveWebContents();
 
   // If the site is visible in an active tab, we should not force a notification
-  // to be shown.
+  // to be shown. Try it twice, since we allow one mistake per 10 push events.
   GCMClient::IncomingMessage message;
-  message.data["data"] = "testdata";
-  push_service()->OnMessage(app_id.ToString(), message);
-  ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result));
-  EXPECT_EQ("testdata", script_result);
-  ASSERT_EQ(0u, notification_manager()->GetNotificationCount());
+  for (int n = 0; n < 2; n++) {
+    message.data["data"] = "testdata";
+    push_service()->OnMessage(app_id.ToString(), message);
+    ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result));
+    EXPECT_EQ("testdata", script_result);
+    EXPECT_EQ(0u, notification_manager()->GetNotificationCount());
+  }
 
   // Open a blank foreground tab so site is no longer visible.
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), GURL("about:blank"), NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
 
-  // If the Service Worker push event handler shows a notification, we should
-  // not show a forced one.
-  message.data["data"] = "shownotification";
+  // If the Service Worker push event handler does not show a notification, we
+  // should show a forced one, but only on the 2nd occurrence since we allow one
+  // mistake per 10 push events.
+  message.data["data"] = "testdata";
   push_service()->OnMessage(app_id.ToString(), message);
   ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));
-  EXPECT_EQ("shownotification", script_result);
-  ASSERT_EQ(1u, notification_manager()->GetNotificationCount());
-  EXPECT_EQ(base::ASCIIToUTF16("push_test_tag"),
-            notification_manager()->GetNotificationAt(0).replace_id());
-
-  notification_manager()->CancelAll();
-  ASSERT_EQ(0u, notification_manager()->GetNotificationCount());
-
-  // However if the Service Worker push event handler does not show a
-  // notification, we should show a forced one.
+  EXPECT_EQ("testdata", script_result);
+  EXPECT_EQ(0u, notification_manager()->GetNotificationCount());
   message.data["data"] = "testdata";
   push_service()->OnMessage(app_id.ToString(), message);
   ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));
   EXPECT_EQ("testdata", script_result);
-  ASSERT_EQ(1u, notification_manager()->GetNotificationCount());
+  EXPECT_EQ(1u, notification_manager()->GetNotificationCount());
   EXPECT_EQ(base::ASCIIToUTF16(kPushMessagingForcedNotificationTag),
             notification_manager()->GetNotificationAt(0).replace_id());
 
@@ -541,7 +536,31 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   push_service()->OnMessage(app_id.ToString(), message);
   ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));
   EXPECT_EQ("shownotification", script_result);
-  ASSERT_EQ(2u, notification_manager()->GetNotificationCount());
+  EXPECT_EQ(2u, notification_manager()->GetNotificationCount());
+
+  notification_manager()->CancelAll();
+  EXPECT_EQ(0u, notification_manager()->GetNotificationCount());
+
+  // However if the Service Worker push event handler shows a notification, we
+  // should not show a forced one.
+  message.data["data"] = "shownotification";
+  for (int n = 0; n < 9; n++) {
+    push_service()->OnMessage(app_id.ToString(), message);
+    ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));
+    EXPECT_EQ("shownotification", script_result);
+    EXPECT_EQ(1u, notification_manager()->GetNotificationCount());
+    EXPECT_EQ(base::ASCIIToUTF16("push_test_tag"),
+              notification_manager()->GetNotificationAt(0).replace_id());
+    notification_manager()->CancelAll();
+  }
+
+  // Now that 10 push messages in a row have shown notifications, we should
+  // allow the next one to mistakenly not show a notification.
+  message.data["data"] = "testdata";
+  push_service()->OnMessage(app_id.ToString(), message);
+  ASSERT_TRUE(RunScript("resultQueue.pop()", &script_result, web_contents));
+  EXPECT_EQ("testdata", script_result);
+  EXPECT_EQ(0u, notification_manager()->GetNotificationCount());
 }
 #endif
 

chrome/browser/services/gcm/push_messaging_service_impl.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/services/gcm/push_messaging_service_impl.h"
 
+#include <bitset>
 #include <vector>
 
 #include "base/bind.h"
@@ -29,6 +30,8 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_switches.h"
@@ -250,78 +253,136 @@ void PushMessagingServiceImpl::RequireUserVisibleUX(
   // is supported.
   int notification_count = notification_service->GetNotificationUIManager()->
       GetAllIdsByProfileAndSourceOrigin(profile_, application_id.origin).size();
-  if (notification_count > 0)
-    return;
-
-  // Sites with a currently visible tab don't need to show notifications.
+  // TODO(johnme): Hiding an existing notification should also count as a useful
+  // user-visible action done in response to a push message - but make sure that
+  // sending two messages in rapid succession which show then hide a
+  // notification doesn't count.
+  bool notification_shown = notification_count > 0;
+
+  bool notification_needed = true;
+  if (!notification_shown) {
+    // Sites with a currently visible tab don't need to show notifications.
 #if defined(OS_ANDROID)
-  for (auto it = TabModelList::begin(); it != TabModelList::end(); ++it) {
-    Profile* profile = (*it)->GetProfile();
-    content::WebContents* active_web_contents =
-        (*it)->GetActiveWebContents();
+    for (auto it = TabModelList::begin(); it != TabModelList::end(); ++it) {
+      Profile* profile = (*it)->GetProfile();
+      content::WebContents* active_web_contents =
+          (*it)->GetActiveWebContents();
 #else
-  for (chrome::BrowserIterator it; !it.done(); it.Next()) {
-    Profile* profile = it->profile();
-    content::WebContents* active_web_contents =
-        it->tab_strip_model()->GetActiveWebContents();
+    for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+      Profile* profile = it->profile();
+      content::WebContents* active_web_contents =
+          it->tab_strip_model()->GetActiveWebContents();
 #endif
-    if (!active_web_contents)
-      continue;
-
-    // Don't leak information from other profiles.
-    if (profile != profile_)
-      continue;
-
-    // Ignore minimized windows etc.
-    switch (active_web_contents->GetMainFrame()->GetVisibilityState()) {
-     case blink::WebPageVisibilityStateHidden:
-     case blink::WebPageVisibilityStatePrerender:
-      continue;
-     case blink::WebPageVisibilityStateVisible:
-      break;
+      if (!active_web_contents)
+        continue;
+
+      // Don't leak information from other profiles.
+      if (profile != profile_)
+        continue;
+
+      // Ignore minimized windows etc.
+      switch (active_web_contents->GetMainFrame()->GetVisibilityState()) {
+       case blink::WebPageVisibilityStateHidden:
+       case blink::WebPageVisibilityStatePrerender:
+        continue;
+       case blink::WebPageVisibilityStateVisible:
+        break;
+      }
+
+      // Use the visible URL since that's the one the user is aware of (and it
+      // doesn't matter whether the page loaded successfully).
+      const GURL& active_url = active_web_contents->GetVisibleURL();
+
+      // Allow https://foo.example.com Service Worker to not show notification
+      // if an https://bar.example.com tab is visible (and hence might
+      // conceivably be showing UI in response to the push message); but http://
+      // doesn't count as the Service Worker can't talk to it, even with
+      // navigator.connect.
+      if (application_id.origin.scheme() != active_url.scheme())
+        continue;
+      if (net::registry_controlled_domains::SameDomainOrHost(
+          application_id.origin, active_url,
+          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
+        notification_needed = false;
+        break;
+      }
     }
+  }
 
-    // Use the visible URL since that's the one the user is aware of (and it
-    // doesn't matter whether the page loaded successfully).
-    const GURL& active_url = active_web_contents->GetVisibleURL();
-
-    // Allow https://foo.example.com Service Worker to not show notification if
-    // an https://bar.example.com tab is visible (and hence might conceivably
-    // be showing UI in response to the push message); but http:// doesn't count
-    // as the Service Worker can't talk to it, even with navigator.connect.
-    if (application_id.origin.scheme() != active_url.scheme())
-      continue;
-    if (net::registry_controlled_domains::SameDomainOrHost(
-        application_id.origin, active_url,
-        net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
-      return;
-    }
+  // Don't track push messages that didn't show a notification but were exempt
+  // from needing to do so.
+  if (notification_shown || notification_needed) {
+    content::ServiceWorkerContext* service_worker_context =
+        content::BrowserContext::GetStoragePartitionForSite(
+            profile_, application_id.origin)->GetServiceWorkerContext();
+
+    PushMessagingService::GetNotificationsShownByLastFewPushes(
+        service_worker_context, application_id.service_worker_registration_id,
+        base::Bind(&PushMessagingServiceImpl::DidGetNotificationsShown,
+                   weak_factory_.GetWeakPtr(),
+                   application_id, notification_shown, notification_needed));
   }
+#endif  // defined(ENABLE_NOTIFICATIONS)
+}
 
-  // If we haven't returned yet, the site failed to show a notification, so we
-  // will show a generic notification. See https://crbug.com/437277
-  // TODO(johnme): The generic notification should probably automatically close
-  // itself when the next push message arrives?
-  content::PlatformNotificationData notification_data;
-  // TODO(johnme): Switch to FormatOriginForDisplay from crbug.com/402698
-  notification_data.title = l10n_util::GetStringFUTF16(
-      IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_TITLE,
-      base::UTF8ToUTF16(application_id.origin.host()));
-  notification_data.direction =
-      content::PlatformNotificationData::NotificationDirectionLeftToRight;
-  notification_data.body =
-      l10n_util::GetStringUTF16(IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_BODY);
-  notification_data.tag =
-      base::ASCIIToUTF16(kPushMessagingForcedNotificationTag);
-  notification_data.icon = GURL();  // TODO(johnme): Better icon?
-  notification_service->DisplayPersistentNotification(
-      profile_,
-      application_id.service_worker_registration_id,
-      application_id.origin,
-      SkBitmap() /* icon */,
-      notification_data,
-      content::ChildProcessHost::kInvalidUniqueID /* render_process_id */);
-#endif
+static void IgnoreResult(bool unused) {
+}
+
+void PushMessagingServiceImpl::DidGetNotificationsShown(
+    const PushMessagingApplicationId& application_id,
+    bool notification_shown, bool notification_needed,
+    const std::string& data, bool success, bool not_found) {
+  content::ServiceWorkerContext* service_worker_context =
+      content::BrowserContext::GetStoragePartitionForSite(
+          profile_, application_id.origin)->GetServiceWorkerContext();
+
+  // We remember whether the last (up to) 10 pushes showed notifications.
+  const size_t MISSED_NOTIFICATIONS_LENGTH = 10;
+  // data is a string like "0001000", where '0' means shown, and '1' means
+  // needed but not shown. We manipulate it in bitset form.
+  std::bitset<MISSED_NOTIFICATIONS_LENGTH> missed_notifications(data);
+
+  bool needed_but_not_shown = notification_needed && !notification_shown;
+
+  // New entries go at the end, and old ones are shifted off the beginning once
+  // the history length is exceeded.
+  missed_notifications <<= 1;
+  missed_notifications[0] = needed_but_not_shown;
+  std::string updated_data(missed_notifications.
+      to_string<char, std::string::traits_type, std::string::allocator_type>());
+  PushMessagingService::SetNotificationsShownByLastFewPushes(
+      service_worker_context, application_id.service_worker_registration_id,
+      application_id.origin, updated_data,
+      base::Bind(&IgnoreResult));  // This is a heuristic; ignore failure.
+
+  if (needed_but_not_shown && missed_notifications.count() >= 2) {
+    // The site failed to show a notification when one was needed, and they have
+    // already failed once in the previous 10 push messages, so we will show a
+    // generic notification. See https://crbug.com/437277.
+    // TODO(johnme): The generic notification should probably automatically
+    // close itself when the next push message arrives?
+    content::PlatformNotificationData notification_data;
+    // TODO(johnme): Switch to FormatOriginForDisplay from crbug.com/402698
+    notification_data.title = l10n_util::GetStringFUTF16(
+        IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_TITLE,
+        base::UTF8ToUTF16(application_id.origin.host()));
+    notification_data.direction =
+        content::PlatformNotificationData::NotificationDirectionLeftToRight;
+    notification_data.body =
+        l10n_util::GetStringUTF16(IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_BODY);
+    notification_data.tag =
+        base::ASCIIToUTF16(kPushMessagingForcedNotificationTag);
+    notification_data.icon = GURL();  // TODO(johnme): Better icon?
+    PlatformNotificationServiceImpl* notification_service =
+        PlatformNotificationServiceImpl::GetInstance();
+    notification_service->DisplayPersistentNotification(
+        profile_,
+        application_id.service_worker_registration_id,
+        application_id.origin,
+        SkBitmap() /* icon */,
+        notification_data,
+        content::ChildProcessHost::kInvalidUniqueID /* render_process_id */);
+  }
 }
 
 void PushMessagingServiceImpl::OnMessagesDeleted(const std::string& app_id) {

chrome/browser/services/gcm/push_messaging_service_impl.h

@@ -87,6 +87,13 @@ class PushMessagingServiceImpl : public content::PushMessagingService,
   // happened in the background. When they forget to do so, display a default
   // notification on their behalf.
   void RequireUserVisibleUX(const PushMessagingApplicationId& application_id);
+  void DidGetNotificationsShown(
+      const PushMessagingApplicationId& application_id,
+      bool notification_shown,
+      bool notification_needed,
+      const std::string& data,
+      bool success,
+      bool not_found);
 
   void RegisterEnd(
       const content::PushMessagingService::RegisterCallback& callback,

content/content_browser.gypi

@@ -182,6 +182,7 @@
       'public/browser/presentation_service_delegate.h',
       'public/browser/profiler_controller.h',
       'public/browser/profiler_subscriber.h',
+      'public/browser/push_messaging_service.cc',
       'public/browser/push_messaging_service.h',
       'public/browser/quota_permission_context.h',
       'public/browser/readback_types.h',

content/public/browser/push_messaging_service.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/public/browser/push_messaging_service.h"
+
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace {
+
+const char kNotificationsShownServiceWorkerKey[] =
+    "notifications_shown_by_last_few_pushes";
+
+}  // namespace
+
+namespace content {
+
+static void CallGetNotificationsShownCallbackFromIO(
+    const PushMessagingService::GetNotificationsShownCallback& callback,
+    const std::string& data,
+    ServiceWorkerStatusCode service_worker_status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  bool success = service_worker_status == SERVICE_WORKER_OK;
+  bool not_found = service_worker_status == SERVICE_WORKER_ERROR_NOT_FOUND;
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(callback, data, success, not_found));
+}
+
+static void CallResultCallbackFromIO(
+    const ServiceWorkerContext::ResultCallback& callback,
+    ServiceWorkerStatusCode service_worker_status) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  bool success = service_worker_status == SERVICE_WORKER_OK;
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(callback, success));
+}
+
+static void GetNotificationsShownOnIO(
+    scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_wrapper,
+    int64 service_worker_registration_id,
+    const PushMessagingService::GetNotificationsShownCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  service_worker_context_wrapper->context()->storage()->GetUserData(
+      service_worker_registration_id, kNotificationsShownServiceWorkerKey,
+      base::Bind(&CallGetNotificationsShownCallbackFromIO, callback));
+}
+
+static void SetNotificationsShownOnIO(
+    scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_wrapper,
+    int64 service_worker_registration_id, const GURL& origin,
+    const std::string& data,
+    const PushMessagingService::ResultCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  service_worker_context_wrapper->context()->storage()->StoreUserData(
+      service_worker_registration_id, origin,
+      kNotificationsShownServiceWorkerKey, data,
+      base::Bind(&CallResultCallbackFromIO, callback));
+}
+
+// static
+void PushMessagingService::GetNotificationsShownByLastFewPushes(
+    ServiceWorkerContext* service_worker_context,
+    int64 service_worker_registration_id,
+    const GetNotificationsShownCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  scoped_refptr<ServiceWorkerContextWrapper> wrapper =
+      static_cast<ServiceWorkerContextWrapper*>(service_worker_context);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&GetNotificationsShownOnIO,
+                                     wrapper,
+                                     service_worker_registration_id,
+                                     callback));
+}
+
+// static
+void PushMessagingService::SetNotificationsShownByLastFewPushes(
+    ServiceWorkerContext* service_worker_context,
+    int64 service_worker_registration_id,
+    const GURL& origin,
+    const std::string& notifications_shown,
+    const ResultCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  scoped_refptr<ServiceWorkerContextWrapper> wrapper =
+      static_cast<ServiceWorkerContextWrapper*>(service_worker_context);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&SetNotificationsShownOnIO,
+                                     wrapper,
+                                     service_worker_registration_id,
+                                     origin,
+                                     notifications_shown,
+                                     callback));
+}
+
+}  // namespace content

content/public/browser/push_messaging_service.h

@@ -15,15 +15,38 @@
 
 namespace content {
 
+class ServiceWorkerContext;
+
 // A push service-agnostic interface that the Push API uses for talking to
 // push messaging services like GCM. Must only be used on the UI thread.
 class CONTENT_EXPORT PushMessagingService {
  public:
+  using GetNotificationsShownCallback =
+      base::Callback<void(const std::string& notifications_shown,
+                          bool success, bool not_found)>;
+
+  using ResultCallback = base::Callback<void(bool success)>;
+
   using RegisterCallback =
       base::Callback<void(const std::string& /* registration_id */,
                           PushRegistrationStatus /* status */)>;
   using UnregisterCallback = base::Callback<void(PushUnregistrationStatus)>;
 
+  // Provide a storage mechanism to read/write an opaque
+  // "notifications_shown_by_last_few_pushes" string associated with a Service
+  // Worker registration. Stored data is deleted when the associated
+  // registration is deleted.
+  static void GetNotificationsShownByLastFewPushes(
+      ServiceWorkerContext* service_worker_context,
+      int64 service_worker_registration_id,
+      const GetNotificationsShownCallback& callback);
+  static void SetNotificationsShownByLastFewPushes(
+      ServiceWorkerContext* service_worker_context,
+      int64 service_worker_registration_id,
+      const GURL& origin,
+      const std::string& notifications_shown,
+      const ResultCallback& callback);
+
   virtual ~PushMessagingService() {}
 
   // Returns the absolute URL exposed by the push server where the webapp server