Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(445)

Issue 2781273004: Graceful handling of new versions of IndexedDB serialized data. (Closed)

Created:
3 years, 8 months ago by pwnall
Modified:
3 years, 8 months ago
Reviewers:
jbroman, jsbell
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, jsbell
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Graceful handling of new versions of IndexedDB serialized data. The renderer currently crashes when attempting to de-serialize an IndexedDB value whose version number is too new (past what we can read), in a very specific situation -- the object store must have a keyPath and autoIncrement set to true. In all other situatios where IndexedDB value serialization fails due to overly new versions, the resulting value is null. This is not as terrible as it sounds, because a future version would generally indicate that a user switched channels and got an older Chrome build. Random storage corruption results in CRC errors, which are handled differently. This CL fixes the crash, resulting in the same behavior (null values) as in other cases involving data saved by newer versions. TESTED=also verified the fix against the manual bug repro steps BUG=703704 Review-Url: https://codereview.chromium.org/2781273004 Cr-Commit-Position: refs/heads/master@{#461938} Committed: https://chromium.googlesource.com/chromium/src/+/cdd55622ffec06c7513b366777fe53566aabde4f

Patch Set 1 : Attempt to fix Windows build error. #

Patch Set 2 : Document reason for unit test. #

Total comments: 6

Patch Set 3 : Addressed feedback. #

Total comments: 18

Patch Set 4 : Feedback + fix additional crash cases. #

Total comments: 7

Patch Set 5 : Addressed nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+229 lines, -19 lines) Patch
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.h View 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp View 1 2 3 4 7 chunks +73 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp View 1 2 3 3 chunks +152 lines, -0 lines 0 comments Download

Messages

Total messages: 48 (37 generated)
pwnall
jbroman: Can you please review the code? jsbell: Earlier, I was thinking we shouldn't commit ...
3 years, 8 months ago (2017-03-31 08:24:46 UTC) #13
pwnall
jbroman: Can you please review the code? jsbell: Earlier, I was thinking we shouldn't commit ...
3 years, 8 months ago (2017-03-31 08:24:47 UTC) #14
jbroman
lgtm; this seems better to me than crashing. (might want to wait for jsbell to ...
3 years, 8 months ago (2017-03-31 18:42:59 UTC) #18
pwnall
Thank you very much for the quick and thorough review! I'll check in with jsbell ...
3 years, 8 months ago (2017-03-31 22:50:24 UTC) #21
jsbell
https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp (right): https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp#newcode403 third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:403: // sub-optimal, as it can cause IndexedDB to return ...
3 years, 8 months ago (2017-04-03 19:31:53 UTC) #25
pwnall
jsbell: While addressing your comments, I saw the potential for more crashes, so I fixed ...
3 years, 8 months ago (2017-04-04 00:55:13 UTC) #32
jsbell
lgtm with comment nits https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp (right): https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp#newcode419 third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:419: return v8::Local<v8::Value>(); Aside: I wonder ...
3 years, 8 months ago (2017-04-04 16:42:34 UTC) #35
pwnall
Thank you very much for the feedback! https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp (right): https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp#newcode419 third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:419: return v8::Local<v8::Value>(); ...
3 years, 8 months ago (2017-04-04 22:41:42 UTC) #39
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/2781273004/180001
3 years, 8 months ago (2017-04-05 02:03:37 UTC) #44
commit-bot: I haz the power
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/cdd55622ffec06c7513b366777fe53566aabde4f
3 years, 8 months ago (2017-04-05 02:11:51 UTC) #47
jsbell
3 years, 8 months ago (2017-04-05 17:56:07 UTC) #48
Message was sent while issue was closed.
https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:419:
return v8::Local<v8::Value>();
On 2017/04/04 22:41:42, pwnall wrote:
> Would you like a follow-up CL?

Tracking bug is fine. 

(Without exploring all of the code, this seems like there should be an assertion
here - we should never fail to convert the key to a value. Maybe it comes out of
handling array buffer allocation failures?)

Powered by Google App Engine
This is Rietveld 408576698