Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by pwnall
Modified:
2 months, 3 weeks 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. #

Messages

Total messages: 48 (37 generated)
pwnall
jbroman: Can you please review the code? jsbell: Earlier, I was thinking we shouldn't commit ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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>(); ...
2 months, 3 weeks 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
2 months, 3 weeks 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
2 months, 3 weeks ago (2017-04-05 02:11:51 UTC) #47
jsbell
2 months, 3 weeks 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?)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318