Bug 91056

Summary: [V8Bindings] Implement generalised method to validates that the passed object is a sequence type.
Product: WebKit Reporter: Vineet Chaudhary (vineetc) <code.vineet>
Component: New BugsAssignee: Vineet Chaudhary (vineetc) <code.vineet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, jochen, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
haraken: review-
Updated Patch
haraken: review-
test_patch
none
Updated Patch none

Description Vineet Chaudhary (vineetc) 2012-07-12 01:15:09 PDT
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().
Comment 1 Vineet Chaudhary (vineetc) 2012-07-12 01:16:44 PDT
Created attachment 151882 [details]
Patch
Comment 2 Vineet Chaudhary (vineetc) 2012-07-12 01:18:56 PDT
I thought of removing custom code from V8Utilities.cpp in another patch.
Please let me know if we want that in one patch.
Comment 3 Kentaro Hara 2012-07-12 01:38:36 PDT
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.
Comment 4 Kentaro Hara 2012-07-12 01:39:06 PDT
(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.
Comment 5 Vineet Chaudhary (vineetc) 2012-07-12 02:24:07 PDT
Created attachment 151891 [details]
Updated Patch

Updated patch with incorporating review comments.
Comment 6 Kentaro Hara 2012-07-12 02:34:40 PDT
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)
Comment 7 Vineet Chaudhary (vineetc) 2012-07-12 02:50:15 PDT
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.
Comment 8 Kentaro Hara 2012-07-12 03:00:21 PDT
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?
Comment 9 Vineet Chaudhary (vineetc) 2012-07-12 03:24:24 PDT
(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.
Comment 10 Vineet Chaudhary (vineetc) 2012-07-12 04:16:27 PDT
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]
Comment 11 Vineet Chaudhary (vineetc) 2012-07-12 07:42:37 PDT
Created attachment 151949 [details]
test_patch
Comment 12 Vineet Chaudhary (vineetc) 2012-07-12 08:16:42 PDT
Created attachment 151958 [details]
Updated Patch

Updated patch.
Comment 13 Vineet Chaudhary (vineetc) 2012-07-12 08:19:49 PDT
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.
Comment 14 Kentaro Hara 2012-07-12 08:37:07 PDT
Comment on attachment 151958 [details]
Updated Patch

OK, thank you very much for the detailed investigation!
Comment 15 WebKit Review Bot 2012-07-13 02:39:02 PDT
Comment on attachment 151958 [details]
Updated Patch

Clearing flags on attachment: 151958

Committed r122555: <http://trac.webkit.org/changeset/122555>
Comment 16 WebKit Review Bot 2012-07-13 02:39:07 PDT
All reviewed patches have been landed.  Closing bug.