Bug 32920 – [V8] Support SerializedScriptValue

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Vitaly Repeshko]

[V8] Support SerializedScriptValue

[Vitaly Repeshko]

Created attachment 45468 [details]
patch

[WebKit Review Bot]

Attachment 45468 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
ll/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:90:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:92:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:102:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:142:  number_bytes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:155:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:166:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:331:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:339:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:353:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bindings/v8/SerializedScriptValue.cpp:455:  raw_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:508:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:516:  raw_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:527:  number_bytes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:530:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:600:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:606:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:649:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 22

[Eric Seidel (no email)]

Comment on attachment 45468 [details]
patch

Since this is all new code, seems we should match the WebKit style guidelines.  r-

[Vitaly Repeshko]

Created attachment 45473 [details]
patch: fixed most style violations

[WebKit Review Bot]

Attachment 45473 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/SerializedScriptValue.cpp:75:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2

[Adam Barth]

> patch: fixed most style violations

Why not fix all of them?

[Vitaly Repeshko]

Created attachment 45475 [details]
patch: fixed all style violations

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45475 [details] without any errors.

[Adam Barth]

I'm a bit unclear what SerializedScriptValue is or why it's valuable.  I'm not trying to be hostile, I just don't know, which makes it hard to review the patch.

[Dimitri Glazkov (Google)]

I think David Levin should review this. He was interested in this neck of the woods. I agree with Adam that ChangeLog needs a bit more info mentioning structured cloning, purpose, etc.

[David Levin]

Looking at it tomorrow (Jan 5).

[David Levin]

Comment on attachment 45475 [details]
patch: fixed all style violations

Overall, nicely done. 

There a few details to address below and then this can get in.

General notes:

* Use FIXME: instead of TODO(name): in WebKit.
* Copyrights should get 2010 in them.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Vitaly Repeshko  <[e-mail]>

> +        * WebCore.gypi: Added SerializedScriptValue.cpp.
> +        * bindings/scripts/CodeGeneratorV8.pm: Removed conversion to string before using SerializedScriptValue.
> +        * bindings/v8/SerializedScriptValue.cpp: Added.
> +        (WebCore::):
> +        (WebCore::ZigZag::Writer::Writer):

The changelog is incorrect in listing ZigZag before all items.

As noted in other comments the changelog is too minimal.

> +        Updated clients:

What does "updated clients" mean?

> +        * bindings/v8/custom/V8DOMWindowCustom.cpp:
> +        (WebCore::V8DOMWindow::postMessageCallback):

> diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp b/WebCore/bindings/v8/SerializedScriptValue.cpp

> +namespace WebCore {
> +
> +#define DO(Action) if (!Action) return false

This define should be removed. It adds little value and it hides control flow changes.

> +// Helpers to do verbose handle casts.
> +
> +template <typename T, typename U>
> +static v8::Handle<T> handleCast(v8::Handle<U> handle) { return v8::Handle<T>::Cast(handle); }

> +
> +template <typename T, typename U>
> +static v8::Local<T> handleCast(v8::Local<U> handle) { return v8::Local<T>::Cast(handle); }

Your use of the verb handle along with the type name Handle is confusing.
I've seen WebKit use to* for casting types of functions. toHandle? and toLocal?

> +static bool isPowerOfTwo(int x)

The function name isn't really correct as it will return true for 0 and MININT (and it really isn't worth it to fix this). You could name it based on how you are using it: shouldCheckForCycles ?

> +// VarInt encoding constants.
> +static const int kVarIntShift = 7;
> +static const int kVarIntMask = (1 << kVarIntShift) - 1;

Constants should be named just like a variable and have no special prefix in WebKit code, so s/kVarIntShift/varIntShift/
Also var is an abbreviation which should be avoided in WebKit code.

> +
> +// ZigZag encoding to help VarInt encoding stay small for negative

/to help/helps/

> +// Writer is responsible for serializing primitive types and storing
> +// information used to reconstruct composite types.
> +class Writer : Noncopyable {
> +  public:
> +    Writer() : m_pos(0) { }
> +
> +    // Write functions for primitive types.
> +
> +    void writeUndefined() { append(UndefinedTag); }

It seems fragile that this relies on the fact that the oonly overload of append() that matches is append(char). It would be nice to add append(SerializationTag tag) which could simply do append(static_cast<char>(tag));

> +    void writeNumber(double number)
> +    {
> +        append(NumberTag);
> +        char* numberBytes = reinterpret_cast<char*>(&number);

There is no need for a local variable here and the name is confusing right now away (numberBytes sounds like it is the number of bytes for something as opposed to char pointer to the stat of a double.

> +        append(numberBytes, sizeof(number));
> +    }


> +    void ensureSpace(int extra)
> +    {
> +        m_buffer.grow((m_pos + extra + 1) / 2);

/ 2 due to sizeof(UChar) right?

Consider
        m_buffer.grow((m_pos + extra + 1) / sizeof(m_buffer::ValueType));
which removes the magic "2"

Why is the + 1 there?


> +    }
> +
> +    void fillHole()
> +    {
> +        // If the writer is at odd position in the buffer then one of

s/buffer then/buffer, then/

> +        // the bytes in the last UChar is not initialized.
> +        if (m_pos % 2)
> +            *charAt(m_pos) = PaddingTag;

Also this relies on the vector type being of size 2

Why not do this?

// Ensure that every byte in the vector is set to a known value.
while (m_pos % m_buffer::ValueType)
    append(PaddingTag);

> +    }
> +
> +    char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data()) + pos; }

Avoid abbreviations "pos" (same for m_pos).

> +class Serializer {
> +  public:
> +    explicit Serializer(Writer* writer)
> +            : m_writer(writer)

Only indent 4 spaces. (There are several other instances of this but I'll only leave this comment for this first case.)

> +            , m_state(0)
> +            , m_depth(0)
> +    {}

In this case just put the braces on separate lines. (There are several other instances of this but I'll only leave this comment for this first case.)

>
> +  private:
> +    class StateBase : public Noncopyable {
> +      public:
> +        virtual ~StateBase() {}

WebKit code typically puts a space between braces in this case. (There are several other instances of this but I'll only leave this comment for this first case.)


> +    class ArrayState : public State<v8::Array, ArrayTag> {
> +        virtual bool done(int* length)

isDone would be a better name here.

> +    bool doSerialize(v8::Handle<v8::Value> value)
> +    {
> +        if (value->IsUndefined()) {
> +            m_writer->writeUndefined();
> +        } else if (value->IsNull()) {
> +            m_writer->writeNull();
> +        } else if (value->IsTrue()) {
> +            m_writer->writeTrue();
> +        } else if (value->IsFalse()) {
> +            m_writer->writeFalse();
> +        } else if (value->IsString()) {
> +            v8::String::Utf8Value stringValue(value);
> +            m_writer->writeString(*stringValue, stringValue.length());
> +        } else if (value->IsInt32()) {
> +            m_writer->writeInt32(value->Int32Value());
> +        } else if (value->IsNumber()) {
> +            m_writer->writeNumber(handleCast<v8::Number>(value)->Value());
> +        } else if (value->IsArray()) {
> +            DO(checkComposite(value));
> +            push(new ArrayState(handleCast<v8::Array>(value)));
> +        } else if (value->IsObject()) {
> +            DO(checkComposite(value));
> +            push(new ObjectState(handleCast<v8::Object>(value)));
> +            // TODO(vitalyr):
> +            // - check not a wrapper
> +            // - support File, ImageData, etc.

What happens if this is checked in before doing these things?

> +        }

Sorry, single line clauses aren't suppose to have braces around them even in this case (so far).


> +    bool checkComposite(v8::Handle<v8::Value> composite)
> +    {
> +        if (m_depth > 20000)
> +            return false;
> +        // Since we are not required to spot the cycle as soon as it
> +        // happens we can check for cycles only when the current depth
> +        // is a power of two.

Nice.

> +        if (!isPowerOfTwo(m_depth))
> +            return true;
> +        for (StateBase* state = top(); state; state = state->nextState()) {
> +            if (state->composite() == composite)
> +                return false;
> +        }
> +        return true;
> +    }
> +
> +    Writer* m_writer;
> +    StateBase* m_state;
> +    int m_depth;
> +};


> +class Reader {

> +    bool readString(v8::Handle<v8::Value>* value)
> +    {
> +        uint32_t length;
> +        DO(doReadUint32(&length));
> +        if (m_pos + length > m_length)
> +            return false;

How is this possible?  Shouldn't this just be a CRASH() (from wtf/Assertions.h)?

> +        char* data = new char[length];

Use OwnArrayPtr (from wtf/OwnArrayPtr.h) and get rid of the delete below.

> +        for (int i = 0; i < length; ++i)
> +            data[i] = m_buffer[m_pos++];
> +        *value = v8::String::New(data, length);

Why can't this use m_buffer directly? 
    *value = v8::String::New(m_buffer, length);
    m_pos += length;

> +        delete[] data;
> +        return true;
> +    }
> +
> +    bool readInt32(v8::Handle<v8::Value>* value)
> +    {
> +        uint32_t rawValue;
> +        DO(doReadUint32(&rawValue));
> +        *value = v8::Integer::New(static_cast<int32_t>(ZigZag::decode(rawValue)));
> +        return true;
> +    }
> +
> +    bool readNumber(v8::Handle<v8::Value>* value)
> +    {
> +        if (m_pos + sizeof(double) > m_length)
> +            return false;

How is this possible?  Shouldn't this just be a CRASH() (from wtf/Assertions.h)?

> +        double number;
> +        char* numberBytes = reinterpret_cast<char*>(&number);

The variable name numberBytes is confusing. You could just get rid of the local variable.

> +        for (int i = 0; i < sizeof(double); ++i)
> +            numberBytes[i] = m_buffer[m_pos++];
> +        *value = v8::Number::New(number);
> +        return true;
> +    }
> +
> +    bool doReadUint32(uint32_t* value)
> +    {
> +        *value = 0;
> +        char b;

Avoid variables like "b". What about currentByte?

> +        int shift = 0;
> +        do {
> +            if (m_pos >= m_length)
> +                return false;

How is this possible?  Shouldn't this just be a CRASH() (from wtf/Assertions.h)?

> +            b = m_buffer[m_pos++];

> +            *value |= ((b & kVarIntMask) << shift);
> +            shift += 7;

Shouldn't 7 really be kVarIntShift?

> +        } while (b & (1 << kVarIntShift));
> +        return true;
> +    }
> +
> +    const char* m_buffer;
> +    const int m_length;
> +    int m_pos;
> +};
> +
> +class Deserializer {
> +  public:
> +    explicit Deserializer(Reader* reader) : m_reader(reader) {}

Since reader can never be 0, it would be better (in WebKit) to pass it in as Reader&.

> +
> +    v8::Local<v8::Value> deserialize()
> +    {
> +        v8::HandleScope scope;
> +        while (!m_reader->isEof()) {
> +            if (!doDeserialize())
> +                return v8::Local<v8::Value>();
> +        }
> +        if (stackDepth() != 1)
> +            return v8::Local<v8::Value>();

Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never call this function if m_reader->isEof()).

> +        return scope.Close(element(0));
> +    }
> +
> +  private:
> +    bool doDeserialize()

doDeserialization

> +    {
> +        SerializationTag tag;
> +        v8::Local<v8::Value> value;
> +        int length;
> +        DO(m_reader->read(&tag, &value, &length));

Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never call this function if m_reader->isEof()).

> +        if (!value.IsEmpty()) {
> +            push(value);
> +        } else if (tag == ObjectTag) {
> +            if (length > stackDepth())

Shouldn't this just be a CRASH() (from wtf/Assertions.h)?

> +                return false;
> +            v8::Local<v8::Object> object = v8::Object::New();
> +            for (int i = stackDepth() - length; i < stackDepth(); i += 2) {
> +                v8::Local<v8::Value> propertyName = element(i);
> +                v8::Local<v8::Value> propertyValue = element(i + 1);
> +                object->Set(propertyName, propertyValue);
> +            }
> +            pop(length);
> +            push(object);
> +        } else if (tag == ArrayTag) {
> +            if (length > stackDepth())

Shouldn't this just be a CRASH() (from wtf/Assertions.h)?

> +                return false;
> +            v8::Local<v8::Array> array = v8::Array::New(length);
> +            const int depth = stackDepth() - length;
> +            {
> +                v8::HandleScope scope;
> +                for (int i = 0; i < length; ++i)

Why not do the for loop like you did above?

                   for (int i = stackDepth() - length; i < stackDepth(); i++) {

> +                    array->Set(v8::Integer::New(i), element(depth + i));

> +            }
> +            pop(length);
> +            push(array);
> +        } else if (tag != PaddingTag)
> +            return false;
> +        return true;
> +    }


> +SerializedScriptValue::SerializedScriptValue(String data, bool wire)

Use an enum instead of a bool here so that it is clear in the calling sites what is being done (instead of just passing in true/false).


> diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp

> +    RefPtr<SerializedScriptValue> message = SerializedScriptValue::create(args[0]);
> +    fprintf(stderr, "SerializedScriptValue created in V8Worker::postMessageCallback\n");

It looks like you left some of your debugging code around "fprintf".

[Vitaly Repeshko]

Created attachment 46607 [details]
patch v2

[Vitaly Repeshko]

(In reply to comment #12)
> (From update of attachment 45475 [details])
> Overall, nicely done. 
> 
> There a few details to address below and then this can get in.

Thanks for thorough review. Please take another look.

> General notes:
> 
> * Use FIXME: instead of TODO(name): in WebKit.
> * Copyrights should get 2010 in them.

Done.

> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > [phone]  Vitaly Repeshko  <[e-mail]>
> 
> > +        * WebCore.gypi: Added SerializedScriptValue.cpp.
> > +        * bindings/scripts/CodeGeneratorV8.pm: Removed conversion to string before using SerializedScriptValue.
> > +        * bindings/v8/SerializedScriptValue.cpp: Added.
> > +        (WebCore::):
> > +        (WebCore::ZigZag::Writer::Writer):
> 
> The changelog is incorrect in listing ZigZag before all items.

Updated.

> As noted in other comments the changelog is too minimal.

Done. Since we're catching up with JSC bindings here I didn't expect extra explanation is needed.

> > +        Updated clients:
> 
> What does "updated clients" mean?

Now explained in the ChangeLog.

> > +        * bindings/v8/custom/V8DOMWindowCustom.cpp:
> > +        (WebCore::V8DOMWindow::postMessageCallback):
> 
> > diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp b/WebCore/bindings/v8/SerializedScriptValue.cpp
> 
> > +namespace WebCore {
> > +
> > +#define DO(Action) if (!Action) return false
> 
> This define should be removed. It adds little value and it hides control flow
> changes.

Removed. I expected this to be useful if we need to propagate more than just a boolean, but it should be easy to bring back.
 
> > +// Helpers to do verbose handle casts.
> > +
> > +template <typename T, typename U>
> > +static v8::Handle<T> handleCast(v8::Handle<U> handle) { return v8::Handle<T>::Cast(handle); }
> 
> > +
> > +template <typename T, typename U>
> > +static v8::Local<T> handleCast(v8::Local<U> handle) { return v8::Local<T>::Cast(handle); }
> 
> Your use of the verb handle along with the type name Handle is confusing.
> I've seen WebKit use to* for casting types of functions. toHandle? and toLocal?

"toHandle" and "toLocal" won't work because we already have Handle or Local types. I'd love to have this named handle_cast and be consistent with static_cast, etc., but WebKit style doesn't allow me to do this.

> > +static bool isPowerOfTwo(int x)
> 
> The function name isn't really correct as it will return true for 0 and MININT
> (and it really isn't worth it to fix this). You could name it based on how you
> are using it: shouldCheckForCycles ?

Done.
 
> > +// VarInt encoding constants.
> > +static const int kVarIntShift = 7;
> > +static const int kVarIntMask = (1 << kVarIntShift) - 1;
> 
> Constants should be named just like a variable and have no special prefix in
> WebKit code, so s/kVarIntShift/varIntShift/
> Also var is an abbreviation which should be avoided in WebKit code.

Renamed. "VarInt" is pretty common abbreviation so it should be allowed.

> > +
> > +// ZigZag encoding to help VarInt encoding stay small for negative
> 
> /to help/helps/

Done.
 
> > +// Writer is responsible for serializing primitive types and storing
> > +// information used to reconstruct composite types.
> > +class Writer : Noncopyable {
> > +  public:
> > +    Writer() : m_pos(0) { }
> > +
> > +    // Write functions for primitive types.
> > +
> > +    void writeUndefined() { append(UndefinedTag); }
> 
> It seems fragile that this relies on the fact that the oonly overload of
> append() that matches is append(char). It would be nice to add
> append(SerializationTag tag) which could simply do
> append(static_cast<char>(tag));

Done.
 
> > +    void writeNumber(double number)
> > +    {
> > +        append(NumberTag);
> > +        char* numberBytes = reinterpret_cast<char*>(&number);
> 
> There is no need for a local variable here and the name is confusing right now
> away (numberBytes sounds like it is the number of bytes for something as
> opposed to char pointer to the stat of a double.

Done.
 
> > +        append(numberBytes, sizeof(number));
> > +    }
> 
> 
> > +    void ensureSpace(int extra)
> > +    {
> > +        m_buffer.grow((m_pos + extra + 1) / 2);
> 
> / 2 due to sizeof(UChar) right?
> 
> Consider
>         m_buffer.grow((m_pos + extra + 1) / sizeof(m_buffer::ValueType));
> which removes the magic "2"
> 
> Why is the + 1 there?

Added an assert instead (see below). Also added a comment about "+ 1".

> > +    }
> > +
> > +    void fillHole()
> > +    {
> > +        // If the writer is at odd position in the buffer then one of
> 
> s/buffer then/buffer, then/
> 
> > +        // the bytes in the last UChar is not initialized.
> > +        if (m_pos % 2)
> > +            *charAt(m_pos) = PaddingTag;
> 
> Also this relies on the vector type being of size 2
> 
> Why not do this?
> 
> // Ensure that every byte in the vector is set to a known value.
> while (m_pos % m_buffer::ValueType)
>     append(PaddingTag);

Because the number of padding bytes also depends on the size.
That's why I added an assert.
 
> > +    }
> > +
> > +    char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data()) + pos; }
> 
> Avoid abbreviations "pos" (same for m_pos).

Some String (from PlatformString.h) methods use "pos". I don't think it's ambiguous in this context.

> > +class Serializer {
> > +  public:
> > +    explicit Serializer(Writer* writer)
> > +            : m_writer(writer)
> 
> Only indent 4 spaces. (There are several other instances of this but I'll only
> leave this comment for this first case.)

Done.
 
> > +            , m_state(0)
> > +            , m_depth(0)
> > +    {}
> 
> In this case just put the braces on separate lines. (There are several other
> instances of this but I'll only leave this comment for this first case.)

Done.

> >
> > +  private:
> > +    class StateBase : public Noncopyable {
> > +      public:
> > +        virtual ~StateBase() {}
> 
> WebKit code typically puts a space between braces in this case. (There are
> several other instances of this but I'll only leave this comment for this first
> case.)

Done. 
 
> > +    class ArrayState : public State<v8::Array, ArrayTag> {
> > +        virtual bool done(int* length)
> 
> isDone would be a better name here.

Done.

> > +    bool doSerialize(v8::Handle<v8::Value> value)
> > +    {
> > +        if (value->IsUndefined()) {
> > +            m_writer->writeUndefined();
> > +        } else if (value->IsNull()) {
> > +            m_writer->writeNull();
> > +        } else if (value->IsTrue()) {
> > +            m_writer->writeTrue();
> > +        } else if (value->IsFalse()) {
> > +            m_writer->writeFalse();
> > +        } else if (value->IsString()) {
> > +            v8::String::Utf8Value stringValue(value);
> > +            m_writer->writeString(*stringValue, stringValue.length());
> > +        } else if (value->IsInt32()) {
> > +            m_writer->writeInt32(value->Int32Value());
> > +        } else if (value->IsNumber()) {
> > +            m_writer->writeNumber(handleCast<v8::Number>(value)->Value());
> > +        } else if (value->IsArray()) {
> > +            DO(checkComposite(value));
> > +            push(new ArrayState(handleCast<v8::Array>(value)));
> > +        } else if (value->IsObject()) {
> > +            DO(checkComposite(value));
> > +            push(new ObjectState(handleCast<v8::Object>(value)));
> > +            // TODO(vitalyr):
> > +            // - check not a wrapper
> > +            // - support File, ImageData, etc.
> 
> What happens if this is checked in before doing these things?

Layout tests will fail (as they're doing now). This code doesn't support these inputs properly but it won't crash.
 
> > +        }
> 
> Sorry, single line clauses aren't suppose to have braces around them even in
> this case (so far).

Misaligned "else if"-s are totally unreadable. Let's leave it as it is given that check style doesn't complain.

> > +    bool checkComposite(v8::Handle<v8::Value> composite)
> > +    {
> > +        if (m_depth > 20000)
> > +            return false;
> > +        // Since we are not required to spot the cycle as soon as it
> > +        // happens we can check for cycles only when the current depth
> > +        // is a power of two.
> 
> Nice.
> 
> > +        if (!isPowerOfTwo(m_depth))
> > +            return true;
> > +        for (StateBase* state = top(); state; state = state->nextState()) {
> > +            if (state->composite() == composite)
> > +                return false;
> > +        }
> > +        return true;
> > +    }
> > +
> > +    Writer* m_writer;
> > +    StateBase* m_state;
> > +    int m_depth;
> > +};
> 
> 
> > +class Reader {
> 
> > +    bool readString(v8::Handle<v8::Value>* value)
> > +    {
> > +        uint32_t length;
> > +        DO(doReadUint32(&length));
> > +        if (m_pos + length > m_length)
> > +            return false;
> 
> How is this possible?  Shouldn't this just be a CRASH() (from
> wtf/Assertions.h)?

This is possible in case the input gets corrupted. I understand that this can be too forgiving (can hide a bug in serialization code) and will return to this when we add proper error reporting. Added a FIXME to the file's comment.
 
> > +        char* data = new char[length];
> 
> Use OwnArrayPtr (from wtf/OwnArrayPtr.h) and get rid of the delete below.
> 
> > +        for (int i = 0; i < length; ++i)
> > +            data[i] = m_buffer[m_pos++];
> > +        *value = v8::String::New(data, length);
> 
> Why can't this use m_buffer directly? 
>     *value = v8::String::New(m_buffer, length);
>     m_pos += length;

Good catch. Left over from when I used buffer with different element type.

> > +        delete[] data;
> > +        return true;
> > +    }
> > +
> > +    bool readInt32(v8::Handle<v8::Value>* value)
> > +    {
> > +        uint32_t rawValue;
> > +        DO(doReadUint32(&rawValue));
> > +        *value = v8::Integer::New(static_cast<int32_t>(ZigZag::decode(rawValue)));
> > +        return true;
> > +    }
> > +
> > +    bool readNumber(v8::Handle<v8::Value>* value)
> > +    {
> > +        if (m_pos + sizeof(double) > m_length)
> > +            return false;
> 
> How is this possible?  Shouldn't this just be a CRASH() (from
> wtf/Assertions.h)?
> 
> > +        double number;
> > +        char* numberBytes = reinterpret_cast<char*>(&number);
> 
> The variable name numberBytes is confusing. You could just get rid of the local
> variable.

"reinterpret_cast<char*>(&number)[i]" is ugly and hard to read. I renamed the variable.
 
> > +        for (int i = 0; i < sizeof(double); ++i)
> > +            numberBytes[i] = m_buffer[m_pos++];
> > +        *value = v8::Number::New(number);
> > +        return true;
> > +    }
> > +
> > +    bool doReadUint32(uint32_t* value)
> > +    {
> > +        *value = 0;
> > +        char b;
> 
> Avoid variables like "b". What about currentByte?

Okay.
 
> > +        int shift = 0;
> > +        do {
> > +            if (m_pos >= m_length)
> > +                return false;
> 
> How is this possible?  Shouldn't this just be a CRASH() (from
> wtf/Assertions.h)?
> 
> > +            b = m_buffer[m_pos++];
> 
> > +            *value |= ((b & kVarIntMask) << shift);
> > +            shift += 7;
> 
> Shouldn't 7 really be kVarIntShift?

Oops, done.
 
> > +        } while (b & (1 << kVarIntShift));
> > +        return true;
> > +    }
> > +
> > +    const char* m_buffer;
> > +    const int m_length;
> > +    int m_pos;
> > +};
> > +
> > +class Deserializer {
> > +  public:
> > +    explicit Deserializer(Reader* reader) : m_reader(reader) {}
> 
> Since reader can never be 0, it would be better (in WebKit) to pass it in as
> Reader&.

Done. (And for Writer too.)
 
> > +
> > +    v8::Local<v8::Value> deserialize()
> > +    {
> > +        v8::HandleScope scope;
> > +        while (!m_reader->isEof()) {
> > +            if (!doDeserialize())
> > +                return v8::Local<v8::Value>();
> > +        }
> > +        if (stackDepth() != 1)
> > +            return v8::Local<v8::Value>();
> 
> Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never
> call this function if m_reader->isEof()).
> 
> > +        return scope.Close(element(0));
> > +    }
> > +
> > +  private:
> > +    bool doDeserialize()
> 
> doDeserialization
> 
> > +    {
> > +        SerializationTag tag;
> > +        v8::Local<v8::Value> value;
> > +        int length;
> > +        DO(m_reader->read(&tag, &value, &length));
> 
> Shouldn't this just be a CRASH() (from wtf/Assertions.h)? (because you never
> call this function if m_reader->isEof()).
> 
> > +        if (!value.IsEmpty()) {
> > +            push(value);
> > +        } else if (tag == ObjectTag) {
> > +            if (length > stackDepth())
> 
> Shouldn't this just be a CRASH() (from wtf/Assertions.h)?
> 
> > +                return false;
> > +            v8::Local<v8::Object> object = v8::Object::New();
> > +            for (int i = stackDepth() - length; i < stackDepth(); i += 2) {
> > +                v8::Local<v8::Value> propertyName = element(i);
> > +                v8::Local<v8::Value> propertyValue = element(i + 1);
> > +                object->Set(propertyName, propertyValue);
> > +            }
> > +            pop(length);
> > +            push(object);
> > +        } else if (tag == ArrayTag) {
> > +            if (length > stackDepth())
> 
> Shouldn't this just be a CRASH() (from wtf/Assertions.h)?
> 
> > +                return false;
> > +            v8::Local<v8::Array> array = v8::Array::New(length);
> > +            const int depth = stackDepth() - length;
> > +            {
> > +                v8::HandleScope scope;
> > +                for (int i = 0; i < length; ++i)
> 
> Why not do the for loop like you did above?
> 
>                    for (int i = stackDepth() - length; i < stackDepth(); i++) {
> 
> > +                    array->Set(v8::Integer::New(i), element(depth + i));

Because I need to set array elements staring from 0 and not from stackDepth() - length.

> > +            }
> > +            pop(length);
> > +            push(array);
> > +        } else if (tag != PaddingTag)
> > +            return false;
> > +        return true;
> > +    }
> 
> 
> > +SerializedScriptValue::SerializedScriptValue(String data, bool wire)
> 
> Use an enum instead of a bool here so that it is clear in the calling sites
> what is being done (instead of just passing in true/false).

I agree that using enums is preferred in general but since it's not a part of the public interface here I don't think it matters.

> > diff --git a/WebCore/bindings/v8/custom/V8WorkerCustom.cpp b/WebCore/bindings/v8/custom/V8WorkerCustom.cpp
> 
> > +    RefPtr<SerializedScriptValue> message = SerializedScriptValue::create(args[0]);
> > +    fprintf(stderr, "SerializedScriptValue created in V8Worker::postMessageCallback\n");
> 
> It looks like you left some of your debugging code around "fprintf".

Oops, removed.

[David Levin]

Comment on attachment 46607 [details]
patch v2

Just a few last comments.

> diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp b/WebCore/bindings/v8/SerializedScriptValue.cpp


> +// Writer is responsible for serializing primitive types and storing
> +// information used to reconstruct composite types.
> +class Writer : Noncopyable {
> +  public:

public:, protected:, private: shouldn't be indented.

> +    void ensureSpace(int extra)
> +    {
> +        ASSERT(sizeof(UChar) == 2);

Use COMPILE_ASSERT from wtf/Assertions.h and ideally use m_buffer::ValueType instead of UChar (if possible, I didn't check to see if that compiles).


> +    void fillHole()
> +    {
> +        ASSERT(sizeof(UChar) == 2);
Use COMPILE_ASSERT


> +    char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data()) + pos; }

> > Avoid abbreviations "pos" (same for m_pos).

> Some String (from PlatformString.h) methods use "pos". I don't think it's
> ambiguous in this context.

StringImpl.h is not a good example of WebKit style in several areas. From the WebKit style guide "Use full words, except in the rare case where an abbreviation would be more canonical and easier to understand."



> +    bool doSerialize(v8::Handle<v8::Value> value)
> +    {
> +        if (value->IsUndefined()) {
> +            m_writer.writeUndefined();
> +        } else if (value->IsNull()) {
> +            m_writer.writeNull();
> +        } else if (value->IsTrue()) {
> +            m_writer.writeTrue();
> +        } else if (value->IsFalse()) {
> +            m_writer.writeFalse();
> +        } else if (value->IsString()) {
> +            v8::String::Utf8Value stringValue(value);
> +            m_writer.writeString(*stringValue, stringValue.length());
> +        } else if (value->IsInt32()) {
> +            m_writer.writeInt32(value->Int32Value());
> +        } else if (value->IsNumber()) {
> +            m_writer.writeNumber(handleCast<v8::Number>(value)->Value());
> +        } else if (value->IsArray()) {
> +            if (!checkComposite(value))
> +                return false;
> +            push(new ArrayState(handleCast<v8::Array>(value)));
> +        } else if (value->IsObject()) {
> +            if (!checkComposite(value))
> +                return false;
> +            push(new ObjectState(handleCast<v8::Object>(value)));
> +            // FIXME:
> +            // - check not a wrapper
> +            // - support File, ImageData, etc.
> +        }

> > Sorry, single line clauses aren't suppose to have braces around them even in
> > this case (so far).

> Misaligned "else if"-s are totally unreadable. Let's leave it as it is given
> that check style doesn't complain.

Check style isn't comprehensive for everything (including this). This is ugly, but it is the current style as stated. This exact issue came up before on webkit-dev and didn't get resolved completely. Perhaps you'd like to drive this style issue to completion?



> +    bool readString(v8::Handle<v8::Value>* value)
> +    {
> +        uint32_t length;
> +        if (!doReadUint32(&length))
> +            return false;

> > How is this possible?  Shouldn't this just be a CRASH() (from
> > wtf/Assertions.h)?

> This is possible in case the input gets corrupted. I understand that this can
> be too forgiving (can hide a bug in serialization code) and will return to this
> when we add proper error reporting. Added a FIXME to the file's comment.

It can hide bugs in serialization, hide bugs elsewhere, hide security issues.....
 
This seem to be the only cases for the corruption happening. In all of these cases, I'd rather the code crash rather than try to hide the issue and continue -- who knows what else has been corrupted.

> +
> +v8::Local<v8::Value> SerializedScriptValue::deserialize()
> +{
> +    if (!m_data.impl())
> +        return v8::Local<v8::Value>();
> +    Reader reader(reinterpret_cast<const char*>(m_data.impl()->characters()), 2 * m_data.length());

 It would be best to get rid of the magic number "2" and replace it with sizeof(m_data::ValueType).

> diff --git a/WebCore/bindings/v8/SerializedScriptValue.h b/WebCore/bindings/v8/SerializedScriptValue.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2009 Google Inc. All rights reserved.
> + * Copyright (C) 2010 Google Inc. All rights reserved.

The 2009 should remain since this code already existed (was checked in) with that copyright. (It should be "Copyright (C) 2009, 2010 Google Inc. All rights reserved."


> +    explicit SerializedScriptValue(v8::Handle<v8::Value> value);

The param name value shouldn't be here because it doesn't add any information.

> +
> +    SerializedScriptValue(String data, bool wire);

As mentioned before, this bool should be an enum. See http://marc.info/?l=webkit-dev&m=125199261526357&w=2

[Vitaly Repeshko]

Created attachment 46678 [details]
patch v3

[Vitaly Repeshko]

(In reply to comment #15)
> (From update of attachment 46607 [details])
> Just a few last comments.
> 
> > diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp b/WebCore/bindings/v8/SerializedScriptValue.cpp
> 
> 
> > +// Writer is responsible for serializing primitive types and storing
> > +// information used to reconstruct composite types.
> > +class Writer : Noncopyable {
> > +  public:
> 
> public:, protected:, private: shouldn't be indented.

Done (in other places too).
 
> > +    void ensureSpace(int extra)
> > +    {
> > +        ASSERT(sizeof(UChar) == 2);
> 
> Use COMPILE_ASSERT from wtf/Assertions.h and ideally use m_buffer::ValueType
> instead of UChar (if possible, I didn't check to see if that compiles).

Unfortunately it won't compile. I added a typedef for UChar and used it everywhere in this file. Also switched to COMPILE_ASSERT.

> > +    void fillHole()
> > +    {
> > +        ASSERT(sizeof(UChar) == 2);
> Use COMPILE_ASSERT

Done.

> > +    char* charAt(int pos) { return reinterpret_cast<char*>(m_buffer.data()) + pos; }
> 
> > > Avoid abbreviations "pos" (same for m_pos).
> 
> > Some String (from PlatformString.h) methods use "pos". I don't think it's
> > ambiguous in this context.
> 
> StringImpl.h is not a good example of WebKit style in several areas. From the
> WebKit style guide "Use full words, except in the rare case where an
> abbreviation would be more canonical and easier to understand."

There are 100+ source files in WebCore using "pos" (whole word), but "position" is more common, and "m_pos" is rare. Switched to "position".

> > +    bool doSerialize(v8::Handle<v8::Value> value)
> > +    {
> > +        if (value->IsUndefined()) {
> > +            m_writer.writeUndefined();
> > +        } else if (value->IsNull()) {
> > +            m_writer.writeNull();
> > +        } else if (value->IsTrue()) {
> > +            m_writer.writeTrue();
> > +        } else if (value->IsFalse()) {
> > +            m_writer.writeFalse();
> > +        } else if (value->IsString()) {
> > +            v8::String::Utf8Value stringValue(value);
> > +            m_writer.writeString(*stringValue, stringValue.length());
> > +        } else if (value->IsInt32()) {
> > +            m_writer.writeInt32(value->Int32Value());
> > +        } else if (value->IsNumber()) {
> > +            m_writer.writeNumber(handleCast<v8::Number>(value)->Value());
> > +        } else if (value->IsArray()) {
> > +            if (!checkComposite(value))
> > +                return false;
> > +            push(new ArrayState(handleCast<v8::Array>(value)));
> > +        } else if (value->IsObject()) {
> > +            if (!checkComposite(value))
> > +                return false;
> > +            push(new ObjectState(handleCast<v8::Object>(value)));
> > +            // FIXME:
> > +            // - check not a wrapper
> > +            // - support File, ImageData, etc.
> > +        }
> 
> > > Sorry, single line clauses aren't suppose to have braces around them even in
> > > this case (so far).
> 
> > Misaligned "else if"-s are totally unreadable. Let's leave it as it is given
> > that check style doesn't complain.
> 
> Check style isn't comprehensive for everything (including this). This is ugly,
> but it is the current style as stated. This exact issue came up before on
> webkit-dev and didn't get resolved completely. Perhaps you'd like to drive this
> style issue to completion?

Not now. I reordered these a bit to make braceless "else if"-s go first. This way I can almost read it.

> > +    bool readString(v8::Handle<v8::Value>* value)
> > +    {
> > +        uint32_t length;
> > +        if (!doReadUint32(&length))
> > +            return false;
> 
> > > How is this possible?  Shouldn't this just be a CRASH() (from
> > > wtf/Assertions.h)?
> 
> > This is possible in case the input gets corrupted. I understand that this can
> > be too forgiving (can hide a bug in serialization code) and will return to this
> > when we add proper error reporting. Added a FIXME to the file's comment.
> 
> It can hide bugs in serialization, hide bugs elsewhere, hide security
> issues.....
> 
> This seem to be the only cases for the corruption happening. In all of these
> cases, I'd rather the code crash rather than try to hide the issue and continue
> -- who knows what else has been corrupted.

As I said I'm not entirely happy with the way it's now (that's why there is a FIXME above). Yet I think it should only crash in debug mode. The plan is to return some status enum instead of simply true/false in all methods. Then we can crash when a stream corruption is found in debug mode and return deserialization failure in release mode.

> > +
> > +v8::Local<v8::Value> SerializedScriptValue::deserialize()
> > +{
> > +    if (!m_data.impl())
> > +        return v8::Local<v8::Value>();
> > +    Reader reader(reinterpret_cast<const char*>(m_data.impl()->characters()), 2 * m_data.length());
> 
>  It would be best to get rid of the magic number "2" and replace it with
> sizeof(m_data::ValueType).

Done.
 
> > diff --git a/WebCore/bindings/v8/SerializedScriptValue.h b/WebCore/bindings/v8/SerializedScriptValue.h
> > @@ -1,5 +1,5 @@
> >  /*
> > - * Copyright (C) 2009 Google Inc. All rights reserved.
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> The 2009 should remain since this code already existed (was checked in) with
> that copyright. (It should be "Copyright (C) 2009, 2010 Google Inc. All rights
> reserved."
> 
> 
> > +    explicit SerializedScriptValue(v8::Handle<v8::Value> value);
> 
> The param name value shouldn't be here because it doesn't add any information.

Removed.
 
> > +
> > +    SerializedScriptValue(String data, bool wire);
> 
> As mentioned before, this bool should be an enum. See
> http://marc.info/?l=webkit-dev&m=125199261526357&w=2

Done. Yet it's not really nice that there are some hard rules that are not part of the official style guide.


Thanks and please take yet another look.

[WebKit Review Bot]

Attachment 46678 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/SerializedScriptValue.cpp:204:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1

[Vitaly Repeshko]

Created attachment 46679 [details]
patch v3.1: fix style issue

[David Levin]

Thanks! It is awesome to have this.

(In reply to comment #17)
> > > +    bool readString(v8::Handle<v8::Value>* value)
> > > +    {
> > > +        uint32_t length;
> > > +        if (!doReadUint32(&length))
> > > +            return false;
> > 
> > > > How is this possible?  Shouldn't this just be a CRASH() (from
> > > > wtf/Assertions.h)?
> > 
> > > This is possible in case the input gets corrupted. I understand that this can
> > > be too forgiving (can hide a bug in serialization code) and will return to this
> > > when we add proper error reporting. Added a FIXME to the file's comment.
> > 
> > It can hide bugs in serialization, hide bugs elsewhere, hide security
> > issues.....
> > 
> > This seem to be the only cases for the corruption happening. In all of these
> > cases, I'd rather the code crash rather than try to hide the issue and continue
> > -- who knows what else has been corrupted.
> 
> As I said I'm not entirely happy with the way it's now (that's why there is a
> FIXME above). Yet I think it should only crash in debug mode. The plan is to
> return some status enum instead of simply true/false in all methods. Then we
> can crash when a stream corruption is found in debug mode and return
> deserialization failure in release mode.

Ok for now. But consider crashing in both cases. It sounds attractive to hide the issue in release mode but in actuality how often will it happen and for what reason. Is it better to hide it or know about it from crash reports.


> Done. Yet it's not really nice that there are some hard rules that are not part
> of the official style guide.

You're right. I'm working on it. I have a patch to put in some of them and will try to finish adding the other ones shortly. (It just has been low in my priority list for too long.)

[WebKit Commit Bot]

Comment on attachment 46679 [details]
patch v3.1: fix style issue

Clearing flags on attachment: 46679

Committed r53344: <http://trac.webkit.org/changeset/53344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This broke the Chromium Mac build:
Ld /Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/./xcodebuild/Debug/webkit_unit_tests normal i386
Undefined symbols:
  "WebCore::SerializedScriptValue::deserialize()", referenced from:
      WebCore::MessageEventInternal::dataAttrGetter(v8::Local<v8::String>, v8::AccessorInfo const&)      in libwebcore_bindings.a(DerivedSourcesAllInOne.o)
  "WebCore::SerializedScriptValue::SerializedScriptValue(WebCore::String, WebCore::SerializedScriptValue::StringDataMode)", referenced from:
      WebCore::SerializedScriptValue::createFromWire(WebCore::String) in libwebkit.a(PlatformMessagePortChannel.o)
      WebCore::SerializedScriptValue::createFromWire(WebCore::String) in libwebkit.a(WebWorkerClientImpl.o)
ld: symbol(s) not found

If I don't hear otherwise soon, I'll start a rollout.

[Eric Seidel (no email)]

Full log:
http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/1823/steps/compile-webkit/logs/stdio

[Eric Seidel (no email)]

Starting a rollout.

[Eric Seidel (no email)]

Reverted r53344 for reason:

Broke the Chromium Mac builder.

Committed r53347: <http://trac.webkit.org/changeset/53347>

[Vitaly Repeshko]

Created attachment 46954 [details]
patch v3.2: fix unsigned-signed comparison issues

[WebKit Commit Bot]

Comment on attachment 46954 [details]
patch v3.2: fix unsigned-signed comparison issues

Rejecting patch 46954 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[Vitaly Repeshko]

Created attachment 47001 [details]
patch v3.3

Apparently another patch is needed to make cq+ work.

[Eric Seidel (no email)]

I'm confused why we have a patch attached which has r+/cq+ and is obsolete.  Sadly obsolete patches still show up in http://webkit.org/pending-commit so we should either clear the flags, or un-obsolete the patch if it is to be landed.

[WebKit Commit Bot]

Comment on attachment 47001 [details]
patch v3.3

Clearing flags on attachment: 47001

Committed r53586: <http://trac.webkit.org/changeset/53586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.