SerializedScriptValue uses of v8::String::Utf8Value stringValue(value) is hot code transferring strings to/from workers.
Created attachment 174155 [details] Patch
Comment on attachment 174155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174155&action=review > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:334 > + doWriteUint32(static_cast<uint32_t>(length * 2)); 2 -> sizeof(UChar) ? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:335 > + ensureSpace(length * 2); Maybe we should put "length * 2" into a local variable since we use it a bunch? Perhaps size?
I filed https://bugs.webkit.org/show_bug.cgi?id=102243 to track the issue this will cause with IndexedDB. We could land this first, but ideally 102243 would land at the same time or within a few days.
wireFormatVersion (line 244) needs to be incremented.
Please wait for bug 96818 so you have a place to add tests and support backwards-compatible changes.
(In reply to comment #4) > wireFormatVersion (line 244) needs to be incremented. Considering bug 72198 and bug 76803, both of which added new serialization tag definitions, then why increment? Seems we've never done so before.
(In reply to comment #6) > (In reply to comment #4) > > wireFormatVersion (line 244) needs to be incremented. > > Considering bug 72198 and bug 76803, both of which added new serialization tag definitions, then why increment? Seems we've never done so before. The scenario we need to account for is: * User installs build N+1 * User visits site using IndexedDB which writes SSVs to disk * User "downgrades" to build N, without removing the "profile" (cookies, DBs, etc) * User visits site; IndexedDB opens the backing store; some SSVs can't be parsed, surfaced as seemingly random data errors or inconsistencies This scenario is unfortunately common enough that we've had to account for it by detecting the downgrade scenario. Our response in the IndexedDB code is rather severe (blow away the database) but the alternative is to block that origin's use of IDB, or surface what appears to web apps as data corruption. The frequency of this scenario was not apparent to us until recently, or we would have implemented code such as https://bugs.webkit.org/show_bug.cgi?id=102243 earlier. Suggestions for alternate approaches are welcome.
Thanks Josuha, I suppose IDB is like a really big cookie and users can blow their cookies (and IDB's) away any time. Applications have to deal with it: IDB storage is persistent, but it's not permanent.
(In reply to comment #2) > (From update of attachment 174155 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174155&action=review > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:334 > > + doWriteUint32(static_cast<uint32_t>(length * 2)); > > 2 -> sizeof(UChar) ? Done. > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:335 > > + ensureSpace(length * 2); > > Maybe we should put "length * 2" into a local variable since we use it a bunch? Perhaps size? Good suggestion, I used size. I also had to account for the length being VarInt encoded when working out if a paddingTag should be added.
Created attachment 175667 [details] Patch
Comment on attachment 175667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175667&action=review > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:247 > +// Version 1: Version numbers manifested in space-time. How about Version 0: Initial version, without explicit support for version numbers. Version 1: support for object references (cycle detection) Version 2:... > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:149 > +var unicodeObject = {a: 'a', u: String.fromCharCode(0x03B1,0x03B2)}; you're going to need to add a backwards-compatible test here too - you'll have to run the test without your patch to generate the value first, then set the 3rd parameter to testSerialization to 'true'
(In reply to comment #11) > (From update of attachment 175667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175667&action=review > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:247 > > +// Version 1: Version numbers manifested in space-time. > > How about > Version 0: Initial version, without explicit support for version numbers. > Version 1: support for object references (cycle detection) > Version 2:... How about (refer comment 6) I just write // Version 2: Added StringUCharTag for UChar v8 strings. since that's the only real fact I have. "support for object references (cycle detection)" does not cover what happened before version 2.
Created attachment 176668 [details] Patch
(In reply to comment #11) > > LayoutTests/platform/chromium/fast/storage/serialized-script-value.html:149 > > +var unicodeObject = {a: 'a', u: String.fromCharCode(0x03B1,0x03B2)}; > > you're going to need to add a backwards-compatible test here too - you'll have to run the test without your patch to generate the value first, then set the 3rd parameter to testSerialization to 'true' ok, I grabbed data for the test without my patch, and added that old data as the third argument of the test. Didn't use a 'true', hope this is correct.
(In reply to comment #14) Also fixed the existing test on http://trac.webkit.org/changeset/136085 so I could grab the data.
Created attachment 176875 [details] Patch
thanks for updating the tests - they lgtm now. (and yes, not sure why I thought the 3rd param was true/false) (I'm realizing that what we really need is to support a whole list of "oldFormat" rather than just one - i.e. we should be supporting all the 0x01-prefixed values here. But you don't need to address that in this patch)
(In reply to comment #17) > thanks for updating the tests - they lgtm now. (and yes, not sure why I thought the 3rd param was true/false) : ) no worries. > (I'm realizing that what we really need is to support a whole list of "oldFormat" rather than just one - i.e. we should be supporting all the 0x01-prefixed values here. But you don't need to address that in this patch) Indeed.
(In reply to comment #3) > I filed https://bugs.webkit.org/show_bug.cgi?id=102243 to track the issue this will cause with IndexedDB. We could land this first, but ideally 102243 would land at the same time or within a few days. So is there anything more I need do here Josuha?
(In reply to comment #19) > So is there anything more I need do here Josuha? Not that I know of - I'd say go ahead and land it! I have 102243 ready to go... apart from tests and those will probably have to live in Chromium anyway. Users downgrading w/o deleting their profiles is not supported, just common enough that I want to be proactive so we don't have to land these together.
This looks reasonable, but I would love to have Dan Carney comment, given that he's been working in the v8-specific 8-bit string code so much lately.
Actually, this has little to do with v8-8bit strings, so Dan's comments although welcome, are not necessary.
Comment on attachment 176875 [details] Patch If Alec says LGTM, then LGTM too.
Comment on attachment 176875 [details] Patch Clearing flags on attachment: 176875 Committed r136624: <http://trac.webkit.org/changeset/136624>
All reviewed patches have been landed. Closing bug.