Bug 124175 – Blob constructor should accept sequences

• Component: New Bugs
• Status: RESOLVED FIXED

[Victor Costan]

According to the WebIDL in the File API, the Blob constructor should accept a sequence for the parts argument.
http://www.w3.org/TR/FileAPI/#dfn-Blob

In WebKit, the constructor only accepts arrays, as can be shown by the test case in the bug's URL.

Chromium bug (fixed, contains LayoutTests that might be useful):
https://code.google.com/p/chromium/issues/detail?id=314755

Firefox bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=937348

[Victor Costan]

Created attachment 216654 [details]
Patch

[Chris Dumez]

Comment on attachment 216654 [details]
Patch

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

> Source/WebCore/bindings/js/JSBlobCustom.cpp:81
> +    } else if (firstArg.isObject()) {

The Web IDL specification says "Any kind of object except for a native Date object or a native RegExp object"
http://heycam.github.io/webidl/#es-sequence

You currently accept Date / RegExp objects which is against the spec and also does not match Blink's behavior.

Also, It might be a good idea to move this to a separate utility function since there are other places that need to handle sequences.

> LayoutTests/fast/files/blob-constructor-expected.txt:13
> +FAIL new Blob('hello') should throw TypeError: First argument of the constructor is not of type Array. Threw exception TypeError: First argument is not a sequence.

The test needs updating. Leaving them as FAIL is not a good idea.

[Chris Dumez]

Comment on attachment 216654 [details]
Patch

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

>> LayoutTests/fast/files/blob-constructor-expected.txt:13
>> +FAIL new Blob('hello') should throw TypeError: First argument of the constructor is not of type Array. Threw exception TypeError: First argument is not a sequence.
> 
> The test needs updating. Leaving them as FAIL is not a good idea.

Also, I am not sure we want to expose the term "sequence" in the exception message since this is a Web IDL term and JavaScript developers won't necessarily know what it is.

[Alexey Proskuryakov]

JSCryptoOperationData.cpp also handles sequences, would be nice to see if some code can be shared.

[Alexey Proskuryakov]

Actually, that uses code that's already in a shared location, toJSSequence().

[Victor Costan]

Thank you for the fast and thorough feedback!


toJSSequence() throws a TypeError with no description. I copied its code and tweaked it to do what I wanted.

The two solutions I thought about are:
1) Adding a toJSSequence overload that takes an extra String containing the error description, and changing the current version to call it with the string TypeError.

2) Creating a separate toJSSequenceWithCustomErrorMessage (I'm terrible at names, sorry) with the tweaks that I added there.

Which way would you like me to go?

I inherited the Date / RegExp issue from the original implementation. I know the V8 bindings in Blink explicitly test, and I thought there's some reason why this isn't happening in JavaScriptCore / WebKit. Should I add the checks to toJSSequence?

[Chris Dumez]

(In reply to comment #6)
> Thank you for the fast and thorough feedback!
> 
> 
> toJSSequence() throws a TypeError with no description. I copied its code and tweaked it to do what I wanted.
> 
> The two solutions I thought about are:
> 1) Adding a toJSSequence overload that takes an extra String containing the error description, and changing the current version to call it with the string TypeError.
> 
> 2) Creating a separate toJSSequenceWithCustomErrorMessage (I'm terrible at names, sorry) with the tweaks that I added there.

Technically, we don't strictly need this custom error message. Throwing a TypeError is enough, but I'll let Alexey decide.

> Which way would you like me to go?
> 
> I inherited the Date / RegExp issue from the original implementation. I know the V8 bindings in Blink explicitly test, and I thought there's some reason why this isn't happening in JavaScriptCore / WebKit. Should I add the checks to toJSSequence?

This is not really related to this patch then and can / should be addressed separately.

[Alexey Proskuryakov]

Providing extra debugging information to web developers is certainly useful. But "First argument is not a sequence" is IMO not materially better than "Value is not a sequence" that could be added to toJSSequence() without any extra arguments.

[Victor Costan]

Created attachment 216729 [details]
Patch

[Victor Costan]

Created attachment 216730 [details]
Patch

[Victor Costan]

I removed the custom code in favor of calling toJSSequence and implemented Alexey's suggested error message. I also updated test expectations to match the new error message.

Please take another look?

[Chris Dumez]

Comment on attachment 216730 [details]
Patch

LGTM, r=me, assuming Alexey is also fine with this.

[Victor Costan]

Thank you, Chris!

Alexey, I'll wait for you to flip the commit-queue flag, or to give me more feedback.

[Alexey Proskuryakov]

Comment on attachment 216730 [details]
Patch

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

> Source/WebCore/bindings/js/JSBlobCustom.cpp:116
> +        JSValue item = blobParts->get(exec, i);

Can't get() throw an exception?

> Source/WebCore/bindings/js/JSBlobCustom.cpp:119
>          if (item.inherits(JSArrayBuffer::info()))
>              blobBuilder.append(toArrayBuffer(item));

This is not the best way to write this - toArrayBuffer will check the type again. I'd write

if (ArrayBuffer* arrayBuffer = toArrayBuffer(item))
    blobBuilder.append(arrayBuffer);

Same inefficiency is repeated below again.

[WebKit Commit Bot]

Comment on attachment 216730 [details]
Patch

Clearing flags on attachment: 216730

Committed r159275: <http://trac.webkit.org/changeset/159275>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Victor Costan]

Alexey, I'm preparing a new patch with the feedback you gave me. While doing that, I ran into something odd.

toArrayBuffer is defined in Source/JavaScriptCore/JSArrayBuffer.h as 
inline ArrayBuffer* toArrayBuffer(JSValue value)

toArrayBufferView is defined in Source/WebCore/bindings/js/JSDomBinding.h as
inline PassRefPtr<JSC::ArrayBufferView> toArrayBufferView(JSC::JSValue value)

Otherwise, the method bodies are really similar.

I think this is weird because, in the Blob constructor, I'd have to use RefPtr<ArrayBufferView> on one branch, and a bare ArrayBuffer* on the other branch, to follow convention.


What do you advise? Should I do this, or change one of the defintions? If I go down that route, I'd also move  toArrayBufferView in JavaScriptCore/JSArrayBufferView.h.

[Alexey Proskuryakov]

It seems worthwhile to look into whether toArrayBufferView can return a raw pointer. But my guess is that it's not possible - an ArrayBufferView only exists when referenced. JSArrayBufferView is not a wrapper, it's a proxy that creates an ArrayBufferView on demand.

This is based on a very brief look at the code, so it may be incorrect.

[Victor Costan]

Thank you very much for your guidance, Alexey!

I implemented your suggestions and submitted a new bug+patch.
https://bugs.webkit.org/show_bug.cgi?id=124343

I'll look into the ArrayBufferView issue sometime soon. I'm completely new to the JavaScriptCore codebase, so it will take a while.