Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2697793004: Push API: Validate storage before returning cached subscriptions (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by johnme
Modified:
4 months, 2 weeks ago
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/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 Review-Url: https://codereview.chromium.org/2697793004 Cr-Commit-Position: refs/heads/master@{#461501} Committed: https://chromium.googlesource.com/chromium/src/+/6576ecf83ab56518daa1274dfa354aee84c8696b

Patch Set 1 #

Patch Set 2 : Comment out PUSH_GETREGISTRATION_STATUS_PUBLIC_KEY_UNAVAILABLE #

Total comments: 26

Patch Set 3 : Rebase onto PushMessagingManager #

Patch Set 4 : Address peter's review comments #

Total comments: 7

Patch Set 5 : Address nasko's review comments #

Patch Set 6 : Fix include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+810 lines, -220 lines) Patch
M chrome/browser/push_messaging/push_messaging_app_identifier.h View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_app_identifier.cc View 1 2 1 chunk +22 lines, -3 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_browsertest.cc View 1 2 3 7 chunks +130 lines, -2 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.h View 1 2 3 2 chunks +14 lines, -7 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.cc View 1 2 3 2 chunks +41 lines, -11 lines 0 comments 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 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_client_impl.cc View 1 2 3 2 chunks +36 lines, -3 lines 0 comments Download
M components/gcm_driver/gcm_driver.h View 1 2 3 4 chunks +38 lines, -19 lines 0 comments Download
M components/gcm_driver/gcm_driver.cc View 1 2 3 1 chunk +0 lines, -6 lines 0 comments Download
M components/gcm_driver/gcm_driver_android.h View 1 2 3 4 5 2 chunks +6 lines, -0 lines 0 comments Download
M components/gcm_driver/gcm_driver_android.cc View 2 chunks +11 lines, -0 lines 0 comments 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 1 2 5 chunks +98 lines, -0 lines 0 comments 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 0 comments Download
M content/browser/push_messaging/push_messaging_manager.h View 1 2 3 4 3 chunks +4 lines, -13 lines 0 comments Download
M content/browser/push_messaging/push_messaging_manager.cc View 1 2 3 17 chunks +166 lines, -82 lines 0 comments Download
M content/child/push_messaging/push_provider.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/common/push_messaging.mojom View 1 2 2 chunks +11 lines, -4 lines 0 comments Download
M content/common/push_messaging_param_traits.cc View 1 2 3 4 2 chunks +9 lines, -3 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 1 2 3 4 5 2 chunks +4 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 21 (9 generated)
johnme
6 months ago (2017-02-15 15:06:27 UTC) #2
Peter Beverloo
This is cool and good, thanks :) lgtm % comments https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_messaging/push_messaging_app_identifier.h File chrome/browser/push_messaging/push_messaging_app_identifier.h (right): https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_messaging/push_messaging_app_identifier.h#newcode47 ...
5 months ago (2017-03-20 23:50:14 UTC) #3
johnme
Addressed review comments - thanks! https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_messaging/push_messaging_app_identifier.h File chrome/browser/push_messaging/push_messaging_app_identifier.h (right): https://codereview.chromium.org/2697793004/diff/20001/chrome/browser/push_messaging/push_messaging_app_identifier.h#newcode47 chrome/browser/push_messaging/push_messaging_app_identifier.h:47: static PushMessagingAppIdentifier LegacyGenerateForTesting( On ...
4 months, 3 weeks ago (2017-03-30 18:36:38 UTC) #4
johnme
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!
4 months, 3 weeks ago (2017-03-30 18:42:50 UTC) #6
Ilya Sherman
histograms.xml lgtm
4 months, 3 weeks ago (2017-03-30 21:58:32 UTC) #7
nasko
https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_messaging/push_messaging_manager.h File content/browser/push_messaging/push_messaging_manager.h (right): https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_messaging/push_messaging_manager.h#newcode122 content/browser/push_messaging/push_messaging_manager.h:122: // Can be used on the IO thread as ...
4 months, 3 weeks ago (2017-03-31 21:48:43 UTC) #8
johnme
Addressed nasko's review comments - thanks :) https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_messaging/push_messaging_manager.h File content/browser/push_messaging/push_messaging_manager.h (right): https://codereview.chromium.org/2697793004/diff/60001/content/browser/push_messaging/push_messaging_manager.h#newcode122 content/browser/push_messaging/push_messaging_manager.h:122: // Can ...
4 months, 2 weeks ago (2017-04-03 13:54:28 UTC) #9
nasko
LGTM https://codereview.chromium.org/2697793004/diff/60001/content/common/push_messaging_param_traits.cc File content/common/push_messaging_param_traits.cc (left): https://codereview.chromium.org/2697793004/diff/60001/content/common/push_messaging_param_traits.cc#oldcode191 content/common/push_messaging_param_traits.cc:191: "PushGetRegistrationStatus enums must match, PUBLIC_KEY_UNAVAILABLE"); On 2017/04/03 13:54:27, ...
4 months, 2 weeks ago (2017-04-03 14:10:38 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697793004/80001
4 months, 2 weeks ago (2017-04-03 14:18:50 UTC) #13
commit-bot: I haz the power
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_swarming_rel/builds/148966)
4 months, 2 weeks ago (2017-04-03 14:45:33 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697793004/100001
4 months, 2 weeks ago (2017-04-03 17:01:31 UTC) #18
commit-bot: I haz the power
4 months, 2 weeks ago (2017-04-03 19:27:39 UTC) #21
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6576ecf83ab56518daa1274dfa35...
Sign in to reply to this message.

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