primary key is missing when querying newly created large record in indexedDB
Reported by
liux...@gmail.com,
Jun 21 2017
|
||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3136.0 Safari/537.36 Steps to reproduce the problem: 1. Open the attached file, or the gist here: https://gist.github.com/gera2ld/29addf3f45fb483622cad2b205208e0b 2. Click on the button 3. See the log What is the expected behavior? Each result should have an `id` key. What went wrong? On Chrome 59.0.3071.109, `id` appears. But on Chrome 61.0.3136.0, `id` is missing. Did this work before? Yes 59.0.3071.109 Does this work in other browsers? Yes Chrome version: 61.0.3136.0 Channel: canary OS Version: OS X 10.11.6 Flash Version:
,
Jun 21 2017
dmurph@ can you try and repro?
,
Jun 22 2017
Confirmed. Theory: because we're auto-blobing this, the id doesn't get added? or that's accidentally not happening? +pwnall does this ring a bell for a possible cause by auto-blobing?
,
Jun 22 2017
Able to reproduce the issue on Canary 61.0.3137.0, Dev# 61.0.3135.4 and Beta# 60.0.3112.40 on Windows 10, Mac 10.12.5, Ubuntu 14.04. Issue is a regression broken in M60 and below are the bisect details for the same using per revision bisect tool: Bisect Info: ============ 60.0.3111.0 - Good Build 60.0.3112.0 - Bad build You are probably looking for a change made after 474873 (known good), but no later than 474874 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/09692cdc03a2723afadf7109b2b8404a9eb497ed..9f25693ac827e8ea87699023e640760fd09ef9bb @pwnall: Kindly take a look into it. Please help us to find an owner if not with respect to your change. Adding Beta blocker, since broken in M60. Please reduce priority or remove if not the case. Thanks.!
,
Jun 22 2017
#4: Thank you very much for the bisect! In theory, value unwrapping (auto-blobbing) is supposed to happen before primary key injection happens. I must have broken something along the way. I'll try to investigate and put together a fix in the morning.
,
Jun 22 2017
,
Jun 23 2017
pwnall@, would be really great if we can land a fix ASAP? so that we'll consider this change for M60 Beta next week.
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2dd9152cf08d518fe0b984bee9252db408cc52a2 commit 2dd9152cf08d518fe0b984bee9252db408cc52a2 Author: Victor Costan <pwnall@chromium.org> Date: Fri Jun 23 20:24:28 2017 Fix key generator handling in IndexedDB value wrapping. IDBValueUnwrapper::Unwrap is responsible for creating an IDBValue that mirrors its input, which is an IDBValue whose SSV data was wrapped in a Blob. Unfortunately, the old implementation did not carry over the IDBValue's primary_key_ and key_path_ members. This snuck through testing because these members are only used when reading values without explicit primary keys from object stores with key generators (where autoIncrement is true). In that specific case, the IDBValue's primary key is injected into the de-serialized V8 value using the IDB's key path. Most of this CL is a JavaScript diff restructures the WPT test that covers serialization/deserialization for IndexedDB valeus that get wrapped in Blobs. The second large component is a C++ diff that adds IDBObjectStores to the unit tests for value wrapping, which is necessary because IDBValues whose primary_key_ is not undefined undergo extra DCHECKs. The actual bugfix is a few lines of diffs in IDBValue.{h,cpp} and IDBValueWrapping.cpp. BUG= 735317 Change-Id: Ib303c6d442239833cc537c3d5c1f01a763165eee Reviewed-on: https://chromium-review.googlesource.com/544543 Commit-Queue: Victor Costan <pwnall@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/master@{#482005} [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/LayoutTests/TestExpectations [delete] https://crrev.com/27a522ddf6ece1cccf741e309e035bf0d461657a/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html [add] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-common.js [add] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-large-multiple.html [add] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-large.html [add] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-small.html [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBValue.h [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp [modify] https://crrev.com/2dd9152cf08d518fe0b984bee9252db408cc52a2/third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp
,
Jun 23 2017
Fixed the bug. Requesting merge on M60. This bug is rather subtle, but nevertheless it can lead to data loss in Web applications that use IndexedDB.
,
Jun 23 2017
To be clear: I meant I'm requesting _permission_ to merge to M60. I can do the merge myself.
,
Jun 24 2017
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 26 2017
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdeb77121c6c77a7d295d74b2d10db8ccc588098 commit cdeb77121c6c77a7d295d74b2d10db8ccc588098 Author: Victor Costan <pwnall@chromium.org> Date: Tue Jun 27 19:07:13 2017 Fix key generator handling in IndexedDB value wrapping. IDBValueUnwrapper::Unwrap is responsible for creating an IDBValue that mirrors its input, which is an IDBValue whose SSV data was wrapped in a Blob. Unfortunately, the old implementation did not carry over the IDBValue's primary_key_ and key_path_ members. This snuck through testing because these members are only used when reading values without explicit primary keys from object stores with key generators (where autoIncrement is true). In that specific case, the IDBValue's primary key is injected into the de-serialized V8 value using the IDB's key path. Most of this CL is a JavaScript diff restructures the WPT test that covers serialization/deserialization for IndexedDB valeus that get wrapped in Blobs. The second large component is a C++ diff that adds IDBObjectStores to the unit tests for value wrapping, which is necessary because IDBValues whose primary_key_ is not undefined undergo extra DCHECKs. The actual bugfix is a few lines of diffs in IDBValue.{h,cpp} and IDBValueWrapping.cpp. BUG= 735317 (cherry picked from commit 2dd9152cf08d518fe0b984bee9252db408cc52a2) Change-Id: Ib303c6d442239833cc537c3d5c1f01a763165eee Reviewed-on: https://chromium-review.googlesource.com/544543 Commit-Queue: Victor Costan <pwnall@chromium.org> Reviewed-by: Joshua Bell <jsbell@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#482005} Reviewed-on: https://chromium-review.googlesource.com/546961 Cr-Commit-Position: refs/branch-heads/3112@{#482} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [delete] https://crrev.com/ac979d32d6ce857f746f44bb009b180676b3114a/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html [add] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-common.js [add] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-large-multiple.html [add] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-large.html [add] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/nested-cloning-small.html [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBMetadata.h [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBValue.h [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp [modify] https://crrev.com/cdeb77121c6c77a7d295d74b2d10db8ccc588098/third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by ranjitkan@chromium.org
, Jun 21 2017