Bug 90381 – [JSBinding] Merging jsUnsignedLongArrayToVector() to toNativeArray() using traits.

• Component: New Bugs
• Status: RESOLVED FIXED

[Vineet Chaudhary (vineetc)]

We can remove jsUnsignedLongArrayToVector() implementaion
merging it to current toNativeArray() traits.

jsUnsignedLongArrayToVector() was introduced in http://trac.webkit.org/changeset/107926

[Vineet Chaudhary (vineetc)]

Created attachment 150411 [details]
wipPatch

Initial patch to get review comments.

[Kentaro Hara]

Comment on attachment 150411 [details]
wipPatch

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

Overall the change looks good, but I am afraid that your patch might be changing the behavior of corner cases.

> Source/WebCore/bindings/js/JSDOMBinding.h:-410
> -        JSC::JSObject* object = toJSSequence(exec, value, length);
> -        if (exec->hadException())

This check and ...

> Source/WebCore/bindings/js/JSDOMBinding.h:-417
> -            indexedValue = object->get(exec, i);
> -            if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())

this check are missing from your patch.

- Would you check the Web IDL spec?

- Would you confirm that your patch won't change the current behavior? (In the first place, I am not sure if the current behavior is 100% conformed to the spec.)

[Vineet Chaudhary (vineetc)]

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

Thanks haraken for review comments.
Actually I had couple doubts as below.

> Source/WebCore/bindings/js/JSDOMBinding.h:-409
> -        JSC::JSObject* object = toJSSequence(exec, value, length);

In this implementation it uses toJSSequence(). As per comment this "Validates that the passed object is a sequence type per section 4.1.13 of the WebIDL spec." 
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/JSDOMBinding.h#L347
But I couldn't understand how it validates. If it does nothing can we eliminate these steps?

> Source/WebCore/bindings/scripts/test/TestObj.idl:203
> +        void methodWithSequenceUnsignedLong(in sequence<unsigned long> unsignedLongSequence);

As we support sequence<T> as method argument should we change
http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/vibration/NavigatorVibration.idl#L26

void webkitVibrate(in unsigned long[] pattern) raises(DOMException);  //TO

void webkitVibrate(in sequence<unsigned long> pattern) raises(DOMException);

[Vineet Chaudhary (vineetc)]

I will confirm WebIDL spec, corner cases and will updated bug.

[Vineet Chaudhary (vineetc)]

Created attachment 151421 [details]
patch_for_review

Updated Patch.

[Vineet Chaudhary (vineetc)]

I tried to cover case of check 
if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())

But I am not sure about 

>JSC::JSObject* object = toJSSequence(exec, value, length);
>        if (exec->hadException())

I think we don't need toJSSequence() specifically here and 

if (!isJSArray(value))
   return Vector<T>();

this check should sufficient.

[Kentaro Hara]

Comment on attachment 151421 [details]
patch_for_review

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

Thanks for the update. The overall approach looks good.

> Source/WebCore/bindings/js/JSDOMBinding.h:335
> +        static inline bool arrayNativeValue(JSC::ExecState* exec, JSC::JSValue jsValue, String& indexValue)

Nit: 'indexedValue' might be a better name.

> Source/WebCore/bindings/js/JSDOMBinding.h:337
> +            indexValue = ustringToString(jsValue.toString(exec)->value(exec));

'if (exec->hadException())' check is needed here too, right?

I know the current code does not check it. You can add the check in a follow-up patch, if needed.

> Source/WebCore/bindings/js/JSDOMBinding.h:344
> +        static inline bool arrayNativeValue(JSC::ExecState* exec, JSC::JSValue jsValue, unsigned long& indexValue)

Nit: indexValue -> indexedValue

> Source/WebCore/bindings/js/JSDOMBinding.h:346
> +            if (exec->hadException() || jsValue.isUndefinedOrNull() || !jsValue.isNumber())

The 'exec->hadException()' check should be after jsValue.toUint32(exec), because jsValue.toUint32(exec) is the guy who might cause the exception.

BTW, is there any possibility that jsValue.toUint32(exec) causes the exception under the condition '!jsValue.isUndefinedOrNull() && jsValue.isNumber()'?

BTW, is the 'jsValue.isUndefinedOrNull()' check needed? I mean, the 'jsValue.isNumber()' check might be enough (I am not sure though).

In summary, I guess the whole code could be:

if (!jsValue.isNumber())
    return true;
indexedValue = jsValue.toUInt32(exec);
return false;

> Source/WebCore/bindings/js/JSDOMBinding.h:365
> +            T indexValue;

Nit: indexValue -> indexedValue

> Source/WebCore/bindings/js/JSDOMBinding.h:366
> +            if (TraitsType::arrayNativeValue(exec, array->getIndex(i), indexValue))

Shall we flip true and false here? I mean, let's flip the return value of arrayNativeValue() so that this code becomes like this:

if (!TraitsType::arrayNativeValue(exec, array->getIndex(i), indexValue))
    return Vector<T>();

It sounds a bit strange to return an empty Vector if the return value is true.

> Source/WebCore/bindings/js/JSDOMBinding.h:-411
> -        JSC::JSObject* object = toJSSequence(exec, value, length);
> -        if (exec->hadException())
> -            return Vector<unsigned long>();

Removing toJSSequence() sounds reasonable to me. Shall we do it in a separate patch first? (Please cc folks who touched this code recently.)

[Kentaro Hara]

cc-ing JSC folks.

[Vineet Chaudhary (vineetc)]

Hi haraken,

Sorry, I was wrong we need to have check of toJSSequence()

The confusion was because of the spec difference older(http://www.w3.org/TR/2008/WD-WebIDL-20081219/#es-sequence) and new(http://www.w3.org/TR/2012/WD-WebIDL-20120207/#es-sequence).

Spec says while converting to Native array object:
1) value should have JSObject.
2) Let n be the result of calling ToUint32(length).

This type checking is currently used in NavigatorVibration.idl and MessagePort.idl::postMessage() which already have test cases for that.

[Kentaro Hara]

(In reply to comment #9)
> Spec says while converting to Native array object:
> 1) value should have JSObject.
> 2) Let n be the result of calling ToUint32(length).
> 
> This type checking is currently used in NavigatorVibration.idl and MessagePort.idl::postMessage() which already have test cases for that.

Now I agree that toJSSequence() is needed. But then...

(1) The current V8 implementation is wrong. The V8 implementation is not checking something corresponding to toJSSequence().

(2) The current JSC implementation is also wrong. toNativeArray() returns an empty vector if it finds jsValue that meets "jsValue.isUndefinedOrNull() || !jsValue.isNumber()". This behavior is not aligned to the spec.

Right?

[Vineet Chaudhary (vineetc)]

(In reply to comment #10)
> Now I agree that toJSSequence() is needed. But then...
> 
> (1) The current V8 implementation is wrong. The V8 implementation is not checking something corresponding to toJSSequence().

LayoutTests/fast/dom/Window/window-postmessage-args.html this is the layout test which actually tests parameters types which passes both for JS and V8.

The reason is JS checks type in toJSSequence() while V8 does these type checking in V8Utilities.cpp::extractTransferables() for MessagePorts.

Actually V8 implementation is wrong exactly but yes it does this for MessagePort only which should be generalised for other types too. Like toJSSequence() we need toV8Sequence().


> (2) The current JSC implementation is also wrong. toNativeArray() returns an empty vector if it finds jsValue that meets "jsValue.isUndefinedOrNull() || !jsValue.isNumber()". This behavior is not aligned to the spec.
> 

I am not very sure here, but as per spec 
...
4. Initialize S0..n−1 to be an IDL sequence with elements of type T, where each element is uninitialized.

5. While i < n:

    Let P be the result of calling ToString(i).
    Let E be the result of calling [[Get]] on A with property name P.
    Set Si to the result of converting E to an IDL value of type T.
    Set i to i + 1.

6. Return S.
...
Which means it can returns empty vector but I would request JS/WebIDL experts for opinion.

[Vineet Chaudhary (vineetc)]

> Actually V8 implementation is wrong exactly but yes it does this for MessagePort only which should be generalised for other types too. Like toJSSequence() we need toV8Sequence().

Actually V8 implementation is not wrong exactly but yes it does this for MessagePort only which should be generalised for other types too. Like toJSSequence() we need toV8Sequence().

[Kentaro Hara]

(In reply to comment #11)
> (In reply to comment #10)
> 4. Initialize S0..n−1 to be an IDL sequence with elements of type T, where each element is uninitialized.
> 
> 5. While i < n:
> 
>     Let P be the result of calling ToString(i).
>     Let E be the result of calling [[Get]] on A with property name P.
>     Set Si to the result of converting E to an IDL value of type T.
>     Set i to i + 1.
> 
> 6. Return S.
> ...
> Which means it can returns empty vector but I would request JS/WebIDL experts for opinion.

From what I understand, the spec implies that "if the length of the original array is n, then the length of the returned array is n". If that's true, it sounds strange to return an empty array when any unexpected jsValue is found.

Anyway, the current JSC/V8 implementation has a couple of issues. I am happy if you could fix them one by one, starting from non-controversial issues.

[Vineet Chaudhary (vineetc)]

Created attachment 152518 [details]
Patch

[Vineet Chaudhary (vineetc)]

Created attachment 152519 [details]
Updated Patch

Please Ignore previous patch //forgot to flip true and false.

[Vineet Chaudhary (vineetc)]

Comment on attachment 152519 [details]
Updated Patch

> Source/WebCore/bindings/js/JSDOMBinding.h:375
> +            if (!jsValue.isNumber())

Form the comments of bug https://bugs.webkit.org/show_bug.cgi?id=77317
What I understood is "UInt32 has the same range as Int32 (~2147483647) in JSValue" while Number in [0, 4294967295]
So even though "isNumber()" passed "toUInt32()" could raise exception

> Source/WebCore/bindings/js/JSDOMBinding.h:-334
> -        if (!isJSArray(value))

This check is redundant  now as toJSSequence() can handle this case.

[Kentaro Hara]

Comment on attachment 152519 [details]
Updated Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.cpp:-275
> -JSC::JSObject* toJSSequence(ExecState* exec, JSValue value, unsigned& length)

Why did you inline this? For performance, or for avoiding some compile error? If it is for performance, we might want to confirm that it really improves performance. (We are likely to inline methods "just in case" and wastes the size of binary.)

> Source/WebCore/bindings/js/JSDOMBinding.h:365
> +            if (exec->hadException())
> +                return false;

This will change the current behavior. Let's make this change in a separate patch with a test case.

> Source/WebCore/bindings/js/JSDOMBinding.h:390
> +        JSC::JSObject* object = toJSSequence(exec, value, length);

Shall we add the fast path for if(isJSArray(value)), as we've done in V8 bindings?

> Source/WebCore/bindings/js/JSDOMBinding.h:391
> +

Nit: This empty line is not needed.

> Source/WebCore/bindings/js/JSDOMBinding.h:-417
> -            if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())

Let me confirm one thing: "If jsValue.isUndefinedOrNull() is false, then jsValue.isNumber() is false", is this right? I want to confirm that you can remove the 'jsValue.isUndefinedOrNull()' check from your patch.

[Early Warning System Bot]

Comment on attachment 152519 [details]
Updated Patch

Attachment 152519 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13253267

[Early Warning System Bot]

Comment on attachment 152519 [details]
Updated Patch

Attachment 152519 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13235987

[Vineet Chaudhary (vineetc)]

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

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:-275
>> -JSC::JSObject* toJSSequence(ExecState* exec, JSValue value, unsigned& length)
> 
> Why did you inline this? For performance, or for avoiding some compile error? If it is for performance, we might want to confirm that it really improves performance. (We are likely to inline methods "just in case" and wastes the size of binary.)

No its not because of performance but for linking error. As its try to have multiple definition of toJSSequence().
Also the reason it is moved from .cpp to .h is the object of JSDOMBinding.cpp is linked in libWebCore.la but it required in libWebCoreInternals.la too.

>> Source/WebCore/bindings/js/JSDOMBinding.h:365
>> +                return false;
> 
> This will change the current behavior. Let's make this change in a separate patch with a test case.

Oke for now I will remove this check keeping only 

indexedValue = ustringToString(jsValue.toString(exec)->value(exec));
return true;

>> Source/WebCore/bindings/js/JSDOMBinding.h:390
>> +        JSC::JSObject* object = toJSSequence(exec, value, length);
> 
> Shall we add the fast path for if(isJSArray(value)), as we've done in V8 bindings?

Right we should have that check here.

>> Source/WebCore/bindings/js/JSDOMBinding.h:-417
>> -            if (exec->hadException() || indexedValue.isUndefinedOrNull() || !indexedValue.isNumber())
> 
> Let me confirm one thing: "If jsValue.isUndefinedOrNull() is false, then jsValue.isNumber() is false", is this right? I want to confirm that you can remove the 'jsValue.isUndefinedOrNull()' check from your patch.

Oke I will try out some test case to verify this.

[Gyuyoung Kim]

Comment on attachment 152519 [details]
Updated Patch

Attachment 152519 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13243960

[Vineet Chaudhary (vineetc)]

Created attachment 154069 [details]
Updated Patch

@haraken 
I confirmed as jsValue.isNumber() and jsValue.isUndefinedOrNull() are different checks so if 
irrespective  isUndefinedOrNull() true or false jsValue should be JSNumber so we can remove the isUndefinedOrNull() check here.

[Kentaro Hara]

Comment on attachment 154069 [details]
Updated Patch

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

OK

> Source/WebCore/bindings/js/JSDOMBinding.h:394
> +        if (exec->hadException())
>              return Vector<T>();

This check is not needed.

[Vineet Chaudhary (vineetc)]

Created attachment 154106 [details]
patch_for_landing

Thanks haraken for review.

[WebKit Review Bot]

Comment on attachment 154106 [details]
patch_for_landing

Clearing flags on attachment: 154106

Committed r123546: <http://trac.webkit.org/changeset/123546>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.