Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1870223002:
[Downloads/History] Comply with RFC 4122 when generating GUIDs. (Closed)
Description[Downloads/History] Comply with RFC 4122 when generating GUIDs.
R=sky
BUG=7648
Committed: https://crrev.com/3b37fd2eabd9492f977a76957aa19ee31ce8fc44
Cr-Commit-Position: refs/heads/master@{#386707}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add a comment explaining previous GUID generation scheme and why no migration is necessary. #
Dependent Patchsets: Messages
Total messages: 13 (5 generated)
Description was changed from ========== [Downloads/History] Comply with RFC 4122 when generating GUIDs. BUG=7648 ========== to ========== [Downloads/History] Comply with RFC 4122 when generating GUIDs. R=sky BUG=7648 ==========
asanka@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... File components/history/core/browser/download_database.cc (right): https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... components/history/core/browser/download_database.cc:222: // XXXXXXXX-RRRR-4RRR-yRRR-RRRRRRRRRRRR The old code may not have generated the 4 and y, right? If so, are there problems with previous entries that may not follow the new values?
https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... File components/history/core/browser/download_database.cc (right): https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... components/history/core/browser/download_database.cc:222: // XXXXXXXX-RRRR-4RRR-yRRR-RRRRRRRRRRRR On 2016/04/08 at 21:59:32, sky wrote: > The old code may not have generated the 4 and y, right? If so, are there problems with previous entries that may not follow the new values? None at all. I'd have added migration code if it did. The decision to use GUIDs was so that the persistent ID of a download had known properties (fixed size, case insensitive, unique-subject-to-case-normalization, comprised of unambiguous character set). Other than that, callers treat the IDs as opaque strings. Those that treat the string as a GUID and attempt to parse it will succeed regardless of whether the GUID matches the requirements of a ver 4 GUID or not. No known parser attempts to validate whether a GUID conforms to just the types specified in RFC 4122. The formats are there so that a GUID generated using one method won't accidentally collide with a GUID generated using another method. Other than that the method isn't interesting to a caller. On Posix base::GenerateGUID() generates type 4 GUIDs. I've experimentally verified that this is also the case for Windows although the CoCreateGuid/UuidCreate APIs don't document the type of GUID generated. The code above also generates type 4 GUIDs on all platforms during migration. The code that was there before generates type 4 GUIDs with the exception that the 6 fixed format indicator bits are not fixed (let's call these type IGs for Incorrectly Generated). Hence the probability of collision between an IG GUID and a type 4 GUID == (1/64) * (probability of collision between two type 4 GUIDs). I decided to be pedantic here since there's a spec and I felt like we should generate conformant GUIDs.
On 2016/04/11 17:32:59, asanka wrote: > https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... > File components/history/core/browser/download_database.cc (right): > > https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... > components/history/core/browser/download_database.cc:222: // > XXXXXXXX-RRRR-4RRR-yRRR-RRRRRRRRRRRR > On 2016/04/08 at 21:59:32, sky wrote: > > The old code may not have generated the 4 and y, right? If so, are there > problems with previous entries that may not follow the new values? > > None at all. I'd have added migration code if it did. > > The decision to use GUIDs was so that the persistent ID of a download had known > properties (fixed size, case insensitive, unique-subject-to-case-normalization, > comprised of unambiguous character set). Other than that, callers treat the IDs > as opaque strings. Those that treat the string as a GUID and attempt to parse it > will succeed regardless of whether the GUID matches the requirements of a ver 4 > GUID or not. No known parser attempts to validate whether a GUID conforms to > just the types specified in RFC 4122. > > The formats are there so that a GUID generated using one method won't > accidentally collide with a GUID generated using another method. Other than that > the method isn't interesting to a caller. > > On Posix base::GenerateGUID() generates type 4 GUIDs. I've experimentally > verified that this is also the case for Windows although the > CoCreateGuid/UuidCreate APIs don't document the type of GUID generated. The code > above also generates type 4 GUIDs on all platforms during migration. The code > that was there before generates type 4 GUIDs with the exception that the 6 fixed > format indicator bits are not fixed (let's call these type IGs for Incorrectly > Generated). > > Hence the probability of collision between an IG GUID and a type 4 GUID == > (1/64) * (probability of collision between two type 4 GUIDs). > > I decided to be pedantic here since there's a spec and I felt like we should > generate conformant GUIDs. I think you should at least at a comment that old guids have a slightly different format. LGTM
On 2016/04/11 at 20:55:08, sky wrote: > On 2016/04/11 17:32:59, asanka wrote: > > https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... > > File components/history/core/browser/download_database.cc (right): > > > > https://codereview.chromium.org/1870223002/diff/1/components/history/core/bro... > > components/history/core/browser/download_database.cc:222: // > > XXXXXXXX-RRRR-4RRR-yRRR-RRRRRRRRRRRR > > On 2016/04/08 at 21:59:32, sky wrote: > > > The old code may not have generated the 4 and y, right? If so, are there > > problems with previous entries that may not follow the new values? > > > > None at all. I'd have added migration code if it did. > > > > The decision to use GUIDs was so that the persistent ID of a download had known > > properties (fixed size, case insensitive, unique-subject-to-case-normalization, > > comprised of unambiguous character set). Other than that, callers treat the IDs > > as opaque strings. Those that treat the string as a GUID and attempt to parse it > > will succeed regardless of whether the GUID matches the requirements of a ver 4 > > GUID or not. No known parser attempts to validate whether a GUID conforms to > > just the types specified in RFC 4122. > > > > The formats are there so that a GUID generated using one method won't > > accidentally collide with a GUID generated using another method. Other than that > > the method isn't interesting to a caller. > > > > On Posix base::GenerateGUID() generates type 4 GUIDs. I've experimentally > > verified that this is also the case for Windows although the > > CoCreateGuid/UuidCreate APIs don't document the type of GUID generated. The code > > above also generates type 4 GUIDs on all platforms during migration. The code > > that was there before generates type 4 GUIDs with the exception that the 6 fixed > > format indicator bits are not fixed (let's call these type IGs for Incorrectly > > Generated). > > > > Hence the probability of collision between an IG GUID and a type 4 GUID == > > (1/64) * (probability of collision between two type 4 GUIDs). > > > > I decided to be pedantic here since there's a spec and I felt like we should > > generate conformant GUIDs. > > I think you should at least at a comment that old guids have a slightly different format. > LGTM Done. Thanks!
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1870223002/#ps20001 (title: "Add a comment explaining previous GUID generation scheme and why no migration is necessary.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870223002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870223002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Downloads/History] Comply with RFC 4122 when generating GUIDs. R=sky BUG=7648 ========== to ========== [Downloads/History] Comply with RFC 4122 when generating GUIDs. R=sky BUG=7648 Committed: https://crrev.com/3b37fd2eabd9492f977a76957aa19ee31ce8fc44 Cr-Commit-Position: refs/heads/master@{#386707} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3b37fd2eabd9492f977a76957aa19ee31ce8fc44 Cr-Commit-Position: refs/heads/master@{#386707} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
