Push API: Validate storage before returning cached subscriptions

Push API: Validate storage before returning cached subscriptions

For desktop users whose GCM Store was reset before
93ec793ac69a542b2213297737178a55d069fd0d
(https://codereview.chromium.org/2473813002), or whose GCM Store was
reset after that patch but who encountered a race condition (e.g. around
browser shutdown) that prevented PushMessagingServiceImpl::OnStoreReset
from deleting affected subscriptions, it was possible to get into a
nasty state where PushManager.getSubscription() and
PushManager.subscribe() would both keep returning the old push
subscriptions cached by the Service Worker database, even though they
were no longer valid (resetting the GCM Store changed Chrome desktop's
device ID, so it's impossible for any messages to be delivered to the
old subscriptions).

It was also possible for the PushMessagingAppIdentifier map to get out
of sync with the Service Worker database (on both Android and desktop).

This patch makes PushManager.getSubscription() & PushManager.subscribe()
both validate that subscriptions are properly stored across all four
databases that could get out of sync:
- the Service Worker database
- the PushMessagingAppIdentifier map
- the GCM Store (on desktop only)
- the GCMKeyStore
(they don't check against the GCM server, since they must work offline)
and if an inconsistency is detected, it will be automatically resolved
by unsubscribing the invalid subscription (then re-subscribing, in the
case of PushManager.subscribe()).

BUG=661660
Review-Url: https://codereview.chromium.org/2697793004
Cr-Commit-Position: refs/heads/master@{#461501}
Committed: https://chromium.googlesource.com/chromium/src/+/6576ecf83ab56518daa1274dfa354aee84c8696b

[johnme, 2017-02-15 15:06:26]

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

[Peter Beverloo, 2017-03-20 23:50:14]

This is cool and good, thanks :)

lgtm % comments

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.h (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.h:47: static
PushMessagingAppIdentifier LegacyGenerateForTesting(
It's unfortunate that this has to be public. Can we perhaps make it private w/
the necessary friend declarations?

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:117: if (out_result)
if -> DCHECK, it's always given

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:632: //
---------------------------------------------------
nit: dito

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:175: //
--------------------------------------------------
nit: did clang-format do this? It's now inconsistent with the rest. Also plural.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1027: if (cached_gcm_registration_info
&&
nit: don't have to be nice for |!cached_gcm_registration_info|

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:121: bool ValidateRegistration(
micro nit: ordering (one line up)

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:81: static const size_t kMaxSenders;
It seems to me like making this protected would be sufficient visibility, given
that the only user is in the GCMDriverDesktop?

Also, ++docs and you could move the value in the header file as well by making
it a `constexpr size_t kMaxSenders = 100`

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:126: // gcm_driver doesn't store
registration IDs on Android, so assume it's valid.
Is there something we could do here long-term, like check with GMScore?

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:589: // callback (let it hang
indefinitely).
I guess this is OK from a lifetime point of view because |callback| will go out
of scope, causing the bind state to be released. A bit unfortunate, but I don't
think it's worth creating a tri-state for this because this should be
exceptional.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:160: callback.Run(false /*
is_valid */);
nit: this potentially is a synchronous return path. Should we post a task
instead?

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:409:
DCHECK(push_service);  // NOT_FOUND response can only come from service.
Core::GetSubscriptionInfoOnUI specifically returns is_valid==false for cases
where there isn't a Push Service. That seems to be in conflict with this
comment?

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:421:
SERVICE_WORKER_ERROR_NOT_FOUND))));
This is deep. Like, literally. Can we maybe make it slightly less deep by using
anonymous functions to bridge?

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:985:
PushMessagingMessageFilter::Core::GetWeakPtrFromIOParentConstructor() {
Would you mind summarising in two sentences or so why it's important to have
this method and cache the WeakPtr?

[johnme, 2017-03-30 18:36:38]

Addressed review comments - thanks!

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_app_identifier.h (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_app_identifier.h:47: static
PushMessagingAppIdentifier LegacyGenerateForTesting(
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> It's unfortunate that this has to be public. Can we perhaps make it private w/
> the necessary friend declarations?

Done.

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_browsertest.cc:117: if (out_result)
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> if -> DCHECK, it's always given

Done.

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:632: //
---------------------------------------------------
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> nit: dito

Done.

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:175: //
--------------------------------------------------
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> nit: did clang-format do this? It's now inconsistent with the rest. Also
plural.

Done.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1027: if (cached_gcm_registration_info
&&
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> nit: don't have to be nice for |!cached_gcm_registration_info|

M'ok. In that case I'll also remove the niceness from GCMClientImpl::Register
(where I copied it from).

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.h:121: bool ValidateRegistration(
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> micro nit: ordering (one line up)

Done.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:81: static const size_t kMaxSenders;
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> It seems to me like making this protected would be sufficient visibility,
given
> that the only user is in the GCMDriverDesktop?
> 
> Also, ++docs and you could move the value in the header file as well by making
> it a `constexpr size_t kMaxSenders = 100`

Doneish. I kept it public as I feel the limit is part of the public interface.
And I switched to `constexpr static` instead.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:126: // gcm_driver doesn't store
registration IDs on Android, so assume it's valid.
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> Is there something we could do here long-term, like check with GMScore?

No, GMSCore has a cache, but it expires, and there's no way to query it without
registering. More importantly, even if our stored registration is out of sync
with GMSCore's cache, GMSCore will happily forward messages that are sent to our
stored registration - i.e. unlike GCMClientImpl, GMSCore doesn't require that
incoming messages match its stored registrations.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:589: // callback (let it hang
indefinitely).
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> I guess this is OK from a lifetime point of view because |callback| will go
out
> of scope, causing the bind state to be released. A bit unfortunate, but I
don't
> think it's worth creating a tri-state for this because this should be
> exceptional.

Acknowledged.

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:160: callback.Run(false /*
is_valid */);
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> nit: this potentially is a synchronous return path. Should we post a task
> instead?

The convention in this file seems to be for synchronous running of callbacks. I
agree that it's a little odd. How about I change them all in a follow-up patch?

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:409:
DCHECK(push_service);  // NOT_FOUND response can only come from service.
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> Core::GetSubscriptionInfoOnUI specifically returns is_valid==false for cases
> where there isn't a Push Service. That seems to be in conflict with this
> comment?

Hmm yeah. In practice it shouldn't be possible to have a stored push
subscription in a profile that doesn't have a push service though. I kept the
DCHECK (well, NOTREACHED), but added a fallback for release builds that returns
PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE, in case some refactoring or
edge case (kill switch?) makes that stop being true.

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:421:
SERVICE_WORKER_ERROR_NOT_FOUND))));
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> This is deep. Like, literally. Can we maybe make it slightly less deep by
using
> anonymous functions to bridge?

Done.

https://codereview.chromium.org/2697793004/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:985:
PushMessagingMessageFilter::Core::GetWeakPtrFromIOParentConstructor() {
On 2017/03/20 23:50:13, Peter Beverloo wrote:
> Would you mind summarising in two sentences or so why it's important to have
> this method and cache the WeakPtr?

weak_factory_ui_to_ui_ should not be public, as it's only safe to use on the UI
thread, and the outer class lives on the IO thread. The constructor of the outer
class exceptionally runs on the UI thread, so can safely grab a weak ptr, hence
this public wrapper method that DCHECKs it's called on UI thread.

I added the following comment to ui_core_weak_ptr_ which hopefully the purpose
clearer:
  // Can be used on the IO thread as the |this| parameter when binding a
  // callback that will be called on the UI thread (an IO -> UI -> UI chain).

And clarified the comment on GetWeakPtrFromIOParentConstructor:
  // Returns a weak ptr. Must only be called on the UI thread (and hence can
  // only be called from the outer class's constructor).

[johnme, 2017-03-30 18:42:49]

johnme@chromium.org changed reviewers:
+ isherman@chromium.org, nasko@chromium.org

[johnme, 2017-03-30 18:42:50]

nasko@chromium.org: Please review changes in content/common/ and content/public/

isherman@chromium.org: Please review changes in
tools/metrics/histograms/histograms.xml

Thanks!

[Ilya Sherman, 2017-03-30 21:58:32]

histograms.xml lgtm

[nasko, 2017-03-31 21:48:43]

https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_manager.h (right):

https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.h:122: // Can be used on
the IO thread as the |this| parameter when binding a
nit: Empty line above the comment.

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
File content/common/push_messaging_param_traits.cc (left):

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
content/common/push_messaging_param_traits.cc:191: "PushGetRegistrationStatus
enums must match, PUBLIC_KEY_UNAVAILABLE");
Why was this enum value removed? It is still present in
content/public/common/push_messaging_status.h.

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
File content/common/push_messaging_param_traits.cc (right):

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
content/common/push_messaging_param_traits.cc:101: "PushRegistrationStatus enums
must match, STORAGE_CORRUPT");
Please keep the ordering of these the same as the order in the header file
(content/public/common/push_messaging_status.h).

[johnme, 2017-04-03 13:54:28]

Addressed nasko's review comments - thanks :)

https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_manager.h (right):

https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.h:122: // Can be used on
the IO thread as the |this| parameter when binding a
On 2017/03/31 21:48:43, nasko wrote:
> nit: Empty line above the comment.

Done.

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
File content/common/push_messaging_param_traits.cc (left):

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
content/common/push_messaging_param_traits.cc:191: "PushGetRegistrationStatus
enums must match, PUBLIC_KEY_UNAVAILABLE");
On 2017/03/31 21:48:43, nasko wrote:
> Why was this enum value removed? It is still present in
> content/public/common/push_messaging_status.h.

No, the patch comments out PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE in
both content/public/common/push_messaging_status.h and
content/common/push_messaging.mojom (perhaps you're thinking of
PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE which is still present in both
places?)

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
File content/common/push_messaging_param_traits.cc (right):

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
content/common/push_messaging_param_traits.cc:101: "PushRegistrationStatus enums
must match, STORAGE_CORRUPT");
On 2017/03/31 21:48:43, nasko wrote:
> Please keep the ordering of these the same as the order in the header file
> (content/public/common/push_messaging_status.h).

Oops, done.

[nasko, 2017-04-03 14:10:38]

LGTM

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
File content/common/push_messaging_param_traits.cc (left):

https://codereview.chromium.org/2697793004/diff/60001/content/common/push_mes...
content/common/push_messaging_param_traits.cc:191: "PushGetRegistrationStatus
enums must match, PUBLIC_KEY_UNAVAILABLE");
On 2017/04/03 13:54:27, johnme wrote:
> On 2017/03/31 21:48:43, nasko wrote:
> > Why was this enum value removed? It is still present in
> > content/public/common/push_messaging_status.h.
> 
> No, the patch comments out PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE
in
> both content/public/common/push_messaging_status.h and
> content/common/push_messaging.mojom (perhaps you're thinking of
> PUSH_REGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE which is still present in both
> places?)

I meant that specific one, however the comment at the beginning of the line
escaped me. Thanks for clarifying.

[johnme, 2017-04-03 14:18:29]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-03 14:18:29]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2697793004/#ps80001
(title: "Address nasko's review comments")

[commit-bot: I haz the power, 2017-04-03 14:18:50]

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

[commit-bot: I haz the power, 2017-04-03 14:45:32]

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

[commit-bot: I haz the power, 2017-04-03 14:45:33]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[johnme, 2017-04-03 17:00:44]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-04-03 17:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
isherman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2697793004/#ps100001
(title: "Fix include")

[commit-bot: I haz the power, 2017-04-03 17:01:31]

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

[commit-bot: I haz the power, 2017-04-03 19:26:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491238844738680,
"parent_rev": "0958b7f56e08e90312288a938205dc33fd69d56b", "commit_rev":
"6576ecf83ab56518daa1274dfa354aee84c8696b"}

[commit-bot: I haz the power, 2017-04-03 19:27:37]

Description was changed from

==========
Push API: Validate storage before returning cached subscriptions

For desktop users whose GCM Store was reset before
93ec793ac69a542b2213297737178a55d069fd0d
(https://codereview.chromium.org/2473813002), or whose GCM Store was
reset after that patch but who encountered a race condition (e.g. around
browser shutdown) that prevented PushMessagingServiceImpl::OnStoreReset
from deleting affected subscriptions, it was possible to get into a
nasty state where PushManager.getSubscription() and
PushManager.subscribe() would both keep returning the old push
subscriptions cached by the Service Worker database, even though they
were no longer valid (resetting the GCM Store changed Chrome desktop's
device ID, so it's impossible for any messages to be delivered to the
old subscriptions).

It was also possible for the PushMessagingAppIdentifier map to get out
of sync with the Service Worker database (on both Android and desktop).

This patch makes PushManager.getSubscription() & PushManager.subscribe()
both validate that subscriptions are properly stored across all four
databases that could get out of sync:
- the Service Worker database
- the PushMessagingAppIdentifier map
- the GCM Store (on desktop only)
- the GCMKeyStore
(they don't check against the GCM server, since they must work offline)
and if an inconsistency is detected, it will be automatically resolved
by unsubscribing the invalid subscription (then re-subscribing, in the
case of PushManager.subscribe()).

BUG=661660
==========

to

==========
Push API: Validate storage before returning cached subscriptions

For desktop users whose GCM Store was reset before
93ec793ac69a542b2213297737178a55d069fd0d
(https://codereview.chromium.org/2473813002), or whose GCM Store was
reset after that patch but who encountered a race condition (e.g. around
browser shutdown) that prevented PushMessagingServiceImpl::OnStoreReset
from deleting affected subscriptions, it was possible to get into a
nasty state where PushManager.getSubscription() and
PushManager.subscribe() would both keep returning the old push
subscriptions cached by the Service Worker database, even though they
were no longer valid (resetting the GCM Store changed Chrome desktop's
device ID, so it's impossible for any messages to be delivered to the
old subscriptions).

It was also possible for the PushMessagingAppIdentifier map to get out
of sync with the Service Worker database (on both Android and desktop).

This patch makes PushManager.getSubscription() & PushManager.subscribe()
both validate that subscriptions are properly stored across all four
databases that could get out of sync:
- the Service Worker database
- the PushMessagingAppIdentifier map
- the GCM Store (on desktop only)
- the GCMKeyStore
(they don't check against the GCM server, since they must work offline)
and if an inconsistency is detected, it will be automatically resolved
by unsubscribing the invalid subscription (then re-subscribing, in the
case of PushManager.subscribe()).

BUG=661660

Review-Url: https://codereview.chromium.org/2697793004
Cr-Commit-Position: refs/heads/master@{#461501}
Committed:
https://chromium.googlesource.com/chromium/src/+/6576ecf83ab56518daa1274dfa35...
==========

[commit-bot: I haz the power, 2017-04-03 19:27:39]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6576ecf83ab56518daa1274dfa35...