|
|
DescriptionGraceful 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)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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), for object stores with non-empty key paths. This CL turns the crash into an exception, so the application code has a chance to work around the database unavailability issue. DO NOT LAND. BUG=703704 ========== to ========== 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. BUG=703704 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + jbroman@chromium.org
jbroman: Can you please review the code? jsbell: Earlier, I was thinking we shouldn't commit this patch, because of the potential for data corruption. After I figured out that we already produce null values in many new-version cases, I now think that we should commit the patch, and figure out throwing exceptions later. I think you tried to explain this to me when you described the problem... thank you for your patience with my slowness :D
jbroman: Can you please review the code? jsbell: Earlier, I was thinking we shouldn't commit this patch, because of the potential for data corruption. After I figured out that we already produce null values in many new-version cases, I now think that we should commit the patch, and figure out throwing exceptions later. I think you tried to explain this to me when you described the problem... thank you for your patience with my slowness :D
Description was changed from ========== 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. BUG=703704 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm; this seems better to me than crashing. (might want to wait for jsbell to weight in) That said, we should have detected this with the earlier logic (which I have a CL out for review now to hopefully fix). I'm not really certain whether we ought to be providing better promises about the compatibility of this data in the future (as "your database is now gone" is kinda a rough experience for users), but I'm also confident you storage folk have rehashed that discussion before. https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:131: v8::Isolate* isolate = scope->isolate(); nit: Just pass the isolate as an argument, rather than the scope? https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:157: ASSERT_EQ(static_cast<unsigned char>(13), wireData[2]); This will make the test fail whenever the v8 version bumps, which will make such changes difficult to roll into Chromium. Could a weaker expectation work? (Ideally we wouldn't be hard-coding the format of this header at all, though I'm not sure I have a fantastic workaround.) In the near future, V8 may expose this version in an API, but in the meantime it would probably suffice to just assert that it's at least 13. https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:333: // instead of returning null on deserialization errors. V8 does throw, but Blink throwing is a more complicated question, because there are a variety of APIs that can deserialize, and many do so lazily (e.g. deserialization of postMessage body happens during the MessageEvent data getter, which might be a surprising place for an exception to emerge) -- and would probably have interop implications.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for the quick and thorough review! I'll check in with jsbell before landing. He asked that I prepare the CL, but we did not discuss landing. https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:131: v8::Isolate* isolate = scope->isolate(); On 2017/03/31 18:42:59, jbroman wrote: > nit: Just pass the isolate as an argument, rather than the scope? Done. Thank you for catching this! I forgot to clean up after debugging. https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:157: ASSERT_EQ(static_cast<unsigned char>(13), wireData[2]); On 2017/03/31 18:42:59, jbroman wrote: > This will make the test fail whenever the v8 version bumps, which will make such > changes difficult to roll into Chromium. Could a weaker expectation work? > > (Ideally we wouldn't be hard-coding the format of this header at all, though I'm > not sure I have a fantastic workaround.) > > In the near future, V8 may expose this version in an API, but in the meantime it > would probably suffice to just assert that it's at least 13. Done. Thank you for the suggestion! This is a very reasonable way to handle the situation. https://codereview.chromium.org/2781273004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:333: // instead of returning null on deserialization errors. On 2017/03/31 18:42:59, jbroman wrote: > V8 does throw, but Blink throwing is a more complicated question, because there > are a variety of APIs that can deserialize, and many do so lazily (e.g. > deserialization of postMessage body happens during the MessageEvent data getter, > which might be a surprising place for an exception to emerge) -- and would > probably have interop implications. I made the comment here be less assertive. Thank you very much for the heads up regarding the complexity in changing deserialization. I haven't looked at this code for too long, so I have very little clue what's going on. (best situation to plan improvements? :D) IIUC, if serialization fails currently, we'll have a spurious message event whose data is null? Ideally, we'd not invoke the app's event handler if deserialization fails. Based on the little I know so far, that'd discard the scope, so the thrown exception would be ignored? (sorry if this is completely wrong, i'm still figuring things out) I didn't know that the postMessage spec covers situations where deserialization fails. I'll pester jsbell for details. Thank you for opening my eyes to all this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp (right): https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:403: // sub-optimal, as it can cause IndexedDB to return incorrect data. The check Maybe note that null is a perfectly valid IDB value, for readers unfamiliar with IDB. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:404: // below is the minimal work needed to avoid a crash. A proper solution would Since this comment tracks future work, rephrase as a TODO(username) and reference a crbug ? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:405: // require SerializedScriptValue to throw rather than returning null when it Return an empty handle? I wouldn't expect SSV APIs to throw (although serialization may throw if a getter throws) https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:407: if (value->primaryKey() && !v8Value->IsNull()) { I wonder if we want to have more comments/DCHECKs here for context: if primaryKey() is set then the record being retrieved is in an object store with a key path and key generator, and the object conceptually passed an assertPrimaryKeyValidOrInjectable() check when serialized therefore it's an object (not a primitive value) https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:151: wireData[0]); Can you make the offsets (0...3) consts somewhere? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:154: // 13 is v8::internal::kLatestVersion in v8/src/value-serializer.cc at the Define this as a const somewhere? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:323: // These steps reproduce the circumstances of http://crbug.com/703704 nit: prefer https Also, I'd put the explanation inline rather than just linking to the bug. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:326: objectBytes[2] += 1; Explain what this is doing. (Having the offset be a const will help) https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:331: // The exception check below will need to be changed if we throw exceptions I don't think you need this comment - it's implicit in the EXPECT (and we'll notice c/o test failures)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell: While addressing your comments, I saw the potential for more crashes, so I fixed everything. I updated this CL because covering all the cases removes the check that I added before, and I figured it doesn't make too much sense to add a check that'll get removed soon. PTAL? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp (right): https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:403: // sub-optimal, as it can cause IndexedDB to return incorrect data. The check On 2017/04/03 19:31:52, jsbell wrote: > Maybe note that null is a perfectly valid IDB value, for readers unfamiliar with > IDB. Done. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:404: // below is the minimal work needed to avoid a crash. A proper solution would On 2017/04/03 19:31:52, jsbell wrote: > Since this comment tracks future work, rephrase as a TODO(username) and > reference a crbug ? I used the crbug targeted by this CL, because the bug asks for the proper behavior. We can decide whether I'll still work on the bug past shipping this CL and fixing the crash. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:405: // require SerializedScriptValue to throw rather than returning null when it On 2017/04/03 19:31:52, jsbell wrote: > Return an empty handle? > > I wouldn't expect SSV APIs to throw (although serialization may throw if a > getter throws) Done. I switched to your suggestion of suggesting returning an empty handle :) I initially thought that an exception would be the right thing to do in order to produce different messages for different error conditions. "Version skew, did you use your profile with a newer version of Chrome?" vs "Invalid tag, your disk is dead." However, the value of these is low enough to not be worth the architectural ugliness. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:407: if (value->primaryKey() && !v8Value->IsNull()) { On 2017/04/03 19:31:52, jsbell wrote: > I wonder if we want to have more comments/DCHECKs here for context: if > primaryKey() is set then the record being retrieved is in an object store with a > key path and key generator, and the object conceptually passed an > assertPrimaryKeyValidOrInjectable() check when serialized therefore it's an > object (not a primitive value) I think that there is a check at a higher level -- see my note about the different crash stack when DCHECKs are on at https://bugs.chromium.org/p/chromium/issues/detail?id=703704#c4 If you'd like an extra DCHECK, it seems like it'd make most sense to DCHECK(v8Value.isObject()) right before the value.As<v8::Object>() call in injectV8KeyIntoV8Value(). WDYT? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:151: wireData[0]); On 2017/04/03 19:31:52, jsbell wrote: > Can you make the offsets (0...3) consts somewhere? Done. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:154: // 13 is v8::internal::kLatestVersion in v8/src/value-serializer.cc at the On 2017/04/03 19:31:52, jsbell wrote: > Define this as a const somewhere? Done. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:323: // These steps reproduce the circumstances of http://crbug.com/703704 On 2017/04/03 19:31:52, jsbell wrote: > nit: prefer https > > Also, I'd put the explanation inline rather than just linking to the bug. Done. (the https part) The explanation was getting rather long, so I distributed it across comments for the functions in V8BindingForModules.cpp. Does this look reasonable? https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:326: objectBytes[2] += 1; On 2017/04/03 19:31:52, jsbell wrote: > Explain what this is doing. (Having the offset be a const will help) Done. https://codereview.chromium.org/2781273004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:331: // The exception check below will need to be changed if we throw exceptions On 2017/04/03 19:31:52, jsbell wrote: > I don't think you need this comment - it's implicit in the EXPECT (and we'll > notice c/o test failures) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comment nits 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>(); Aside: I wonder if we test this code path? https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:542: // event of database corruption. maybe append "or undetected version skew." https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:131: // V8 serialization code. The code below DCHECKs that nit: truncated comment? https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:139: // its serialization version, so this number might get stale. TODO(jbroman): Update when it is exported by v8.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #5 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for the feedback! 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 16:42:33, jsbell wrote: > Aside: I wonder if we test this code path? Nope. I looked at it... then I thought of what I'd need to do to see what happens end-to-end... then I looked away :) Would you like a follow-up CL? https://codereview.chromium.org/2781273004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:542: // event of database corruption. On 2017/04/04 16:42:33, jsbell wrote: > maybe append "or undetected version skew." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2781273004/#ps180001 (title: "Addressed nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491357786543210, "parent_rev": "3a2b278dd6801cd159680fb848ce65b471f9658b", "commit_rev": "cdd55622ffec06c7513b366777fe53566aabde4f"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/cdd55622ffec06c7513b366777fe... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/cdd55622ffec06c7513b366777fe...
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?) |