Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1133813002:
Introduce the NotificationIdGenerator in //content. (Closed)
|
Created:
5 years, 7 months ago by Peter Beverloo Modified:
5 years, 6 months ago Reviewers:
johnme CC:
chromium-reviews, darin-cc_chromium.org, peter+watch_chromium.org, jam, mlamouri+watch-notifications_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce the NotificationIdGenerator in //content.
We plan to move towards generating notification ids in //content rather
than having a random GUID in the NotificationObjectProxy. This enables
us to have deterministic ids, which will allow for clean replacements on
desktop and many simplications on Android.
BUG=485985
Committed: https://crrev.com/59d19e2c71476330518770dbd03e9ca6d68565f5
Cr-Commit-Position: refs/heads/master@{#331959}
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #Patch Set 3 : #
Total comments: 12
Patch Set 4 : address comments #Patch Set 5 : #Patch Set 6 : Windows build fix #
Total comments: 10
Patch Set 7 : address comments #
Messages
Total messages: 15 (3 generated)
peter@chromium.org changed reviewers: + johnme@chromium.org
+johnme for review
Initial feedback. Haven't looked at tests. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:32: stream << base::Hash(browser_context_->GetPath().value()); base::Hash is just 32-bits, and not a secure hash. So there's a nontrivial chance of these colliding, for at least some users. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:34: stream << origin.GetOrigin(); This is ambiguous. The following two cases will give the same output: origin = https://example.com:805 tag = 17 origin = https://example.com:8051 tag = 7 In both cases the generated string will end with 5117 https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:59: stream << render_process_id_; This is ambiguous. The following two cases will give the same output: render_process_id_ = 5 tag = 17 render_process_id_ = 51 tag = 7 In both cases the generated string will end with 5117 https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:24: // based on the render process id. For persistent notifications, the generated Can render process ids ever get reused (without the browser restarting)? https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:25: // id will be globally unique for on the lifetime of the notification database. s/for on/for/ https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:33: class CONTENT_EXPORT NotificationIdGenerator { Since multiple distinct notifications may share a generated id, it feels like this is really a NotificationTagGenerator (i.e. it generates a unique tag for notifications with empty tag, so that replacement based on tag matching can work). Especially since you're actually passing in an ID to the Generate methods! https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:47: // origin, tag and non-persistent notification id. Where do these non_persistent_notification_ids come from? It would be nice to add "generated by ClassNameHere" or at least "generated by Blink" or whatever so someone reading this can reason about what they mean. Ditto above for persistent_notification_ids - whilst I happen to know that those are generated by the notifications DB, future readers likely won't yet know that. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:54: BrowserContext* browser_context_; Perhaps add a comment like "Required to outlive this"? or "Should own this"?
Thanks John! Please take another look. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:32: stream << base::Hash(browser_context_->GetPath().value()); On 2015/05/08 15:35:25, johnme wrote: > base::Hash is just 32-bits, and not a secure hash. So there's a nontrivial > chance of these colliding, for at least some users. What would you propose to do instead, without overcomplicating this? https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:34: stream << origin.GetOrigin(); On 2015/05/08 15:35:25, johnme wrote: > This is ambiguous. The following two cases will give the same output: > > origin = https://example.com:805 > tag = 17 > > origin = https://example.com:8051 > tag = 7 > > In both cases the generated string will end with 5117 They wouldn't, because origins end with slashes. :-) Added a test anyway in case we ever decide to change this. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.cc:59: stream << render_process_id_; On 2015/05/08 15:35:25, johnme wrote: > This is ambiguous. The following two cases will give the same output: > > render_process_id_ = 5 > tag = 17 > > render_process_id_ = 51 > tag = 7 > > In both cases the generated string will end with 5117 Fixed, and added a test. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:24: // based on the render process id. For persistent notifications, the generated On 2015/05/08 15:35:25, johnme wrote: > Can render process ids ever get reused (without the browser restarting)? No. See ChildProcessHostImpl::GenerateChildProcessUniqueId(). https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:25: // id will be globally unique for on the lifetime of the notification database. On 2015/05/08 15:35:25, johnme wrote: > s/for on/for/ Done. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:33: class CONTENT_EXPORT NotificationIdGenerator { On 2015/05/08 15:35:25, johnme wrote: > Since multiple distinct notifications may share a generated id, it feels like > this is really a NotificationTagGenerator (i.e. it generates a unique tag for > notifications with empty tag, so that replacement based on tag matching can > work). Especially since you're actually passing in an ID to the Generate > methods! The technical term for this form of ID would be a notification "delegate id", but that's a concept that lives outside of //content. It's also a concept I would very much like to generalize to a generic kind of id. The tag of a notification is what the user (e.g. the Web API or the extensions API) can use to identify uniqueness within its own constraints. This is just a further representation of that concept within the delegate id. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:47: // origin, tag and non-persistent notification id. On 2015/05/08 15:35:26, johnme wrote: > Where do these non_persistent_notification_ids come from? It would be nice to > add "generated by ClassNameHere" or at least "generated by Blink" or whatever so > someone reading this can reason about what they mean. > > Ditto above for persistent_notification_ids - whilst I happen to know that those > are generated by the notifications DB, future readers likely won't yet know > that. Done. https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... content/browser/notifications/notification_id_generator.h:54: BrowserContext* browser_context_; On 2015/05/08 15:35:25, johnme wrote: > Perhaps add a comment like "Required to outlive this"? or "Should own this"? Done.
On 2015/05/20 18:43:34, Peter Beverloo wrote: > Thanks John! Please take another look. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > File content/browser/notifications/notification_id_generator.cc (right): > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.cc:32: stream << > base::Hash(browser_context_->GetPath().value()); > On 2015/05/08 15:35:25, johnme wrote: > > base::Hash is just 32-bits, and not a secure hash. So there's a nontrivial > > chance of these colliding, for at least some users. > > What would you propose to do instead, without overcomplicating this? > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.cc:34: stream << > origin.GetOrigin(); > On 2015/05/08 15:35:25, johnme wrote: > > This is ambiguous. The following two cases will give the same output: > > > > origin = https://example.com:805 > > tag = 17 > > > > origin = https://example.com:8051 > > tag = 7 > > > > In both cases the generated string will end with 5117 > > They wouldn't, because origins end with slashes. :-) > > Added a test anyway in case we ever decide to change this. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.cc:59: stream << > render_process_id_; > On 2015/05/08 15:35:25, johnme wrote: > > This is ambiguous. The following two cases will give the same output: > > > > render_process_id_ = 5 > > tag = 17 > > > > render_process_id_ = 51 > > tag = 7 > > > > In both cases the generated string will end with 5117 > > Fixed, and added a test. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > File content/browser/notifications/notification_id_generator.h (right): > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.h:24: // based on the > render process id. For persistent notifications, the generated > On 2015/05/08 15:35:25, johnme wrote: > > Can render process ids ever get reused (without the browser restarting)? > > No. See ChildProcessHostImpl::GenerateChildProcessUniqueId(). > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.h:25: // id will be > globally unique for on the lifetime of the notification database. > On 2015/05/08 15:35:25, johnme wrote: > > s/for on/for/ > > Done. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.h:33: class > CONTENT_EXPORT NotificationIdGenerator { > On 2015/05/08 15:35:25, johnme wrote: > > Since multiple distinct notifications may share a generated id, it feels like > > this is really a NotificationTagGenerator (i.e. it generates a unique tag for > > notifications with empty tag, so that replacement based on tag matching can > > work). Especially since you're actually passing in an ID to the Generate > > methods! > > The technical term for this form of ID would be a notification "delegate id", > but that's a concept that lives outside of //content. It's also a concept I > would very much like to generalize to a generic kind of id. > > The tag of a notification is what the user (e.g. the Web API or the extensions > API) can use to identify uniqueness within its own constraints. This is just a > further representation of that concept within the delegate id. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.h:47: // origin, tag and > non-persistent notification id. > On 2015/05/08 15:35:26, johnme wrote: > > Where do these non_persistent_notification_ids come from? It would be nice to > > add "generated by ClassNameHere" or at least "generated by Blink" or whatever > so > > someone reading this can reason about what they mean. > > > > Ditto above for persistent_notification_ids - whilst I happen to know that > those > > are generated by the notifications DB, future readers likely won't yet know > > that. > > Done. > > https://codereview.chromium.org/1133813002/diff/1/content/browser/notificatio... > content/browser/notifications/notification_id_generator.h:54: BrowserContext* > browser_context_; > On 2015/05/08 15:35:25, johnme wrote: > > Perhaps add a comment like "Required to outlive this"? or "Should own this"? > > Done. Friendly ping :)
Thanks; sorry, still haven't looked at the test. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.cc:36: BASE_HASH_NAMESPACE::hash<base::FilePath>()(browser_context_->GetPath()); I'm still uncomfortable with using a hash function that has 18742 collisions after hashing just 216553 strings[1]. [1]: http://programmers.stackexchange.com/a/145633/122643 https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.cc:74: stream << render_process_id_string.size(); This is also ambiguous. Whilst it appears to be true that string(string_of_digits.size()) + string_of_digits is unambiguous (but I consider that non-obvious), concatenating it with the non_persistent_notification_id becomes ambiguous. Please add appropriate separator characters (i.e. at least add one between render_process_id_string and non_persistent_notification_id; and either add one between render_process_id_string.size() and render_process_id_string or add a comment explaining that that concatenation is in fact unambiguous). https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.h:22: // and tag, when the tag is non-empty, or unique for the given notification when Please clearly explain that calling new Notification() or swr.showNotification() with the same (non-empty) tag as an existing notification is considered to update the existing notification 'object', hence these will have the same id, since this might otherwise be surprising to people who expect "new Notification(..)" to create a different notification 'object'.
https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator_unittest.cc (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:12: namespace { Nit: It seems unusual to wrap the entire file in an anonymous namespace - I couldn't find other unittests in content/browser which did this. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:206: // notification id. Please append "when the tag is empty", and please add an additional test in between that tests that the ids do not impact the generated nid when the tag is non-empty. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:316: TEST_F(NotificationIdGeneratorTest, NonPersistentRenderProcessIdTagAmbiguity) { Given that NonPersistentDifferentRenderProcessIds above tests that generated NIDs are independent of render process id, it would make more sense for this test to use an empty tag, and instead test concatenations of render process id and non persistent notification id, which would have caught the ambiguity I pointed out above ;)
Thanks! PTAL https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.cc:36: BASE_HASH_NAMESPACE::hash<base::FilePath>()(browser_context_->GetPath()); On 2015/05/27 18:18:35, johnme wrote: > I'm still uncomfortable with using a hash function that has 18742 collisions > after hashing just 216553 strings[1]. > > [1]: http://programmers.stackexchange.com/a/145633/122643 Switched to SHA-1 after talking to bauerb@. Generated IDs are now ~50 characters long, which is on the long side, but still OK. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.cc:74: stream << render_process_id_string.size(); On 2015/05/27 18:18:35, johnme wrote: > This is also ambiguous. Whilst it appears to be true that > string(string_of_digits.size()) + string_of_digits is unambiguous (but I > consider that non-obvious), concatenating it with the > non_persistent_notification_id becomes ambiguous. > > Please add appropriate separator characters (i.e. at least add one between > render_process_id_string and non_persistent_notification_id; and either add one > between render_process_id_string.size() and render_process_id_string or add a > comment explaining that that concatenation is in fact unambiguous). Done. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator.h:22: // and tag, when the tag is non-empty, or unique for the given notification when On 2015/05/27 18:18:35, johnme wrote: > Please clearly explain that calling new Notification() or swr.showNotification() > with the same (non-empty) tag as an existing notification is considered to > update the existing notification 'object', hence these will have the same id, > since this might otherwise be surprising to people who expect "new > Notification(..)" to create a different notification 'object'. Done. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... File content/browser/notifications/notification_id_generator_unittest.cc (right): https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:12: namespace { On 2015/05/28 11:00:02, johnme wrote: > Nit: It seems unusual to wrap the entire file in an anonymous namespace - I > couldn't find other unittests in content/browser which did this. See the "[chromium-dev] Namespace in unit tests" thread, which recommended doing this. Since I do not need any friend statement, this works just fine and minimizes the chance of name collisions within //content tests. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:206: // notification id. On 2015/05/28 11:00:02, johnme wrote: > Please append "when the tag is empty", and please add an additional test in > between that tests that the ids do not impact the generated nid when the tag is > non-empty. Done. https://codereview.chromium.org/1133813002/diff/40001/content/browser/notific... content/browser/notifications/notification_id_generator_unittest.cc:316: TEST_F(NotificationIdGeneratorTest, NonPersistentRenderProcessIdTagAmbiguity) { On 2015/05/28 11:00:02, johnme wrote: > Given that NonPersistentDifferentRenderProcessIds above tests that generated > NIDs are independent of render process id, it would make more sense for this > test to use an empty tag, and instead test concatenations of render process id > and non persistent notification id, which would have caught the ambiguity I > pointed out above ;) Done.
lgtm with nits - thanks! https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:24: // manager), thus SHA-1 is used to maximise preimage resistance. Nit: "preimage resistance" is somewhat obscure, and I also don't think it's technically correct (since you don't care if an attacker can craft an input which hashes to a desired value, you just care whether the inputs generated by Chrome ever collide). It's also not particularly relevant that it's used outside of Chrome (any attacker outside of Chrome will have access to the filesystem, and can easily enumerate Chrome profile paths). I'd replace the entire last sentence with "Since we only store the hash, SHA-1 is used to make the probability of collisions negligible.". https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:65: stream << base::Int64ToString(persistent_notification_id); Nit: the base::Int64ToString is presumably unnecessary (see below). So either remove it, or add IntToString for render_process_id_. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:91: stream << base::IntToString(non_persistent_notification_id); Nit: the base::IntToString seems unnecessary, since << can convert ints to string (as you do for render_process_id_ above). So either remove it, or add it for render_process_id_. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.h:27: // Notifications of the same type coming from the same origin and having the "of the same type"? That's not what the code enforces - it generates the same ids for both persistent and non-persistent if they have the same tag. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.h:57: int non_persistent_notification_id) const; Nit: I still find it a bit weird that you pass in a "notification id" to this and get back a "notification id" (which might be the same as one obtained earlier when passing in a different id value, if the tag is the same). Perhaps we can rename non_persistent_notification_id to non_persistent_notification_request_id so there's only a single concept of notification id? And something similar for persistent notifications...
https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... File content/browser/notifications/notification_id_generator.cc (right): https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:24: // manager), thus SHA-1 is used to maximise preimage resistance. On 2015/05/29 09:34:57, johnme wrote: > Nit: "preimage resistance" is somewhat obscure, and I also don't think it's > technically correct (since you don't care if an attacker can craft an input > which hashes to a desired value, you just care whether the inputs generated by > Chrome ever collide). It's also not particularly relevant that it's used outside > of Chrome (any attacker outside of Chrome will have access to the filesystem, > and can easily enumerate Chrome profile paths). I'd replace the entire last > sentence with "Since we only store the hash, SHA-1 is used to make the > probability of collisions negligible.". Done. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:65: stream << base::Int64ToString(persistent_notification_id); On 2015/05/29 09:34:57, johnme wrote: > Nit: the base::Int64ToString is presumably unnecessary (see below). So either > remove it, or add IntToString for render_process_id_. Done. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.cc:91: stream << base::IntToString(non_persistent_notification_id); On 2015/05/29 09:34:57, johnme wrote: > Nit: the base::IntToString seems unnecessary, since << can convert ints to > string (as you do for render_process_id_ above). So either remove it, or add it > for render_process_id_. Done. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... File content/browser/notifications/notification_id_generator.h (right): https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.h:27: // Notifications of the same type coming from the same origin and having the On 2015/05/29 09:34:57, johnme wrote: > "of the same type"? That's not what the code enforces - it generates the same > ids for both persistent and non-persistent if they have the same tag. Done. https://codereview.chromium.org/1133813002/diff/100001/content/browser/notifi... content/browser/notifications/notification_id_generator.h:57: int non_persistent_notification_id) const; On 2015/05/29 09:34:57, johnme wrote: > Nit: I still find it a bit weird that you pass in a "notification id" to this > and get back a "notification id" (which might be the same as one obtained > earlier when passing in a different id value, if the tag is the same). Perhaps > we can rename non_persistent_notification_id to > non_persistent_notification_request_id so there's only a single concept of > notification id? And something similar for persistent notifications... I'm going to leave it as it is in this patch, since renaming that consistently would have to be done in a variety of places in either case. Let's discuss next week in person.
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org Link to the patchset: https://codereview.chromium.org/1133813002/#ps120001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1133813002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/59d19e2c71476330518770dbd03e9ca6d68565f5 Cr-Commit-Position: refs/heads/master@{#331959} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
