Chromium Code Reviews
Help | Chromium Project | Sign in
(15)

Issue 2697793004: Push API: Validate storage before returning cached subscriptions

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by johnme
Modified:
1 week, 2 days ago
Reviewers:
Peter Beverloo
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, zea+watch_chromium.org, Peter Beverloo, johnme+watch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, einbinder+watch-test-runner_chromium.org, harkness+watch_chromium.org, jochen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Comment out PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+769 lines, -203 lines) Patch
M chrome/browser/push_messaging/push_messaging_app_identifier.h View 2 chunks +10 lines, -0 lines 1 comment Download
M chrome/browser/push_messaging/push_messaging_app_identifier.cc View 1 chunk +22 lines, -3 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 7 chunks +130 lines, -2 lines 1 comment Download
M chrome/browser/push_messaging/push_messaging_service_impl.h View 2 chunks +15 lines, -7 lines 1 comment Download
M chrome/browser/push_messaging/push_messaging_service_impl.cc View 2 chunks +42 lines, -11 lines 1 comment Download
M chrome/test/data/push_messaging/push_test.js View 1 chunk +2 lines, -5 lines 0 comments Download
M components/gcm_driver/crypto/gcm_encryption_provider.h View 1 chunk +3 lines, -2 lines 0 comments Download
M components/gcm_driver/fake_gcm_client.h View 1 chunk +3 lines, -0 lines 0 comments Download
M components/gcm_driver/fake_gcm_client.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M components/gcm_driver/fake_gcm_driver.h View 1 chunk +5 lines, -0 lines 0 comments Download
M components/gcm_driver/fake_gcm_driver.cc View 2 chunks +11 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_client.h View 1 chunk +7 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_client_impl.h View 1 chunk +3 lines, -0 lines 1 comment Download
M components/gcm_driver/gcm_client_impl.cc View 1 chunk +35 lines, -0 lines 1 comment Download
M components/gcm_driver/gcm_driver.h View 4 chunks +34 lines, -16 lines 1 comment Download
M components/gcm_driver/gcm_driver.cc View 2 chunks +2 lines, -6 lines 0 comments Download
M components/gcm_driver/gcm_driver_android.h View 1 chunk +5 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_driver_android.cc View 2 chunks +11 lines, -0 lines 1 comment Download
M components/gcm_driver/gcm_driver_desktop.h View 3 chunks +15 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_driver_desktop.cc View 5 chunks +98 lines, -0 lines 1 comment Download
M components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.h View 1 chunk +5 lines, -0 lines 0 comments Download
M components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc View 1 chunk +10 lines, -0 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id.h View 2 chunks +19 lines, -11 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id_android.h View 1 chunk +4 lines, -0 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id_android.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id_impl.h View 2 chunks +8 lines, -0 lines 0 comments Download
M components/gcm_driver/instance_id/instance_id_impl.cc View 1 chunk +30 lines, -0 lines 1 comment Download
M content/browser/push_messaging/push_messaging_message_filter.h View 3 chunks +1 line, -13 lines 0 comments Download
M content/browser/push_messaging/push_messaging_message_filter.cc View 15 chunks +151 lines, -78 lines 3 comments Download
M content/child/push_messaging/push_provider.cc View 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/push_messaging_service.h View 2 chunks +22 lines, -19 lines 0 comments Download
M content/public/common/push_messaging_status.h View 1 3 chunks +19 lines, -6 lines 0 comments Download
M content/public/common/push_messaging_status.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_push_messaging_service.h View 1 chunk +17 lines, -18 lines 0 comments Download
M content/shell/browser/layout_test/layout_test_push_messaging_service.cc View 4 chunks +7 lines, -5 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +4 lines, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 3 (1 generated)
johnme
1 month, 1 week ago (2017-02-15 15:06:27 UTC) #2
Peter Beverloo
1 week, 2 days ago (2017-03-20 23:50:14 UTC) #3
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46