Implement getting notifications up to the browser process.

Implement getting notifications up to the browser process.

This CL implements the ability to get notifications for a given Service
Worker up to the point where it hits the browser process, where there's
a NOTIMPLEMENTED() waiting for now. The browser process then replies
with an empty vector of open notifications.

The second part of the implementation depends on the Notification
Database, which is still an in-progress work.

This CL is part of a three-sided patch:
  [1] https://codereview.chromium.org/1018613003/
  [2] This CL.
  [3] https://codereview.chromium.org/1016843002/

BUG=442143
Committed: https://crrev.com/f6dd3c3320e44b29ac283f6ce9d41018cd8eed26
Cr-Commit-Position: refs/heads/master@{#321564}

[Peter Beverloo, 2015-03-17 17:41:34]

peter@chromium.org changed reviewers:
+ johnme@chromium.org, mkwst@chromium.org

[Peter Beverloo, 2015-03-17 17:41:35]

+johnme for notifications
+mkwst for IPC

The IPC fuzzer is able to work with both std::pair and std::vector. This seems
nicer than adding another struct.

[Mike West, 2015-03-17 20:13:26]

mkwst@chromium.org changed reviewers:
+ tsepez@chromium.org

[Mike West, 2015-03-17 20:13:27]

Sorry, I'm not going to be able to look at this in detail tonight. Perhaps
tsepez@ can take a look at the IPC change during PST daytime? If not, I'll look
tomorrow.

[Tom Sepez, 2015-03-17 21:32:39]

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
File content/common/platform_notification_messages.h (right):

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:39: using
PersistentNotificationInfo =
This will have to move to an include guarded section of the file to prevent the
compiler seeing it multiple times as code is generated. See something like
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/geo...
for an example of how to structure this section.

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:56: // Informs the renderer
about a vector of available notifications in response
nit: this comment might indicate 
"reply to PlatformNotificationHostMsg_GetNotifications" if that's the case.

[johnme, 2015-03-18 15:03:06]

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

https://codereview.chromium.org/1014073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:132:
NOTIMPLEMENTED();
Depending on the NOTIMPLEMENTED_POLICY, this might fail at compile or runtime
when trying to run
LayoutTests/http/tests/notifications/serviceworkerregistration-get-empty.html?
Perhaps just put a DLOG here?

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:127: GURL origin =
GURL(service_worker_registration_impl->scope()).GetOrigin();
Nit: it seems slightly inconsistent that showPersistent has the origin passed to
it explicitly, whereas getNotifications extracts it from the WebSWR. Though
neither seems unreasonable...

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:134:
notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
GenerateNotificationId could be made to overflow. It seems that might enable a
malicious webapp to receive the results of a non-same-origin website that shares
the renderer process and has run getNotifications?

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.h:60: // TODO(peter): Remove
this method when Blink has been updated.
<offtopic> Is this being removed by some other CL? </offtopic>

[Peter Beverloo, 2015-03-19 15:46:44]

Thanks for the reviews!

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

https://codereview.chromium.org/1014073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:132:
NOTIMPLEMENTED();
On 2015/03/18 15:03:06, johnme wrote:
> Depending on the NOTIMPLEMENTED_POLICY, this might fail at compile or runtime
> when trying to run
> LayoutTests/http/tests/notifications/serviceworkerregistration-get-empty.html?
> Perhaps just put a DLOG here?

Good point. I removed the notice entirely, since I'm probably the only one who
cares anyway.

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:127: GURL origin =
GURL(service_worker_registration_impl->scope()).GetOrigin();
On 2015/03/18 15:03:06, johnme wrote:
> Nit: it seems slightly inconsistent that showPersistent has the origin passed
to
> it explicitly, whereas getNotifications extracts it from the WebSWR. Though
> neither seems unreasonable...

Extracting it from the SWR is the favorable option, and I'll eventually update
showPersistent() to match this.

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:134:
notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
On 2015/03/18 15:03:06, johnme wrote:
> GenerateNotificationId could be made to overflow. It seems that might enable a
> malicious webapp to receive the results of a non-same-origin website that
shares
> the renderer process and has run getNotifications?

I added a CHECK_GE in the NotificationDispatcher. Does this alleviate your
concern?

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.h (right):

https://codereview.chromium.org/1014073002/diff/1/content/child/notifications...
content/child/notifications/notification_manager.h:60: // TODO(peter): Remove
this method when Blink has been updated.
On 2015/03/18 15:03:06, johnme wrote:
> <offtopic> Is this being removed by some other CL? </offtopic>

One that landed two days ago :-).

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
File content/common/platform_notification_messages.h (right):

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:39: using
PersistentNotificationInfo =
On 2015/03/17 21:32:38, Tom Sepez wrote:
> This will have to move to an include guarded section of the file to prevent
the
> compiler seeing it multiple times as code is generated. See something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/geo...
> for an example of how to structure this section.

Done.

https://codereview.chromium.org/1014073002/diff/1/content/common/platform_not...
content/common/platform_notification_messages.h:56: // Informs the renderer
about a vector of available notifications in response
On 2015/03/17 21:32:38, Tom Sepez wrote:
> nit: this comment might indicate 
> "reply to PlatformNotificationHostMsg_GetNotifications" if that's the case.

Done.

[johnme, 2015-03-19 16:26:59]

lgtm, thanks

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
File content/child/notifications/notification_dispatcher.cc (right):

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
content/child/notifications/notification_dispatcher.cc:20:
CHECK_GE(next_notification_id_, 0);
If a legit website polls at 60 FPS for just over 1 year, it might overflow this.
But I guess at least it would only crash the renderer.

[Tom Sepez, 2015-03-19 18:10:18]

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
File content/child/notifications/notification_dispatcher.cc (right):

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
content/child/notifications/notification_dispatcher.cc:20:
CHECK_GE(next_notification_id_, 0);
Just wondering why this is a signed type in the first place?

[Tom Sepez, 2015-03-19 18:10:47]

LGTM otherwise.

[Peter Beverloo, 2015-03-19 18:17:00]

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
File content/child/notifications/notification_dispatcher.cc (right):

https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
content/child/notifications/notification_dispatcher.cc:20:
CHECK_GE(next_notification_id_, 0);
On 2015/03/19 18:10:18, Tom Sepez wrote:
> Just wondering why this is a signed type in the first place?

I initially introduced it as such for consistency with other features, which
already did what I'm introducing with this patch - the key type for base's IDMap
(base/id_map.h) is an int32, so changing this to either uint32_t or int64_t
would cause problems there.

Perhaps we should consider making an IDMap's KeyType an optional template
argument?

[Tom Sepez, 2015-03-19 20:08:26]

On 2015/03/19 18:17:00, Peter Beverloo wrote:
>
https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
> File content/child/notifications/notification_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
> content/child/notifications/notification_dispatcher.cc:20:
> CHECK_GE(next_notification_id_, 0);
> On 2015/03/19 18:10:18, Tom Sepez wrote:
> > Just wondering why this is a signed type in the first place?
> 
> I initially introduced it as such for consistency with other features, which
> already did what I'm introducing with this patch - the key type for base's
IDMap
> (base/id_map.h) is an int32, so changing this to either uint32_t or int64_t
> would cause problems there.
> 
> Perhaps we should consider making an IDMap's KeyType an optional template
> argument?
Ok, makes sense to keep it as a signed type.  This is only ever incremented,
right, so you're testing for a rollover? If so, then you should know the signed
overflow is undefined behavior, and the compiler may remove the check entirely
(thought I doubt it's smart enough to prove that its only incremented).  Or is
the being passed over the IPC channel?

[Peter Beverloo, 2015-03-19 20:11:30]

On 2015/03/19 20:08:26, Tom Sepez wrote:
> On 2015/03/19 18:17:00, Peter Beverloo wrote:
> >
>
https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
> > File content/child/notifications/notification_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/1014073002/diff/20001/content/child/notificat...
> > content/child/notifications/notification_dispatcher.cc:20:
> > CHECK_GE(next_notification_id_, 0);
> > On 2015/03/19 18:10:18, Tom Sepez wrote:
> > > Just wondering why this is a signed type in the first place?
> > 
> > I initially introduced it as such for consistency with other features, which
> > already did what I'm introducing with this patch - the key type for base's
> IDMap
> > (base/id_map.h) is an int32, so changing this to either uint32_t or int64_t
> > would cause problems there.
> > 
> > Perhaps we should consider making an IDMap's KeyType an optional template
> > argument?
> Ok, makes sense to keep it as a signed type.  This is only ever incremented,
> right, so you're testing for a rollover? If so, then you should know the
signed
> overflow is undefined behavior, and the compiler may remove the check entirely
> (thought I doubt it's smart enough to prove that its only incremented).  Or is
> the being passed over the IPC channel?

That's a good point! I guess a CHECK_LT on std::numeric_limits<int>::max() would
be more sensible?

Yes, the request_id is being passed over IPC, but the browser process will not
do anything with this information (it will pass it back to the renderer as-is).

[Peter Beverloo, 2015-03-20 12:32:23]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-20 12:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1014073002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-03-20 12:32:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1014073002/40001

[commit-bot: I haz the power, 2015-03-20 13:22:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-20 13:23:25]

Patchset 3 (id:??) landed as
https://crrev.com/f6dd3c3320e44b29ac283f6ce9d41018cd8eed26
Cr-Commit-Position: refs/heads/master@{#321564}