Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2460223004:
Fix potential bug in push_test.js - do not cache subscription options (Closed)
|
Created:
4 years, 1 month ago by awdf Modified:
4 years, 1 month ago Reviewers:
Peter Beverloo CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, harkness+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix potential bug in push_test.js - do not cache subscription options
- Prior to this patch, the application server key could be inadvertently
cached, e.g. if a test in PushMessagingBrowserTest did the following:
documentSubscribePush()
removeManifest()
documentSubscribePushWithoutKey()
then the second push would actually use the cached key from the first.
- No existing tests happen to hit the bug but that's just luck.
BUG=660874
Committed: https://crrev.com/5069448246d980f994a700bc937aba59dc6e9ab6
Cr-Commit-Position: refs/heads/master@{#428983}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixup failing test, inline the options object #Patch Set 3 : oops, remove duplicated line #
Total comments: 1
Patch Set 4 : stop passing empty dict #Patch Set 5 : rebase #
Messages
Total messages: 25 (13 generated)
awdf@chromium.org changed reviewers: + peter@chromium.org
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:97: pushSubscriptionOptions) nit: consider inlining the dictionary https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:97: pushSubscriptionOptions) I see failing tests. Is it possible that subscribing for push without a key perhaps wasn't done without a key at all? https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:144: return swRegistration.pushManager.permissionState(pushSubscriptionOptions) nit: consider inlining the dictionary
https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:97: pushSubscriptionOptions) On 2016/10/31 17:30:20, Peter Beverloo wrote: > I see failing tests. Is it possible that subscribing for push without a key > perhaps wasn't done without a key at all? Huh, that's weird, I thought they might but I'm pretty sure I checked and none in PushMessagingBrowserTest failed for me locally with this patch. Although now I come to think of it, a bunch are disabled on Linux. But some linux trybots failed too so it's still odd.. I will investigate!
On 2016/10/31 17:35:02, awdf wrote: > https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... > File chrome/test/data/push_messaging/push_test.js (right): > > https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... > chrome/test/data/push_messaging/push_test.js:97: pushSubscriptionOptions) > On 2016/10/31 17:30:20, Peter Beverloo wrote: > > I see failing tests. Is it possible that subscribing for push without a key > > perhaps wasn't done without a key at all? > > Huh, that's weird, I thought they might but I'm pretty sure I checked and none > in PushMessagingBrowserTest failed for me locally with this patch. Although now > I come to think of it, a bunch are disabled on Linux. But some linux trybots > failed too so it's still odd.. I will investigate! Turns out I didn't spot it because I was locally testing PushMessagingBrowserTest.*, and the failing test is actually in PushMessagingBrowserTestEmptySubscriptionOptions.* The reason is because that class uses the following html file which overrides the global var to give it no subscription options -_- https://cs.chromium.org/chromium/src/chrome/test/data/push_messaging/test_no_... will fix it up, probably by inventing a new method to call instead of all this html overriding javascript cleverness
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:97: pushSubscriptionOptions) On 2016/10/31 17:30:20, Peter Beverloo wrote: > nit: consider inlining the dictionary Done. https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:106: let pushSubscriptionOptions = { ah did you prefer this one not inlined actually? i can put it back.. https://codereview.chromium.org/2460223004/diff/1/chrome/test/data/push_messa... chrome/test/data/push_messaging/push_test.js:144: return swRegistration.pushManager.permissionState(pushSubscriptionOptions) On 2016/10/31 17:30:20, Peter Beverloo wrote: > nit: consider inlining the dictionary Done.
lgtm https://codereview.chromium.org/2460223004/diff/40001/chrome/test/data/push_m... File chrome/test/data/push_messaging/push_test.js (right): https://codereview.chromium.org/2460223004/diff/40001/chrome/test/data/push_m... chrome/test/data/push_messaging/push_test.js:103: {} /* pushSubscriptionOptions */) nit: no need to pass anything here, it defaults to an "empty" dictionary.
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2460223004/#ps60001 (title: "stop passing empty dict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2016/10/31 19:24:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) Looks like that test is unrelated to my changes & just flaky - https://bugs.chromium.org/p/chromium/issues/detail?id=660343 So I guess I'll just try again..
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/2460223004/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Fix potential bug in push_test.js - do not cache subscription options
- Prior to this patch, the application server key could be inadvertently
cached, e.g. if a test in PushMessagingBrowserTest did the following:
documentSubscribePush()
removeManifest()
documentSubscribePushWithoutKey()
then the second push would actually use the cached key from the first.
- No existing tests happen to hit the bug but that's just luck.
BUG=660874
==========
to
==========
Fix potential bug in push_test.js - do not cache subscription options
- Prior to this patch, the application server key could be inadvertently
cached, e.g. if a test in PushMessagingBrowserTest did the following:
documentSubscribePush()
removeManifest()
documentSubscribePushWithoutKey()
then the second push would actually use the cached key from the first.
- No existing tests happen to hit the bug but that's just luck.
BUG=660874
Committed: https://crrev.com/5069448246d980f994a700bc937aba59dc6e9ab6
Cr-Commit-Position: refs/heads/master@{#428983}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5069448246d980f994a700bc937aba59dc6e9ab6 Cr-Commit-Position: refs/heads/master@{#428983} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
