Bug 107655 – [v8] prepare SerializedScriptValue for transition to Latin-1

• Component: New Bugs
• Status: RESOLVED FIXED

[Dan Carney]

[v8] prepare SerializedScriptValue for transition to Latin-1

[Dan Carney]

Created attachment 184190 [details]
Patch

[Kentaro Hara]

Comment on attachment 184190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184190&action=review

> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:13
> +var curVer = 0x02ff;

Nit: curVer => currentVersion

WebKit uses a fully qualified variable name.

> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:28
> -                  [ 0x02ff, 0x003f, 0x3f6f, 0x5301, 0x6603,
> +                  [ curVer, 0x003f, 0x3f6f, 0x5301, 0x6603,

Won't this change lose compatibility for already stored serialized script values (e.g. localStorage)? (I just wanted to confirm that it's OK to change the serialized results.)

[Dan Carney]

Comment on attachment 184190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184190&action=review

>> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:13
>> +var curVer = 0x02ff;
> 
> Nit: curVer => currentVersion
> 
> WebKit uses a fully qualified variable name.

I used curVer to have 6 characters and keep all the byte arrays nicely lined up.

>> LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:28
>> +                  [ curVer, 0x003f, 0x3f6f, 0x5301, 0x6603,
> 
> Won't this change lose compatibility for already stored serialized script values (e.g. localStorage)? (I just wanted to confirm that it's OK to change the serialized results.)

This change is backwards but not forwards compatible.  If i'm reading SerializedScriptValue.cpp correctly, this bumping the version number is the correct way to do this transition.

[Kentaro Hara]

(In reply to comment #3)
> I used curVer to have 6 characters and keep all the byte arrays nicely lined up.

Ah, makes sense.

> This change is backwards but not forwards compatible.  If i'm reading SerializedScriptValue.cpp correctly, this bumping the version number is the correct way to do this transition.

Looks like so.

jsbell, alecflett: would you check that the patch is safe in terms of compatibility?

[Dan Carney]

jsbell, alecflett: just to clarify what will happen.  Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf.  This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value.  Version 3 can read version 2 without issue.

[Joshua Bell]

(In reply to comment #5)
> jsbell, alecflett: just to clarify what will happen.  Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf.  This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value.  Version 3 can read version 2 without issue.

Thanks. Conceptually, this looks correct.

There is a problem, though - this conceptually *removes* test coverage for parsing v2 SSVs, since with this patch and once the new flag is defined, the test will only exercise parsing v3 SSVs.

You'll need to add tests that use the (optional) oldFormat param of _testSerialization and explicitly test 0x02ff, or expand testSerialization() to insert the version prefix since most v2 and v3 SSVs will serialize identically. I'm not sure which is better. Or perhaps oldFormat should turn into a map of version/serializations? Not sure what is best here.

(alecflett's not going to be able to look at this until tomorrow, unfortunately.)

[Dan Carney]

(In reply to comment #6)
> (In reply to comment #5)
> > jsbell, alecflett: just to clarify what will happen.  Once V8_ONE_BYTE_STRINGS_ENABLED is defined in v8, one byte strings will now potentially have payloads with bytes > 0xf.  This was previously impossible, so a version 2 SSV should not be allowed to read from a version 3 value.  Version 3 can read version 2 without issue.
> 
> Thanks. Conceptually, this looks correct.
> 
> There is a problem, though - this conceptually *removes* test coverage for parsing v2 SSVs, since with this patch and once the new flag is defined, the test will only exercise parsing v3 SSVs.
> 
> You'll need to add tests that use the (optional) oldFormat param of _testSerialization and explicitly test 0x02ff, or expand testSerialization() to insert the version prefix since most v2 and v3 SSVs will serialize identically. I'm not sure which is better. Or perhaps oldFormat should turn into a map of version/serializations? Not sure what is best here.
> 
> (alecflett's not going to be able to look at this until tomorrow, unfortunately.)

Good point.  I'll work something into the test to test both v2 and v3, including something that differs between v2 and v3.

[Dan Carney]

Created attachment 184470 [details]
Patch

[Dan Carney]

After looking into it, I realized that only utf8 and two byte strings are supported by the current system.  I've adjusted the patch to handle this, without needing to bump the version.  It comes at a performance cost, but once v8 latin-1 support is stable, I'll implement a version 3 SSV which handles one byte strings directly, without the need to unnecessarily jump in and out of utf8.

[Alec Flett]

Yes, the test should be purely additive - all existing tests should remain or be tweaked to indicate that its testing an old version (to avoid the bidirectional serialization tests for those)

(The "old version" parameter should probably be tweaked to take a list of old versioned data, so we can keep adding to the test with each subsequent version)

[WebKit Review Bot]

Comment on attachment 184470 [details]
Patch

Attachment 184470 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16119005

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Kentaro Hara]

jsbell: Does it look fine?

[Build Bot]

Comment on attachment 184470 [details]
Patch

Attachment 184470 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16112351

New failing tests:
svg/as-image/img-preserveAspectRatio-support-2.html

[Joshua Bell]

Comment on attachment 184470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184470&action=review

Looks fine to me.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:324
> +        ensureSpace(utf8Length);

Could move this up and use ensureSpace(sizeof(StringTag) + sizeof(uint32_t) + utf8Length) (probably premature optimization, though)

[Kentaro Hara]

Comment on attachment 184470 [details]
Patch

Please apply jsbell's comment before landing.

[Dan Carney]

(In reply to comment #15)
> (From update of attachment 184470 [details])
> Please apply jsbell's comment before landing.

kentaro, I'm marking this cq? for now as I have no access to a dev environment this weekend.  I have to do a bunch of optimization in this class after latin-1 is stable in v8, so I'll apply jsbell's optimization there instead.

[WebKit Review Bot]

Comment on attachment 184470 [details]
Patch

Clearing flags on attachment: 184470

Committed r140911: <http://trac.webkit.org/changeset/140911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #9)
> After looking into it, I realized that only utf8 and two byte strings are supported by the current system.

huh?

>  I've adjusted the patch to handle this, without needing to bump the version.  It comes at a performance cost, but once v8 latin-1 support is stable, I'll implement a version 3 SSV which handles one byte strings directly, without the need to unnecessarily jump in and out of utf8.

And did we file about that somewhere?

[Ryosuke Niwa]

This bug is clearly dead since we've removed V8 from WebKit.

[noel gordon]

Remove yourself from the bug then, seeing as it's dead to you.