Future-proof data format used for LocalStorage.

Future-proof data format used for LocalStorage.

To enable future changes to the way data is stored in localstorage without
requiring a full migration of all existing data (for example adding
compression, or simply storing 8-bit only strings more efficiently), add
a tag to each encoded string to indicate the data format used.

BUG=640707
Review-Url: https://codereview.chromium.org/2748023008
Cr-Commit-Position: refs/heads/master@{#458614}
Committed: https://chromium.googlesource.com/chromium/src/+/f8d3cf5cfc015e0cbffc2a1fa65b6b96f85fe353

[Marijn Kruisselbrink, 2017-03-16 21:32:22]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 21:33:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 22:27:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 22:27:32]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-03-16 23:02:24]

Description was changed from

==========
Future-proof data format used for LocalStorage.

To enable future changes to the way data is stored in localstorage (for
example adding compression, or simply storing 8-bit only strings more
efficiently), add a tag to each encoded string to indicate the data format
used.

BUG=640707
==========

to

==========
Future-proof data format used for LocalStorage.

To enable future changes to the way data is stored in localstorage without
requiring a full migration of all existing data (for example adding
compression, or simply storing 8-bit only strings more efficiently), add
a tag to each encoded string to indicate the data format used.

BUG=640707
==========

[Marijn Kruisselbrink, 2017-03-16 23:02:34]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2017-03-18 01:00:30]

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
content/renderer/dom_storage/local_storage_cached_area.cc:33: CHECK_EQ(input[0],
static_cast<uint8_t>(StorageFormat::UTF16));
Not sure we should crash the renderer here? Most users won't figure out how to
fix it.

For this to happen, either we downgrading to an older version of chrome or
there's corruption in database (assuming its not memory corruption of some
kind). Is that right? Is the version downgrade case detected farther upstream?

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
content/renderer/dom_storage/local_storage_cached_area.cc:36:
std::memcpy(reinterpret_cast<void*>(&result[0]), input.data() + 1,
is there a reason to use memcpy instead of a string16 ctor directly?

[Marijn Kruisselbrink, 2017-03-21 21:37:14]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:37:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 22:31:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:31:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marijn Kruisselbrink, 2017-03-21 23:16:29]

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
content/renderer/dom_storage/local_storage_cached_area.cc:33: CHECK_EQ(input[0],
static_cast<uint8_t>(StorageFormat::UTF16));
On 2017/03/18 at 01:00:30, michaeln wrote:
> Not sure we should crash the renderer here? Most users won't figure out how to
fix it.
> 
> For this to happen, either we downgrading to an older version of chrome or
there's corruption in database (assuming its not memory corruption of some
kind). Is that right? Is the version downgrade case detected farther upstream?

Or compromised renderers, yes. So yeah, crashing the renderer is probably not
the right thing to do (even though that apparently is what indexeddb currently
does with data it can't deserialize). Changed it to for now just silently ignore
corrupt data, and added a TODO to do something better.

https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
content/renderer/dom_storage/local_storage_cached_area.cc:36:
std::memcpy(reinterpret_cast<void*>(&result[0]), input.data() + 1,
On 2017/03/18 at 01:00:30, michaeln wrote:
> is there a reason to use memcpy instead of a string16 ctor directly?

I believe that using a string16 ctor directly would be potentially undefined
behavior, as it would involve reading a base::char16 from an address that is
almost certainly not correctly aligned for such a type. Having said that, I
believe that on all platforms and compilers we support it would actually work
correctly, but using memcpy seemed cleaner, since (I think) it's guaranteed to
work correctly.

[michaeln, 2017-03-22 00:35:43]

On 2017/03/21 23:16:29, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
> File content/renderer/dom_storage/local_storage_cached_area.cc (right):
> 
>
https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
> content/renderer/dom_storage/local_storage_cached_area.cc:33:
CHECK_EQ(input[0],
> static_cast<uint8_t>(StorageFormat::UTF16));
> On 2017/03/18 at 01:00:30, michaeln wrote:
> > Not sure we should crash the renderer here? Most users won't figure out how
to
> fix it.
> > 
> > For this to happen, either we downgrading to an older version of chrome or
> there's corruption in database (assuming its not memory corruption of some
> kind). Is that right? Is the version downgrade case detected farther upstream?
> 
> Or compromised renderers, yes. So yeah, crashing the renderer is probably not
> the right thing to do (even though that apparently is what indexeddb currently
> does with data it can't deserialize). Changed it to for now just silently
ignore
> corrupt data, and added a TODO to do something better.

thnx

>
https://codereview.chromium.org/2748023008/diff/1/content/renderer/dom_storag...
> content/renderer/dom_storage/local_storage_cached_area.cc:36:
> std::memcpy(reinterpret_cast<void*>(&result[0]), input.data() + 1,
> On 2017/03/18 at 01:00:30, michaeln wrote:
> > is there a reason to use memcpy instead of a string16 ctor directly?
> 
> I believe that using a string16 ctor directly would be potentially undefined
> behavior, as it would involve reading a base::char16 from an address that is
> almost certainly not correctly aligned for such a type. Having said that, I
> believe that on all platforms and compilers we support it would actually work
> correctly, but using memcpy seemed cleaner, since (I think) it's guaranteed to
> work correctly.

lgtm, i'm glad i asked!

[Marijn Kruisselbrink, 2017-03-22 00:41:52]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-03-22 00:42:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 01:44:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490143312839850,
"parent_rev": "2625374404695b3c1c5e62c0b7921fdde1395c7c", "commit_rev":
"f8d3cf5cfc015e0cbffc2a1fa65b6b96f85fe353"}

[commit-bot: I haz the power, 2017-03-22 01:45:36]

Description was changed from

==========
Future-proof data format used for LocalStorage.

To enable future changes to the way data is stored in localstorage without
requiring a full migration of all existing data (for example adding
compression, or simply storing 8-bit only strings more efficiently), add
a tag to each encoded string to indicate the data format used.

BUG=640707
==========

to

==========
Future-proof data format used for LocalStorage.

To enable future changes to the way data is stored in localstorage without
requiring a full migration of all existing data (for example adding
compression, or simply storing 8-bit only strings more efficiently), add
a tag to each encoded string to indicate the data format used.

BUG=640707

Review-Url: https://codereview.chromium.org/2748023008
Cr-Commit-Position: refs/heads/master@{#458614}
Committed:
https://chromium.googlesource.com/chromium/src/+/f8d3cf5cfc015e0cbffc2a1fa65b...
==========

[commit-bot: I haz the power, 2017-03-22 01:45:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f8d3cf5cfc015e0cbffc2a1fa65b...