Bug 46541 – Implement DataView interface from Typed Array specification

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Filing this under component WebGL since there is no better component at the moment for Typed Arrays.

The proposed DataView interface in the Typed Array specification ( https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/TypedArray-spec.html#6 ) needs to be implemented in WebKit. This will make it easier to read multi-byte values with specified endianness out of an ArrayBuffer.

[Jian Li]

Created attachment 74339 [details]
Proposed Patch

[Jian Li]

Created attachment 74349 [details]
Proposed Patch

[Kenneth Russell]

Comment on attachment 74349 [details]
Proposed Patch

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

This mostly looks good but there are a few issues. Sorry about the license header complaint, but it's been made clear that new files need to conform to the correct version of the BSD license.

> LayoutTests/fast/canvas/webgl/data-view-test.html:55
> +function runTests(isTestingGet, array, start, length) {

Please add test cases for more unaligned values, in particular int16/uint16/int32/uint32/float32 with odd offsets.

> LayoutTests/fast/canvas/webgl/data-view-test.html:126
> +    // Little endian.
> +    test(isTestingGet, "Float32", 4, 0, "3.820471434542632e-37", true);
> +    test(isTestingGet, "Float32", 4, 8, "-7.670445022226018e-37", true);
> +    test(isTestingGet, "Float32", 4, 10, "-4.1956035317439417e+37", true);
> +
> +    // Big endian.
> +    test(isTestingGet, "Float32", 4, 0, "9.25571648671185e-41");
> +    test(isTestingGet, "Float32", 4, 8, "-1.1893597787372423e-38");
> +    test(isTestingGet, "Float32", 4, 10, "-1.9393928146782153e-37");
> +
> +    // Little endian.
> +    test(isTestingGet, "Float64", 8, 0, "1.2473227726741413e+190", true);
> +    test(isTestingGet, "Float64", 8, 4, "-9.2722175465198070e-292", true);
> +    test(isTestingGet, "Float64", 8, 7, "-5.1409065709798940e+303", true);
> +
> +    // Big endian.
> +    test(isTestingGet, "Float64", 8, 0, "1.4016077617689270e-309");
> +    test(isTestingGet, "Float64", 8, 4, "4.2342998002728550e+175");
> +    test(isTestingGet, "Float64", 8, 7, "3.6771083188053240e+190");

You might want to consider using test values that are exactly representable with floating-point numbers (e.g., both small whole numbers and small fractional values).

> WebCore/bindings/js/JSDataViewCustom.cpp:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/bindings/js/JSDataViewCustom.cpp:53
> +        throwTypeError(exec);
> +        return JSValue::encode(JSValue());

return throwVMTypeError(exec);

> WebCore/bindings/js/JSDataViewCustom.cpp:59
> +        throwTypeError(exec);
> +        return JSValue::encode(JSValue());

Same.

> WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:75
> +    args.Holder()->SetIndexedPropertiesToExternalArrayData(array.get()->baseAddress(), arrayType, array.get()->length());

This call to SetIndexedPropertiesToExternalArrayData must only be done for the TypedArray subclasses, and not for DataView. You'll need to add a boolean argument to this helper indicating whether to make this call. Please add a negative test to your test case that indexing (i.e., "dataView[0]" and "dataView[0] = 0" don't work. (I think reading should return undefined, and setting should silently add a new property to the object -- not 100% sure.)

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:28
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:60
> +    if (!wrapper.IsEmpty())
> +        wrapper->SetIndexedPropertiesToExternalArrayData(impl->baseAddress(), v8::kExternalByteArray, impl->length());

This call must not be done for DataView instances. It would transform them into array-like objects.

> WebCore/html/canvas/DataView.cpp:28
> + * Copyright (C) 2010 Google Inc.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/html/canvas/DataView.cpp:63
> +static void flipBytesToMeetEndianRequirement(void* value, size_t typeSize, bool littleEndian)
> +{
> +#if CPU(BIG_ENDIAN)
> +    bool flipBytes = littleEndian;
> +#else
> +    bool flipBytes = !littleEndian;
> +#endif
> +
> +    if (!flipBytes) 
> +        return;
> +
> +    char* begin = static_cast<char*>(value);
> +    char* end = static_cast<char*>(value) + typeSize - 1;
> +    for (size_t i = 0; i < typeSize / 2; ++i, ++begin, --end) {
> +        char temp = *begin;
> +        *begin = *end;
> +        *end = temp;
> +    }
> +}

This could be specialized for the sizes we will need to swap (2, 4, and 8 byte values) to be much more efficient. You could use a switch statement in getData and setData to determine which specialized version to call. Even though all of these code paths are not optimal and will require intrinsics in the JavaScript engine to be truly efficient, it's worth it to not make these slower than needed.

> WebCore/html/canvas/DataView.cpp:72
> +    T value = *(static_cast<const T*>(static_cast<const void*>(static_cast<const char*>(m_baseAddress) + byteOffset)));

This will cause a bus error on architectures that don't support unaligned loads. I think the best solution is to use a template union. See http://www.devx.com/cplus/Article/35609/1954 . memcpy from the DataView into the "char bytes[]" arm of the union, byte swap there, and then return the typed arm of the union.

> WebCore/html/canvas/DataView.cpp:87
> +    *(static_cast<T*>(static_cast<void*>(static_cast<char*>(m_baseAddress) + byteOffset))) = value;

This will similarly cause a bus error on architectures that don't support unaligned stores. Use the above template union trick to convert the value to bytes and then memcpy it into place.

> WebCore/html/canvas/DataView.h:28
> + * Copyright (C) 2010 Google Inc.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/html/canvas/DataView.idl:28
> + * Copyright (C) 2010 Google Inc.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Wrong license header. See e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/html/canvas/DataView.idl:62
> +        [StrictTypeChecking, RequiresAllArguments=Raise] float getFloat32(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> +            raises (DOMException);
> +        [StrictTypeChecking, RequiresAllArguments=Raise] double getFloat64(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> +            raises (DOMException);

I'm afraid that getFloat32 and getFloat64 require custom JSC bindings that check to see whether the return value is NaN (using isnan(), #include <wtf/MathExtras.h>) and if so return jsValue(nonInlineNaN()) (JavaScriptCore/runtime/JSValue.h). Actually, Float32Array currently has the same problem, so if you want to file a follow-up bug to fix it both here and in Float32Array at the same time that's OK, but I'd prefer it if you could fix it here before the initial commit.

[Jian Li]

(In reply to comment #3)
> > WebCore/html/canvas/DataView.idl:62
> > +        [StrictTypeChecking, RequiresAllArguments=Raise] float getFloat32(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> > +            raises (DOMException);
> > +        [StrictTypeChecking, RequiresAllArguments=Raise] double getFloat64(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> > +            raises (DOMException);
> 
> I'm afraid that getFloat32 and getFloat64 require custom JSC bindings that check to see whether the return value is NaN (using isnan(), #include <wtf/MathExtras.h>) and if so return jsValue(nonInlineNaN()) (JavaScriptCore/runtime/JSValue.h). Actually, Float32Array currently has the same problem, so if you want to file a follow-up bug to fix it both here and in Float32Array at the same time that's OK, but I'd prefer it if you could fix it here before the initial commit.

Why do we need to check for NaN specifically? It seems that it has been taken care of (I checked JSC bindings, will check V8 bindings).

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #3)
> > > WebCore/html/canvas/DataView.idl:62
> > > +        [StrictTypeChecking, RequiresAllArguments=Raise] float getFloat32(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> > > +            raises (DOMException);
> > > +        [StrictTypeChecking, RequiresAllArguments=Raise] double getFloat64(in unsigned long byteOffset, in [Optional] boolean littleEndian)
> > > +            raises (DOMException);
> > 
> > I'm afraid that getFloat32 and getFloat64 require custom JSC bindings that check to see whether the return value is NaN (using isnan(), #include <wtf/MathExtras.h>) and if so return jsValue(nonInlineNaN()) (JavaScriptCore/runtime/JSValue.h). Actually, Float32Array currently has the same problem, so if you want to file a follow-up bug to fix it both here and in Float32Array at the same time that's OK, but I'd prefer it if you could fix it here before the initial commit.
> 
> Why do we need to check for NaN specifically? It seems that it has been taken care of (I checked JSC bindings, will check V8 bindings).

JSC encodes integer and pointer values in quiet NaNs. See JavaScriptCore/runtime/JSImmediate.h. We need to take care not to allow those bit patterns to flow back into the engine. I didn't see any tests during the construction of JSValue that canonicalize NaNs, so I think we need to do it using JSC custom bindings. V8 doesn't do this trick, so those bindings are OK (I think).

[Jian Li]

Created attachment 74719 [details]
Proposed Patch

All fixed.

[Jian Li]

Created attachment 74720 [details]
Proposed Patch

New patch to make the bot happy.

[Kenneth Russell]

Comment on attachment 74720 [details]
Proposed Patch

There's still a style error somewhere in this patch. Can you fix it?

[Jian Li]

Created attachment 74797 [details]
Proposed Patch

Fix style error.

[Kenneth Russell]

Comment on attachment 74797 [details]
Proposed Patch

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

This looks excellent. I apologize but there are a couple of issues in the custom bindings which I didn't notice the first time. Let me know if you'd prefer to fix them upon landing the patch, but for now marking r-.

> WebCore/bindings/js/JSDataViewCustom.cpp:151
> +        if (value < -128 || value > 127)
> +            return throwError(exec, createSyntaxError(exec, "The value to set to is out of range"));
> +        imp->setInt8(byteOffset, static_cast<unsigned char>(value), ec);

unsigned char -> char.

I don't think we should be raising exceptions for out-of-range values. Web IDL and ECMA-262 specify the conversions from e.g. double to octet, and they don't involve throwing exceptions.

> WebCore/bindings/js/JSDataViewCustom.cpp:155
> +        if (value < 0 || value > 255)
> +            return throwError(exec, createSyntaxError(exec, "The value to set to is out of range"));

Here as well.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:64
> +    if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() && !args[0]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }

The behavior of the StrictTypeChecking extended attribute in WebKit IDL was recently changed so that it is more compliant to ECMA-262. See https://bugs.webkit.org/show_bug.cgi?id=49218 . This code which converts to numbers needs to be changed to match the current autogenerated code, which doesn't throw exceptions.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:85
> +    if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() && !args[0]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }

Same here.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:110
> +    if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() && !args[0]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }
> +    if (!isUndefinedOrNull(args[1]) && !args[1]->IsNumber() && !args[1]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }

Shouldn't throw exceptions for conversions to numbers.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:117
> +    if (value < -128 || value > 127)
> +        return throwError("The value to set to is out of range", V8Proxy::SyntaxError);

Shouldn't throw exceptions for out-of-range values.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:136
> +    if (!isUndefinedOrNull(args[0]) && !args[0]->IsNumber() && !args[0]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }
> +    if (!isUndefinedOrNull(args[1]) && !args[1]->IsNumber() && !args[1]->IsBoolean()) {
> +        V8Proxy::throwTypeError();
> +        return notHandledByInterceptor();
> +    }

Shouldn't throw exceptions for conversions to numbers.

> WebCore/bindings/v8/custom/V8DataViewCustom.cpp:143
> +    if (value < 0 || value > 255)
> +        return throwError("The value to set to is out of range", V8Proxy::SyntaxError);

Shouldn't throw exceptions for out-of-range values.

[Jian Li]

Created attachment 74805 [details]
Proposed Patch

All problems in binding codes are fixed.

[Eric Seidel (no email)]

Attachment 74805 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6324034

[Jian Li]

Created attachment 74809 [details]
Proposed Patch

[Kenneth Russell]

Comment on attachment 74809 [details]
Proposed Patch

Great. Thanks for doing this. r=me

[Jian Li]

Committed as http://trac.webkit.org/changeset/72718.