[Webkit-unassigned] [Bug 104354] IndexedDB: Don't use strings to represent serialized values

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=104354





--- Comment #12 from Michael Pruett <michael at 68k.org>  2012-12-12 14:10:58 PST ---
(In reply to comment #11)
> (From update of attachment 178915 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178915&action=review
> 
> > Source/WebCore/bindings/js/SerializedScriptValue.h:110
> > +        return adopt(bytes);
> 
> I'm confused how/why this works - I'm not familiar with the JSC SSV implementation, but aren't you adopting a stack-allocated Vector? or maybe can you just adopt data instead? 
> 
> (ah.. it looks like the constructor is relying on swap(), but maybe it should just be copying it in SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>& buffer) instead of swapping?

I agree that adding a constructor which takes a const Vector<uint8_t>& would be cleaner.

> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2262
> > +        dst[i] = ntohs(src[i]);
> 
> I'm actually now a little confused as to why the ntohs is necessary... This may be the most prudent fix for right now, but could this also be fixed by the V8 SSV parser/serializer simply unpacking/packing bytes in the right order? (it seems unfortunate that the act of creating or extracting an SSV requires this conversion, and that that cost should be paid by the parser/serializer in case the SSV is never deserialized)
> 
> > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2324
> > +        dst[i] = htons(src[i]);
> 
> (ditto for htons here)

I have added byte swapping to the V8 implementation of SSV's toWireBytes() and createFromWire(const Vector<uint8_t>&) in order to maintain binary compatibility with existing IDB databases. In the previous call path, the string in toWireString() and createFromWire(const String&) was swapped in IDBLevelDBCoding.h's encodeString() and decodeString().

Certainly swapping for deserialization could be performed at a later stage such as in SerializedScriptValue::deserialize() if performance is a concern, but that approach would be significantly more complex. I believe the approach in the proposed patch has equivalent performance to the status quo as every value which was written to or read from a LevelDB database was swapped.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.