Issue 2735213002: Indexed DB: Ensure large explicit keys consistently max out generator

Issue 2735213002: Indexed DB: Ensure large explicit keys consistently max out generator (Closed)

Created:
3 years, 9 months ago by jsbell

Modified:
3 years, 9 months ago

Reviewers:
pwnall

CC:
chromium-reviews, blink-reviews-w3ctests_chromium.org, jam, darin-cc_chromium.org, cmumford, blink-reviews, jsbell+idb_chromium.org

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Indexed DB: Ensure large explicit keys consistently max out generator Background: Stores can have a key generator which generates successive numeric keys. Storing a record with an explicit numeric key adjusts they key generator to produce values above the explicit key. Once the generator hits 2^53 it stops generating new keys (since that's the maximum integer uniquely representable as a JS number). Chrome's logic for certain values above this limit was "wonky". Values above 2^53 would max out the generator. Values above 2^63 and Infinity would be ignored and not adjust the generator, due to relying on undefined double->int64_t casting behavior. Fix to always max out the generator for large values. Also adds web-platform-tests - other implementations are wonky too. :( Also adds some missing test coverage for key injection cases. Spec discussion: https://github.com/w3c/IndexedDB/issues/147 BUG=691754 Review-Url: https://codereview.chromium.org/2735213002 Cr-Commit-Position: refs/heads/master@{#455256} Committed: https://chromium.googlesource.com/chromium/src/+/c4c73fb74bfeedbc41e0fc41f14feed642c6a86b

Patch Set 1 #

Total comments: 10

Patch Set 2 : Incorporate review feedback #

Created: 3 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+278 lines, -6 lines)			Patch
M	content/browser/indexed_db/indexed_db_database.cc	View	1	4 chunks	+13 lines, -6 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-explicit.html	View	1	1 chunk	+146 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html	View	1	1 chunk	+119 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (8 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

pwnall

LGTM with nits. https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/indexed_db_database.cc File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/indexed_db_database.cc#newcode1216 content/browser/indexed_db/indexed_db_database.cc:1216: // Maximum integer storable as ECMAScript ...

3 years, 9 months ago (2017-03-07 19:42:41 UTC) #5

jsbell

Description was changed from ========== Indexed DB: Ensure large explicit keys consistently max out generator ...

3 years, 9 months ago (2017-03-07 20:07:45 UTC) #6

jsbell

https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/indexed_db_database.cc File content/browser/indexed_db/indexed_db_database.cc (right): https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/indexed_db_database.cc#newcode1216 content/browser/indexed_db/indexed_db_database.cc:1216: // Maximum integer storable as ECMAScript number. On 2017/03/07 ...

3 years, 9 months ago (2017-03-07 20:27:57 UTC) #7

https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2735213002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1216: // Maximum integer
storable as ECMAScript number.
On 2017/03/07 19:42:41, pwnall wrote:
> Technically not true, 9007199254740994 can also be stored. I suspect you mean
> it's the top of the interval of integers w/ the property that all interval
> members can be stored. I wonder if it'd be easier to say this is
> Number.MAX_SAFE_INTEGER + 1, and which gives a good hint to the interested
> reader, who can then Google for the details.

Yeah, bad comment (copied pasted from above). Needs the qualifier "uniquely";
fixed both.

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-explicit.html
(right):

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-explicit.html:95:
{
On 2017/03/07 19:42:41, pwnall wrote:
> Would it make sense to have two tests around the edge case? I mean one test
> where the key is incremented (Number.MAX_SAFE_INTEGER + 1), and one where it
> isn't (Number.MAX_SAFE_INTEGER + 1).

keygenerator-overflow.htm tests this, in theory. I added a case for
Number.MAX_SAFE_INTEGER + 1

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html
(right):

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html:1:
<!DOCTYPE html>
On 2017/03/07 19:42:41, pwnall wrote:
> Nit: This test doesn't look related to the commit message. More tests > fewer
> tests, so let's keep it in, if it's not duplicated.

Agreed, it's unrelated. Added a note to the CL description.

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html:32:
'Key is injected into value - simple path');
On 2017/03/07 19:42:41, pwnall wrote:
> How about single-segment key path and multi-segment key path instead of
> simple/complex?
> 
> I initially parsed "simple path" vs "complex path" as code paths, which got me
> confused for a bit, because I knew it's not your style to leak implementation
> details into tests.

Done.

https://codereview.chromium.org/2735213002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/keygenerator-inject.html:57:

On 2017/03/07 19:42:41, pwnall wrote:
> Would it make sense to have a test with store.put({a: {name: {'b'}}), to
verify
> that the implementation can switch between following & creating key path
> segments?

Good idea. Done.

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/2735213002/20001

3 years, 9 months ago (2017-03-07 20:28:46 UTC) #10

commit-bot: I haz the power

Description was changed from ========== Indexed DB: Ensure large explicit keys consistently max out generator ...

3 years, 9 months ago (2017-03-07 22:35:21 UTC) #12

Message was sent while issue was closed.

Description was changed from

==========
Indexed DB: Ensure large explicit keys consistently max out generator

Background: Stores can have a key generator which generates successive
numeric keys. Storing a record with an explicit numeric key adjusts
they key generator to produce values above the explicit key. Once the
generator hits 2^53 it stops generating new keys (since that's the
maximum integer uniquely representable as a JS number).

Chrome's logic for certain values above this limit was "wonky". Values
above 2^53 would max out the generator. Values above 2^63 and Infinity
would be ignored and not adjust the generator, due to relying on
undefined double->int64_t casting behavior.

Fix to always max out the generator for large values. Also adds
web-platform-tests - other implementations are wonky too. :(

Also adds some missing test coverage for key injection cases.

Spec discussion: https://github.com/w3c/IndexedDB/issues/147

BUG=691754
==========

to

==========
Indexed DB: Ensure large explicit keys consistently max out generator

Background: Stores can have a key generator which generates successive
numeric keys. Storing a record with an explicit numeric key adjusts
they key generator to produce values above the explicit key. Once the
generator hits 2^53 it stops generating new keys (since that's the
maximum integer uniquely representable as a JS number).

Chrome's logic for certain values above this limit was "wonky". Values
above 2^53 would max out the generator. Values above 2^63 and Infinity
would be ignored and not adjust the generator, due to relying on
undefined double->int64_t casting behavior.

Fix to always max out the generator for large values. Also adds
web-platform-tests - other implementations are wonky too. :(

Also adds some missing test coverage for key injection cases.

Spec discussion: https://github.com/w3c/IndexedDB/issues/147

BUG=691754

Review-Url: https://codereview.chromium.org/2735213002
Cr-Commit-Position: refs/heads/master@{#455256}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4c73fb74bfeedbc41e0fc41f14f...
==========

commit-bot: I haz the power

3 years, 9 months ago (2017-03-07 22:35:22 UTC) #13

Message was sent while issue was closed.

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c4c73fb74bfeedbc41e0fc41f14f...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages