Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773823002:
Use a two-part data format version in IndexedDB metadata. (Closed)
|
Created:
3 years, 9 months ago by jbroman Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, dmurph, jsbell+idb_chromium.org, kinuko+watch, pwnall, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse a two-part data format version in IndexedDB metadata.
This breaks the 64-bit data format version stored in the database metadata
into two 32-bit parts (V8 and Blink format versions), and allows them to be
compared by checking that the database uses supported versions of both.
If either component has reverted, the database is deemed to be from the future
(i.e. corrupt).
BUG=704293
Review-Url: https://codereview.chromium.org/2773823002
Cr-Commit-Position: refs/heads/master@{#466002}
Committed: https://chromium.googlesource.com/chromium/src/+/3ee0c8ab5180fb87ce012b784bb47a3da507c1a0
Patch Set 1 #Patch Set 2 : correct path #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : partially revert test change #
Total comments: 8
Patch Set 7 : jsbell, cmumford #
Total comments: 8
Patch Set 8 : compile-time #Patch Set 9 : move to content/browser/ #Patch Set 10 : static #
Total comments: 4
Patch Set 11 : base::MakeShared #
Total comments: 3
Patch Set 12 : fix comment #
Total comments: 2
Patch Set 13 : merge with origin/master #Patch Set 14 : rename blink constants #
Messages
Total messages: 100 (66 generated)
The CQ bit was checked by jbroman@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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbroman@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jbroman@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: This issue passed the CQ dry run.
The CQ bit was checked by jbroman@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: This issue passed the CQ dry run.
The CQ bit was checked by jbroman@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 checked by jbroman@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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by jbroman@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...
jbroman@chromium.org changed reviewers: + jsbell@chromium.org
This change requires extra scrutiny because it deals with the schema logic (and so causes the "throw away the LevelDB store" behavior). This should end up being essentially equivalent to our current logic, but: - the version now comes from the client (this means that clever tricks to get the version number to exist in the browser process aren't needed, but means that a renderer could intentionally specify a future data version number and discard the backing store -- this seems okay, because it seems that such a client could also drop the IndexedDB database anyhow) - there's additional plumbing - I'm not sure how this sort of change is usually tested in your code; I couldn't find the tests for the existing logic. With respect to the underlying issue, this should restore the previous non-crashy behaviour, but it does mean that (IIUC) in the case of a version being rolled back (either in a branch or because a user downgraded Chrome), they would lose their database rather than getting a crash but being able to recover the data by switching back. It seems that this is the behavior that was consciously chosen earlier, but we should probably consider this carefully (and this CL is a starting point).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... File content/common/indexed_db/indexed_db_struct_traits.cc (right): https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_struct_traits.cc:256: }; Delete semicolon.
lgtm ; still pondering the TODOs around how to fail. https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... File content/common/indexed_db/indexed_db_data_format_version.h (right): https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:40: // Encodes and decodes the tuple from an int64_t. Can you update content/browser/indexed_db/leveldb_coding_scheme.md look for DataVersionKey - maybe just a pointer to this file and/or a note that the top/bottom 32 bits are used for ... https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:42: // the v8 version would overflow int32_t. We check both, to be consistent. Also note that the encoding scheme is chosen such that old records that serialized only the blink version as an int64_t decode correctly. https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:48: static IndexedDBDataFormatVersion Decode(int64_t encoded) { DCHECK_GE(encoded, 0) ?
On 2017/03/29 21:13:22, jbroman wrote: > It seems that this is the behavior that was consciously chosen earlier, but we > should probably consider this carefully (and this CL is a starting point). Yes, we settled on that behavior consciously because otherwise the web app becomes broken from the user's perspective - it will fail to open the database and can't even initiate a recover (since 'deleteDatabase' is similarly blocked). This is something we could revisit at some point in time, but agreed that keeping the behavior w/ this CL is the right starting point.
jbroman@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for SECURITY_OWNERS (mojom change) My TODOs for how we'd like to handle the error are poorly worded so that we pick a side at least for now. If we're happy with LOG(ERROR)ing and continuing, I can leave that (and add a better comment). I'll wait to land until we have something we're happy with for now. A malicious client could give incorrect or inconsistent versions to the service, but I'm not sure this empowers them to do anything but delete databases (which I think they can already do, if I understand correctly). https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... File content/common/indexed_db/indexed_db_data_format_version.h (right): https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:40: // Encodes and decodes the tuple from an int64_t. On 2017/04/03 at 18:08:26, jsbell wrote: > Can you update content/browser/indexed_db/leveldb_coding_scheme.md > > look for DataVersionKey - maybe just a pointer to this file and/or a note that the top/bottom 32 bits are used for ... Done. https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:42: // the v8 version would overflow int32_t. We check both, to be consistent. On 2017/04/03 at 18:08:26, jsbell wrote: > Also note that the encoding scheme is chosen such that old records that serialized only the blink version as an int64_t decode correctly. Done. https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:48: static IndexedDBDataFormatVersion Decode(int64_t encoded) { On 2017/04/03 at 18:08:26, jsbell wrote: > DCHECK_GE(encoded, 0) ? Done. https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... File content/common/indexed_db/indexed_db_struct_traits.cc (right): https://codereview.chromium.org/2773823002/diff/100001/content/common/indexed... content/common/indexed_db/indexed_db_struct_traits.cc:256: }; On 2017/04/03 at 16:55:02, cmumford wrote: > Delete semicolon. Done.
The CQ bit was checked by jbroman@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...
https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:102: // TODO(jbroman): We could take mojo::GetBadMessageCallback here to tell the If this is happening w/in dispatch, we should be able to call mojo::ReportBadMessage() directly. Here's an idea for error handling: * Change client_data_format_version_ to be used only on BrowserThread::IO * Check version here, and call mojo::ReportBadMessage()/early return if changed. * Check in GetDatabaseNames/Open/DeleteDatabase (the non-OnIDBThread versions) and call mojo::ReportBadMessage()/early return if not set. * Pass the version through to the *OnIDBThread methods as a new argument.
https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.h (right): https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.h:379: const IndexedDBDataFormatVersion data_format_version() const { Nit: no const and return by value, or const and return by ref. https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:175: if (!client_data_format_version_) Since this is coming from the renderer, we do need to check that the specified values are legal (e.g. both v8 version and blink version are <= std::numeric_limits<int32_t>::max()). But I think I would strongly prefer it if we could someone make this compile-time... https://codereview.chromium.org/2773823002/diff/120001/content/browser/indexe... content/browser/indexed_db/indexed_db_dispatcher_host.cc:178: LOG(ERROR) << "IndexedDB client's supported data format version changed!"; Nit: DLOG instead of LOG? I don't think this will be helpful to people that aren't developers. https://codereview.chromium.org/2773823002/diff/120001/content/common/indexed... File content/common/indexed_db/indexed_db.mojom (right): https://codereview.chromium.org/2773823002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db.mojom:337: // version is fixed at runtime. It kind of feels like this could even bound at compile time--it doesn't seem like either the v8 or blink version should be changing past that point? https://codereview.chromium.org/2773823002/diff/120001/content/common/indexed... File content/common/indexed_db/indexed_db_data_format_version.h (right): https://codereview.chromium.org/2773823002/diff/120001/content/common/indexed... content/common/indexed_db/indexed_db_data_format_version.h:41: // Since negative values are considered invalid, this scheme will only work Nit: I think there's a missing word or two here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm probably just missing something. I'm wondering why each Render process really needs to inform, at runtime, the browser process of these two versions - aren't they known at compile time? I know we don't want content to be including private header files, but what about putting two new constants in a public header, and then using static_assert to guarantee that the private (SerializedScriptValue::wireFormatVersion) and public constant values are equal. This way we could avoid a lot of complexity - specifically the situations where calls are made (like GetDatabaseNames) possibly before the supported versions are known.
On 2017/04/04 at 22:50:18, cmumford wrote: > I'm probably just missing something. I'm wondering why each Render process really needs to inform, at runtime, the browser process of these two versions - aren't they known at compile time? I know we don't want content to be including private header files, but what about putting two new constants in a public header, and then using static_assert to guarantee that the private (SerializedScriptValue::wireFormatVersion) and public constant values are equal. This way we could avoid a lot of complexity - specifically the situations where calls are made (like GetDatabaseNames) possibly before the supported versions are known. Today we do indeed do this at compile time, for the Blink version. What makes it more complicated is that one of the constants is in Blink, and one is in V8. Since we can't put both constants in one header (unless we want to turn every V8-side wire format change into a three-sided patch, which would be a mess), we'd have to replicate this pattern there as well, and also override the checking that should prevent v8 use in the browser. I think this approach is conceptually cleaner, but I do appreciate that it adds some of these...edge cases.
On 2017/04/04 at 23:22:13, jbroman wrote: > On 2017/04/04 at 22:50:18, cmumford wrote: > > I'm probably just missing something. I'm wondering why each Render process really needs to inform, at runtime, the browser process of these two versions - aren't they known at compile time? I know we don't want content to be including private header files, but what about putting two new constants in a public header, and then using static_assert to guarantee that the private (SerializedScriptValue::wireFormatVersion) and public constant values are equal. This way we could avoid a lot of complexity - specifically the situations where calls are made (like GetDatabaseNames) possibly before the supported versions are known. > > Today we do indeed do this at compile time, for the Blink version. > > What makes it more complicated is that one of the constants is in Blink, and one is in V8. Since we can't put both constants in one header (unless we want to turn every V8-side wire format change into a three-sided patch, which would be a mess), we'd have to replicate this pattern there as well, and also override the checking that should prevent v8 use in the browser. > > I think this approach is conceptually cleaner, but I do appreciate that it adds some of these...edge cases. (To be clear, I'm fine with talking to V8 folk about exposing an additional header to include this version number in that way, if that's what people would prefer. Certainly the arguments in favour of that are strong.)
On 2017/04/04 23:23:04, jbroman wrote: > On 2017/04/04 at 23:22:13, jbroman wrote: > > On 2017/04/04 at 22:50:18, cmumford wrote: > > > I'm probably just missing something. I'm wondering why each Render process > really needs to inform, at runtime, the browser process of these two versions - > aren't they known at compile time? I know we don't want content to be including > private header files, but what about putting two new constants in a public > header, and then using static_assert to guarantee that the private > (SerializedScriptValue::wireFormatVersion) and public constant values are equal. > This way we could avoid a lot of complexity - specifically the situations where > calls are made (like GetDatabaseNames) possibly before the supported versions > are known. > > > > Today we do indeed do this at compile time, for the Blink version. > > > > What makes it more complicated is that one of the constants is in Blink, and > one is in V8. Since we can't put both constants in one header (unless we want to > turn every V8-side wire format change into a three-sided patch, which would be a > mess), we'd have to replicate this pattern there as well, and also override the > checking that should prevent v8 use in the browser. > > > > I think this approach is conceptually cleaner, but I do appreciate that it > adds some of these...edge cases. > > (To be clear, I'm fine with talking to V8 folk about exposing an additional > header to include this version number in that way, if that's what people would > prefer. Certainly the arguments in favour of that are strong.) That's what I'd prefer. It would let us avoid the question of error handling completely.
https://codereview.chromium.org/2773823002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSerializedScriptValueVersion.h (right): https://codereview.chromium.org/2773823002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSerializedScriptValueVersion.h:52: uint32_t v8Version = 0; v8 has a major and minor version (V8_MAJOR_VERSION & V8_MINOR_VERSION). How will they map to v8Version?
https://codereview.chromium.org/2773823002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebSerializedScriptValueVersion.h (right): https://codereview.chromium.org/2773823002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebSerializedScriptValueVersion.h:52: uint32_t v8Version = 0; On 2017/04/05 at 21:23:18, cmumford wrote: > v8 has a major and minor version (V8_MAJOR_VERSION & V8_MINOR_VERSION). How will they map to v8Version? V8 has a wire format version, which is analogous to Blink's. I recently exposed it as v8::ValueSerializer::GetCurrentDataFormatVersion(), though that's only available at runtime, so it'd have to move to be a header constant to satisfy the concerns you and dcheng have. The V8 published version number has to do with the V8 release process, and isn't great for this purpose. (Similarly, we don't use the Chromium milestone.minor.branch.patch version for the Blink-side versioning.)
jbroman@ here's what I'm suggesting - see: https://codereview.chromium.org/2798083003/ With an approach like the one above, you can have a compile-time dependency on both version numbers as header constants, and you don't need to modify v8 or expose any private headers. The static_assert's guarantee that the constant values are always correct. Or, if the caller is currently linking with blink_web, then you could convert WebSerializedScriptValueVersion::blinkVersion into a static method that just returns SerializedScriptValue::wireFormatVersion. I think that either of these two approaches will eliminate the need pass these values from renderer → browser (reducing complexity), but most importantly eliminates the likelihood that a database would ever be opened before the versions have been received - which is a current problem with patch set 7.
On 2017/04/05 at 21:38:11, cmumford wrote: > jbroman@ here's what I'm suggesting - see: https://codereview.chromium.org/2798083003/ Yes, I understand what you're proposing. > With an approach like the one above, you can have a compile-time dependency on both version numbers as header constants, and you don't need to modify v8 or expose any private headers. The static_assert's guarantee that the constant values are always correct. > > Or, if the caller is currently linking with blink_web, then you could convert WebSerializedScriptValueVersion::blinkVersion into a static method that just returns SerializedScriptValue::wireFormatVersion. > > I think that either of these two approaches will eliminate the need pass these values from renderer → browser (reducing complexity), but most importantly eliminates the likelihood that a database would ever be opened before the versions have been received - which is a current problem with patch set 7. 1. When I say "the version in V8", I don't mean "the version of the V8 library" (which follows the Chromium milestone anyhow -- V8 5.9 is the version of V8 that will ship with Chromium 59, and so on). The constant in V8 is a different one (kLatestVersion in v8/src/value-serializer.cc), which isn't currently statically exposed in a public V8 header. 2. Putting a constant that we assert (statically or otherwise) matches one in the Chromium repository will make rolling V8 changes a pain. What will happen is: - V8 wire format version increases - V8 roll bot tries to update Blink DEPS to a new revision of V8 - trybots fail due to static_assert failure Preventing this requires three-sided patches, which are rather painful. I could certainly mimic that pattern in a v8 public header (the difference being that it's in the same repository as the v8 implementation), which is what I'm currently planning on doing based on this feedback.
Thanks jbroman@. Great explanation and I now understand your predicament.
The CQ bit was checked by jbroman@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 checked by jbroman@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...
Description was changed from ========== Send a data format version tuple to IndexedDB from the renderer. BUG=704293 ========== to ========== Use a two-part data format version in IndexedDB metadata. This breaks the 64-bit data format version stored in the database metadata into two 32-bit parts (V8 and Blink format versions), and allows them to be compared by checking that the database uses supported versions of both. If either component has reverted, the database is deemed to be from the future (i.e. corrupt). BUG=704293 ==========
The CQ bit was checked by jbroman@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...
PTAL; this reverts to computing this at compile time. Will require follow-ups: V8: https://codereview.chromium.org/2804643006 Chromium: https://codereview.chromium.org/2801053003 If this looks reasonable to you, I'll ask for a review of the V8 side (and not land until I can get everyone on the same page :P).
https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... File content/browser/indexed_db/indexed_db_data_format_version.cc (right): https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... content/browser/indexed_db/indexed_db_data_format_version.cc:14: blink::kSerializedScriptValueVersion); Is there a guarantee that this will avoid runtime initialization?
https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... File content/browser/indexed_db/indexed_db_data_format_version.cc (right): https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... content/browser/indexed_db/indexed_db_data_format_version.cc:14: blink::kSerializedScriptValueVersion); On 2017/04/06 at 21:14:19, dcheng wrote: > Is there a guarantee that this will avoid runtime initialization? The type has a constexpr constructor, so it's certainly permitted to avoid it. I've checked that at least gcc and clang do (indeed it's _really_ easy for them to). It's a little more work for me to check MSVC, but I'd be surprised if it didn't do the right thing here. Technically I'm not sure the language forbids compilers from putting runtime initialization for any variable with static initialization if it wants, so I don't know how I can guarantee this other than saying that I've checked two compilers, and I'd expect the sizes alarm to go off if I'm wrong. I can't make the whole thing constexpr because I actually want this to be a mutable static field, for tests. But this should compile down to the obvious thing (8 bytes in the .data section or equivalent).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-owner lgtm -- this approach seems reasonable to me. https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... File content/browser/indexed_db/indexed_db_factory_unittest.cc (right): https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... content/browser/indexed_db/indexed_db_factory_unittest.cc:551: auto db_callbacks = make_scoped_refptr(new MockIndexedDBDatabaseCallbacks); Another option is to call base::MakeShared (Maybe we should have named this something different... meh)
The CQ bit was checked by jbroman@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...
https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... File content/browser/indexed_db/indexed_db_factory_unittest.cc (right): https://codereview.chromium.org/2773823002/diff/180001/content/browser/indexe... content/browser/indexed_db/indexed_db_factory_unittest.cc:551: auto db_callbacks = make_scoped_refptr(new MockIndexedDBDatabaseCallbacks); On 2017/04/07 at 06:36:04, dcheng wrote: > Another option is to call base::MakeShared > > (Maybe we should have named this something different... meh) Ah, I missed that (didn't expect it to be called MakeShared given it's not called shared_ptr). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.cc:119: const int64_t kLatestKnownSchemaVersion = 3; Just calling out: we're explicitly NOT bumping this here. Old code would see the high bits of the version and assume it's just a very high SSV version and fail the IsSchemaKnown test, which is desired. If we bump this number we'd need to write it in SetUpMetadata() but there's no actual change to what we'd persist. https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_data_format_version.h (right): https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_data_format_version.h:48: // Since negative values are considered invalid, this scheme will only work Seems like there's a word or two missing in this sentence.
The CQ bit was checked by jbroman@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...
https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... File content/browser/indexed_db/indexed_db_data_format_version.h (right): https://codereview.chromium.org/2773823002/diff/200001/content/browser/indexe... content/browser/indexed_db/indexed_db_data_format_version.h:48: // Since negative values are considered invalid, this scheme will only work On 2017/04/07 at 16:30:01, jsbell wrote: > Seems like there's a word or two missing in this sentence. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
jsbell, cmumford: double-checking that this approach lgty before I ask for v8-side review
On 2017/04/10 14:11:34, jbroman wrote: > jsbell, cmumford: double-checking that this approach lgty before I ask for > v8-side review Yep, lgtm
Apologies for taking so long - was out Friday. https://codereview.chromium.org/2773823002/diff/220001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2773823002/diff/220001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.cc:1140: WARN_UNUSED_RESULT Status IndexedDBBackingStore::SetUpMetadata() { The metadata version is set when the database is first created when IndexedDBBackingStore::Create() calls IndexedDBBackingStore::SetUpMetadata() and the metadata version isn't changed afterwards. If the serialized script version is written at create time then new serialized scripts can be written later on when that version has possibly changed. Have I overlooked something?
https://codereview.chromium.org/2773823002/diff/220001/content/browser/indexe... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/2773823002/diff/220001/content/browser/indexe... content/browser/indexed_db/indexed_db_backing_store.cc:1140: WARN_UNUSED_RESULT Status IndexedDBBackingStore::SetUpMetadata() { On 2017/04/10 at 17:35:12, cmumford wrote: > The metadata version is set when the database is first created when IndexedDBBackingStore::Create() calls IndexedDBBackingStore::SetUpMetadata() and the metadata version isn't changed afterwards. If the serialized script version is written at create time then new serialized scripts can be written later on when that version has possibly changed. > > Have I overlooked something? I'm mostly following the existing code, but my understanding is that IndexedDBBackingStore::Create is called to create the backing store object in memory, regardless of whether a new database is being created or an existing database is being opened. If that were not the case, there's a great deal of logic in SetUpMetadata that wouldn't apply.
lgtm
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2773823002/#ps220001 (title: "fix comment")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jbroman@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jbroman@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...
jbroman@chromium.org changed reviewers: + jam@chromium.org
+jam for content/browser/BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/19 18:51:50, jbroman wrote: > +jam for content/browser/BUILD.gn lgtm maybe time to add a BUILD.gn file for that subdirectory? then you won't need to get content/browser/owners stamp
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cmumford@chromium.org, dcheng@chromium.org, jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2773823002/#ps260001 (title: "rename blink constants")
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": 260001, "attempt_start_ts": 1492698093976840,
"parent_rev": "a15a2d8062d9919bef96f18455a062a920f4568a", "commit_rev":
"3ee0c8ab5180fb87ce012b784bb47a3da507c1a0"}
Message was sent while issue was closed.
Description was changed from ========== Use a two-part data format version in IndexedDB metadata. This breaks the 64-bit data format version stored in the database metadata into two 32-bit parts (V8 and Blink format versions), and allows them to be compared by checking that the database uses supported versions of both. If either component has reverted, the database is deemed to be from the future (i.e. corrupt). BUG=704293 ========== to ========== Use a two-part data format version in IndexedDB metadata. This breaks the 64-bit data format version stored in the database metadata into two 32-bit parts (V8 and Blink format versions), and allows them to be compared by checking that the database uses supported versions of both. If either component has reverted, the database is deemed to be from the future (i.e. corrupt). BUG=704293 Review-Url: https://codereview.chromium.org/2773823002 Cr-Commit-Position: refs/heads/master@{#466002} Committed: https://chromium.googlesource.com/chromium/src/+/3ee0c8ab5180fb87ce012b784bb4... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3ee0c8ab5180fb87ce012b784bb4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
