Address a race condition in displaying notifications

Address a race condition in displaying notifications

When a developer displays a notification and then immediately gets the
displayed notifications, without waiting for the result to be
acknowledged, there's a chance that we end up in a race condition where
the PlatformNotificationService is still displaying the notification and
thus doesn't return it in the set of displayed ones yet.

This will, in turn, result in the notification being deleted from the
database. That will render clicking on the notification useless.

Instead, have a brief grace period in which we'll ignore the recently
displayed notification when synchronizing the database.

BUG=

[Peter Beverloo, 2017-05-10 14:44:49]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[Peter Beverloo, 2017-05-10 14:45:21]

peter@chromium.org changed reviewers:
+ awdf@chromium.org, miguelg@chromium.org

[Peter Beverloo, 2017-05-10 14:45:21]

+miguelg, awdf for review

I think this is reasonable enough. WDYT?

[commit-bot: I haz the power, 2017-05-10 14:45:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[awdf, 2017-05-10 14:55:50]

On 2017/05/10 14:45:21, Peter Beverloo wrote:
> +miguelg, awdf for review
> 
> I think this is reasonable enough. WDYT?

I will take a look, but just to clarify the bug, in the cl description what
exactly do you 
mean by 'without waiting for the result to be acknowledged'? The
notificationclick event?

[Peter Beverloo, 2017-05-10 15:10:14]

On 2017/05/10 14:55:50, awdf wrote:
> On 2017/05/10 14:45:21, Peter Beverloo wrote:
> > +miguelg, awdf for review
> > 
> > I think this is reasonable enough. WDYT?
> 
> I will take a look, but just to clarify the bug, in the cl description what
> exactly do you 
> mean by 'without waiting for the result to be acknowledged'? The
> notificationclick event?

serviceWorkerRegistration.showNotification(...)
  1. Loading resources, IPC to the browser process
  2. Write notification data to the database
  3. Request display from the PlatformNotificationService
  4. Request display from the underlying OS

(3) involves a thread jump, (4) is somewhat out of our control

serviceWorkerRegistration.getNotifications(...)
  1. IPC to the browser process
  2. Get displayed notifications from the PlatformNotificationService
  3. Read notifications from the database
  4. Delete notifications that were read by (3), but not included in (2)


Suppose that a developer displays a notification, and then calls
getNotifications() to verify that it has been displayed. In most cases we'd be
good, but there are cases where there's a little bit of delay. To name two
examples: we need to start and show through an XPC service on Mac, or, due to
congestion, there's a delay when using the Windows Action Center.

When that happens, step (2) of getNotifications() would *not* include the
notification that's still stuck somewhere in the OS' code, causing us to remove
the notification from the database. We then end up in a state where the
notification is being displayed and available to the user, but we lost track of
it when they try to click on it.

This CL means we ignore recently shown notifications to compensate for such
delays.

[commit-bot: I haz the power, 2017-05-10 15:40:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 15:40:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[awdf, 2017-05-10 17:01:58]

lgtm % fixing up the existing tests.

https://codereview.chromium.org/2878443002/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2878443002/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:569:
ASSERT_EQ(0u, notification_database_datas.size());
This line is now failing - see
https://00e9e64bacafae9f30f2cf269632054d540ffb500e728c90aa-apidata.googleuser...


Guess you'll need a test clock in this test too to fake time fast-forwarding.

[Peter Beverloo, 2017-05-10 17:09:04]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[Peter Beverloo, 2017-05-10 17:09:22]

Thanks!

https://codereview.chromium.org/2878443002/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2878443002/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:569:
ASSERT_EQ(0u, notification_database_datas.size());
On 2017/05/10 17:01:57, awdf wrote:
> This line is now failing - see
>
https://00e9e64bacafae9f30f2cf269632054d540ffb500e728c90aa-apidata.googleuser...
> 
> 
> Guess you'll need a test clock in this test too to fake time fast-forwarding.

Done.

[commit-bot: I haz the power, 2017-05-10 17:10:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miguelg, 2017-05-10 17:38:34]

miguelg@google.com changed reviewers:
+ miguelg@google.com

[miguelg, 2017-05-10 17:38:35]

lgtm

[commit-bot: I haz the power, 2017-05-10 18:57:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 18:57:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[awdf, 2017-05-11 10:38:53]

https://codereview.chromium.org/2878443002/diff/20001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/2878443002/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:420: void
PlatformNotificationContextImpl::DeleteNotificationData(
Just to check my understanding - this DeleteNotificationData method should get
called for every notification, right?

[Miguel Garcia, 2017-05-11 12:16:58]

lgtm