Bug 91056 – [V8Bindings] Implement generalised method to validates that the passed object is a sequence type.

• Component: New Bugs
• Status: RESOLVED FIXED

[Vineet Chaudhary (vineetc)]

This has come up with Comment #10 from Bug90381.
Currently the V8 implementation validates that the passed object is a sequence type only
for MessagePort in V8Utilities::extractTransferables().
There should be generalised method for other types too.
JSDOMBindings already have such generic function toJSSequence().

[Vineet Chaudhary (vineetc)]

Created attachment 151882 [details]
Patch

[Vineet Chaudhary (vineetc)]

I thought of removing custom code from V8Utilities.cpp in another patch.
Please let me know if we want that in one patch.

[Kentaro Hara]

Comment on attachment 151882 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:444
> +    inline v8::Handle<v8::Object> toV8Sequence(v8::Handle<v8::Value> value, uint32_t& length)

I think the current toJSSequence() is conformed to the spec. Would you implement toV8Sequence() so that it exactly corresponds to toJSSequence()? (toV8Sequence() in this patch is not conformed to the spec and has a couple of bugs.)

> Source/WebCore/bindings/v8/V8Binding.h:447
> +            V8Proxy::throwTypeError("TransferArray argument must be an object");

We want to make this message the same as the toJSSequence()'s one.

> Source/WebCore/bindings/v8/V8Binding.h:455
> +            v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value);
> +            length = array->Length();

You are doing something like:

  array = ...;
  length = ...;
  return object;

which would be wrong.

[Kentaro Hara]

(In reply to comment #2)
> I thought of removing custom code from V8Utilities.cpp in another patch.
> Please let me know if we want that in one patch.

Another patch is fine.

[Vineet Chaudhary (vineetc)]

Created attachment 151891 [details]
Updated Patch

Updated patch with incorporating review comments.

[Kentaro Hara]

Comment on attachment 151891 [details]
Updated Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:447
> +            V8Proxy::throwTypeError("Type error");

To align with toJSSequence(), this could be V8Proxy::throwTypeError(). (though it would just print "Type error".)

> Source/WebCore/bindings/v8/V8Binding.h:448
> +            return v8::Local<v8::Object>();

You can simply write 'return V8Proxy::throwTypeError()'.

> Source/WebCore/bindings/v8/V8Binding.h:453
> +        v8::Local<v8::Value> lengthValue = object->Get(v8::String::New("length"));

Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h)

> Source/WebCore/bindings/v8/V8Binding.h:457
> +            V8Proxy::throwTypeError("Type error");
> +            return v8::Local<v8::Object>();

return V8Proxy::throwTypeError();

> Source/WebCore/bindings/v8/V8Binding.h:463
> +        if (!lengthValue->IsNumber()) {
> +            V8Proxy::throwTypeError("Type error");
> +            return v8::Local<v8::Object>();
> +        }

Is this check needed? toJSSequence() does not have the check.

> Source/WebCore/bindings/v8/V8Binding.h:465
> +        length = lengthValue->Uint32Value();

Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h)

[Vineet Chaudhary (vineetc)]

Comment on attachment 151891 [details]
Updated Patch

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

>> Source/WebCore/bindings/v8/V8Binding.h:448
>> +            return v8::Local<v8::Object>();
> 
> You can simply write 'return V8Proxy::throwTypeError()'.

actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object

>> Source/WebCore/bindings/v8/V8Binding.h:453
>> +        v8::Local<v8::Value> lengthValue = object->Get(v8::String::New("length"));
> 
> Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h)

Oke I will do.

[Kentaro Hara]

Comment on attachment 151891 [details]
Updated Patch

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

>>> Source/WebCore/bindings/v8/V8Binding.h:448
>>> +            return v8::Local<v8::Object>();
>> 
>> You can simply write 'return V8Proxy::throwTypeError()'.
> 
> actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object

What if we change the return type from v8::Handle<v8::Object> to v8::Handle<v8::Value>? Would it cause a problem in the caller side?

[Vineet Chaudhary (vineetc)]

(In reply to comment #8)
> (From update of attachment 151891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151891&action=review
> 
> >>> Source/WebCore/bindings/v8/V8Binding.h:448
> >>> +            return v8::Local<v8::Object>();
> >> 
> >> You can simply write 'return V8Proxy::throwTypeError()'.
> > 
> > actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object
> 
> What if we change the return type from v8::Handle<v8::Object> to v8::Handle<v8::Value>? Would it cause a problem in the caller side?

The original code is 
    v8::Local<v8::Object> transferrables = v8::Local<v8::Object>::Cast(value);

The caller code with Object return type would look like 

    uint32_t length = 0;
    v8::Hand<v8::Object> transferrables = toV8Sequence(value,length);
    if (transferrables.IsEmpty())
        return false;
And this "transferrables" further used.

With return type v8::Value along with the original code we need to add check what throwTypeError() returns.
BTW toJSSequence() returns JSObject.

[Vineet Chaudhary (vineetc)]

I was trying to use 
EXCEPTION_BLOCK(v8::Local<v8::Value>, lengthValue, object->Get(v8::String::New("length"))); and 
EXCEPTION_BLOCK(uint32_t, sequenceLength, lengthValue->Uint32Value());

But getting some compile errors like 

Source/WebKit/chromium/v8/include/v8.h: In constructor ‘v8::Handle<T>::Handle(v8::Handle<S>) [with S = v8::Value, T = v8::Object]’:
Source/WebCore/bindings/v8/V8Binding.h:462:9:   instantiated from here
Source/WebKit/chromium/v8/include/v8.h:202:5: error: invalid conversion from ‘v8::Value*’ to ‘v8::Object*’ [-fpermissive]

[Vineet Chaudhary (vineetc)]

Created attachment 151949 [details]
test_patch

[Vineet Chaudhary (vineetc)]

Created attachment 151958 [details]
Updated Patch

Updated patch.

[Vineet Chaudhary (vineetc)]

Comment on attachment 151958 [details]
Updated Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:445
> +    inline v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value> value, uint32_t& length)

inline is to avoid multiple definition error in multiple header inclusion.

> Source/WebCore/bindings/v8/V8Binding.h:462
> +        EXCEPTION_BLOCK(uint32_t, sequenceLength, lengthValue->Int32Value());

using length gives error of multiple declaration error.

[Kentaro Hara]

Comment on attachment 151958 [details]
Updated Patch

OK, thank you very much for the detailed investigation!

[WebKit Review Bot]

Comment on attachment 151958 [details]
Updated Patch

Clearing flags on attachment: 151958

Committed r122555: <http://trac.webkit.org/changeset/122555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.