Bug 157231 – [Web IDL] Pass even more types by reference

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Pass even more types by references. Some types such as typed arrays are still passed by pointer even if the parameter is not marked as nullable in the IDL.

[Chris Dumez]

Created attachment 277811 [details]
Patch

[Darin Adler]

Comment on attachment 277811 [details]
Patch

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

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:164
> +        Ref<Uint8Array> pendingKey = m_pendingKeys.takeFirst();

Seems like this should be auto.

> Source/WebCore/Modules/encryptedmedia/MediaKeySession.h:103
> +        PendingKeyRequest(const String& mimeType, Ref<Uint8Array>&& initData)
> +            : mimeType(mimeType)
> +            , initData(WTFMove(initData))
> +        { }

Can we get rid of this constructor and use initializer lists instead?

> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:114
> +    Ref<MediaKeySession> session = MediaKeySession::create(context, this, keySystem());

Should use auto.

> Source/WebCore/Modules/encryptedmedia/MediaKeys.idl:32
> -    // FIXME: The default value for the 'type' parameter is wrong.
> -    [CallWith=ScriptExecutionContext, RaisesException] MediaKeySession createSession(optional DOMString type = "undefined", optional Uint8Array initData = null);
> +    [CallWith=ScriptExecutionContext, RaisesException] MediaKeySession createSession(DOMString type, Uint8Array initData);

Took me a while to think through why this doesn’t change any behavior (other than which exception is raised).

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:228
> +    RefPtr<ArrayBuffer> arrayBuffer(data.buffer());
>      ASSERT(arrayBuffer);
>      send(*arrayBuffer, ec);

I would have written this as a one-liner without the assertion.

> Source/WebCore/Modules/webaudio/AnalyserNode.h:64
> -    void getFloatFrequencyData(JSC::Float32Array* array) { m_analyser.getFloatFrequencyData(array); }
> -    void getByteFrequencyData(JSC::Uint8Array* array) { m_analyser.getByteFrequencyData(array); }
> -    void getByteTimeDomainData(JSC::Uint8Array* array) { m_analyser.getByteTimeDomainData(array); }
> +    void getFloatFrequencyData(RefPtr<JSC::Float32Array>&& array) { m_analyser.getFloatFrequencyData(array.get()); }
> +    void getByteFrequencyData(RefPtr<JSC::Uint8Array>&& array) { m_analyser.getByteFrequencyData(array.get()); }
> +    void getByteTimeDomainData(RefPtr<JSC::Uint8Array>&& array) { m_analyser.getByteTimeDomainData(array.get()); }

This seems peculiar. These functions don’t take ownership so raw pointers makes more sense than RefPtr&&. So why the change?If the generated code is passing a RefPtr and we can’t stop it from doing that, then can we at least make this const RefPtr& instead of RefPtr&&? With a FIXME, I think. It’s not good to take && when we don’t pass ownership.

> Source/WebCore/Modules/webaudio/AudioContext.h:144
> +    RefPtr<PeriodicWave> createPeriodicWave(Float32Array& real, Float32Array& imag, ExceptionCode&);

Maybe rename "imag" to "imaginary"?

> Source/WebCore/Modules/webaudio/AudioParam.h:92
> -    void setValueCurveAtTime(Float32Array* curve, float time, float duration) { m_timeline.setValueCurveAtTime(curve, time, duration); }
> +    void setValueCurveAtTime(RefPtr<Float32Array>&& curve, float time, float duration) { m_timeline.setValueCurveAtTime(curve.get(), time, duration); }

Same comment as above. Old code seems better than new. Raw pointer is a better type here than RefPtr&&.

> Source/WebCore/Modules/webaudio/BiquadFilterNode.h:65
> -    void getFrequencyResponse(const Float32Array* frequencyHz,
> -                              Float32Array* magResponse,
> -                              Float32Array* phaseResponse);
> +    void getFrequencyResponse(RefPtr<Float32Array>&& frequencyHz, RefPtr<Float32Array>&& magResponse, RefPtr<Float32Array>&& phaseResponse);

Same thing again!?

> Source/WebCore/Modules/webaudio/BiquadFilterNode.idl:49
> +    void getFrequencyResponse(Float32Array? frequencyHz,
> +                              Float32Array? magResponse,
> +                              Float32Array? phaseResponse);

Lets put this on one line?

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:52
> +    Ref<PeriodicWave> waveTable = adoptRef(*new PeriodicWave(sampleRate));

auto would be better here

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:53
> +    size_t numberOfComponents = real.length();

We shouldn’t use a local variable here.

> Source/WebCore/Modules/websockets/WebSocket.cpp:358
> +    RefPtr<ArrayBuffer> arrayBuffer(arrayBufferView.buffer());
> +    m_channel->send(*arrayBuffer, arrayBufferView.byteOffset(), arrayBufferView.byteLength());

Not sure why this uses a local variable, or why it’s a RefPtr. But possibly risky to change :-(

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3690
> +            if ($codeGenerator->IsTypedArrayType($argType) and $argType ne "ArrayBuffer") {

Really irritating we have to have so many type names hard coded; not new but continues to be annoying.

> Source/WebCore/html/HTMLMediaElement.h:238
> +    void webkitAddKey(const String& keySystem, Uint8Array& key, RefPtr<Uint8Array>&& initData, const String& sessionId, ExceptionCode&);

This function doesn’t take ownership of the initData. It reads the data out of the array, it doesn’t adopt the array. The type should be Uint8Array*, not RefPtr<UintArray>&&.

> Source/WebCore/html/ImageData.cpp:55
>      RefPtr<ImageData> data = adoptRef(new ImageData(size));

I think this should use Ref rather than RefPtr, and should use auto (although then we’d need WTFMove below).

> Source/WebCore/html/ImageData.cpp:68
>      return adoptRef(new ImageData(size));

Should be adoptRef(*new instead of adoptRef(new).

> Source/WebCore/html/ImageData.cpp:83
> +    return adoptRef(new ImageData(size, WTFMove(byteArray)));

Should be adoptRef(*new instead of adoptRef(new).

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:187
> +void WebGL2RenderingContext::texImage3D(GC3Denum target, GC3Dint level, GC3Dint internalformat, GC3Dsizei width, GC3Dsizei height, GC3Dsizei depth, GC3Dint border, GC3Denum format, GC3Denum type, RefPtr<ArrayBufferView>&& pixels)

These functions should omit the argument names rather than listing them all and then doing UNUSED_PARAM on all of them. The compiler will check this against the header; there’s no need for us to restate all the argument names.

> Source/WebCore/html/canvas/WebGL2RenderingContext.h:49
> -    void getBufferSubData(GC3Denum target, GC3Dint64 offset, ArrayBufferView* returnedData);
> +    void getBufferSubData(GC3Denum target, GC3Dint64 offset, RefPtr<ArrayBufferView>&& returnedData);

I am puzzled by all of these. Are we passing ownership? I don’t understand why we are making a change here.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4574
>      if (isContextLostOrPending())
>          return;
> -    if (!v) {
> -        synthesizeGLError(GraphicsContext3D::INVALID_VALUE, functionName, "no array");
> -        return;
> -    }
> -    vertexAttribfvImpl(functionName, index, v->data(), v->length(), expectedSize);
> +
> +    vertexAttribfvImpl(functionName, index, v.data(), v.length(), expectedSize);

I like it better without the blank line.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:145
> -    void bufferSubData(GC3Denum target, long long offset, ArrayBufferView* data, ExceptionCode&);
> +    void bufferSubData(GC3Denum target, long long offset, RefPtr<ArrayBufferView>&& data, ExceptionCode&);

Another one of these “why are we using &&?” cases.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.idl:483
> +    [StrictTypeChecking, RaisesException] void         bufferData(GLenum target, ArrayBuffer data, GLenum usage);
> +    [StrictTypeChecking, RaisesException] void         bufferData(GLenum target, ArrayBufferView data, GLenum usage);

We should fix the strange formatting here at some point. All those spaces that someone originally used to line things up; should just use single spaces and not try to line things up.

> Source/WebCore/testing/Internals.cpp:2688
>      Ref<TimeRanges> ranges = TimeRanges::create();

Should use auto.

[Chris Dumez]

Comment on attachment 277811 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AnalyserNode.h:64
>> +    void getByteTimeDomainData(RefPtr<JSC::Uint8Array>&& array) { m_analyser.getByteTimeDomainData(array.get()); }
> 
> This seems peculiar. These functions don’t take ownership so raw pointers makes more sense than RefPtr&&. So why the change?If the generated code is passing a RefPtr and we can’t stop it from doing that, then can we at least make this const RefPtr& instead of RefPtr&&? With a FIXME, I think. It’s not good to take && when we don’t pass ownership.

I actually went back and forth on this. The thing is that the bindings have a RefPtr<> (because they create the TypedArray from the JSValue). Therefore, I have updated the generated bindings to either pass a Ref<>&& (if not nullable) or a RefPtr<>&& (if nullable). We have a smart pointer anyway and the object gets created only to call the implementation. Therefore, it makes sense for the bindings to transfer ownership. The thing is that in most cases, the implementation does not actually take ownership, they merely get the internal data from the array. In some cases though they do and we avoid some refcounting churn.

I am not sure what's the best approach here. I could leave the bindings generator untouched and pass either a reference / raw pointer. It will cause a bit more refcounting churns in the few cases where the implementation does want to adopt the array passed in. However, in most cases, it will be a bit more readable.

What do you think?

>> Source/WebCore/html/HTMLMediaElement.h:238
>> +    void webkitAddKey(const String& keySystem, Uint8Array& key, RefPtr<Uint8Array>&& initData, const String& sessionId, ExceptionCode&);
> 
> This function doesn’t take ownership of the initData. It reads the data out of the array, it doesn’t adopt the array. The type should be Uint8Array*, not RefPtr<UintArray>&&.

As explained previously, the bindings create this RefPtr for us and now transfer ownership of it to us. It does not build if I use a raw pointer here, unless I change the bindings to keep passing a raw pointer (which I am willing to do). For clarity, the generated bindings look like this at the moment:

EncodedJSValue JSC_HOST_CALL jsHTMLMediaElementPrototypeFunctionWebkitAddKey(ExecState* state)
{
    JSValue thisValue = state->thisValue();
    auto castedThis = jsDynamicCast<JSHTMLMediaElement*>(thisValue);
    if (UNLIKELY(!castedThis))
        return throwThisTypeError(*state, "HTMLMediaElement", "webkitAddKey");
    ASSERT_GC_OBJECT_INHERITS(castedThis, JSHTMLMediaElement::info());
    auto& impl = castedThis->wrapped();
    if (UNLIKELY(state->argumentCount() < 2))
        return throwVMError(state, createNotEnoughArgumentsError(state));
    ExceptionCode ec = 0;
    String keySystem = valueToStringWithUndefinedOrNullCheck(state, state->argument(0));
    if (UNLIKELY(state->hadException()))
        return JSValue::encode(jsUndefined());
    RefPtr<Uint8Array> key = toUint8Array(state->argument(1));
    if (UNLIKELY(state->hadException()))
        return JSValue::encode(jsUndefined());
    if (UNLIKELY(!key))
        return throwVMTypeError(state);
    RefPtr<Uint8Array> initData = state->argument(2).isUndefined() ? nullptr : toUint8Array(state->uncheckedArgument(2));
    if (UNLIKELY(state->hadException()))
        return JSValue::encode(jsUndefined());
    String sessionId = state->argument(3).isUndefined() ? String() : state->uncheckedArgument(3).toString(state)->value(state);
    if (UNLIKELY(state->hadException()))
        return JSValue::encode(jsUndefined());
    impl.webkitAddKey(keySystem, key.releaseNonNull(), WTFMove(initData), sessionId, ec);
    setDOMException(state, ec);
    return JSValue::encode(jsUndefined());
}

Note that toUint8Array() ends up constructing an object and is not a mere casting function. It ends up calling:
    PassRefPtr<typename Adaptor::ViewType> typedImpl()
    {
        return Adaptor::ViewType::create(buffer(), byteOffset(), length());
    }

in JSGenericTypedArrayView.h

[Darin Adler]

Seems OK to land without fixing this.

I do think it’s inelegant for a function to have to take a certain type because of the way it’s called when that type doesn’t make complete. I wonder if there’s any way we could write the code so that the RefPtr would turn itself into a raw pointer as needed; unfortunately I can’t think of a way to do that. The reason we don’t have an operator T* in RefPtr is that we think it’s unsafe, but it’s not unsafe when passing an argument to a function. An over engineered solution is that we could invent a new wrapper type that holds a RefPtr that knows how to turn itself into a RefPtr&& and into a raw pointer. Then we’d just have to use something like "AutoGetRefPtrArgument<UintArray>" instead of "auto" or "RefPtr<Uint8Array>" to store the result of toUint8Array.

I also noticed a related problem for String. We don’t take advantage of cases where we only need a StringView; we always make a String. We don’t make it possible for classes to take ownership of the String; we don’t call WTFMove even though the strings are just temporaries. Maybe the String thing could be an actual performance boost for hot bindings.

I suppose the bigger problem is having to make these reference counted wrappers at all just to pass arguments to code that doesn’t need a wrapper, just needs a view of the data.

[Chris Dumez]

Created attachment 277832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277832 [details]
Patch

Clearing flags on attachment: 277832

Committed r200298: <http://trac.webkit.org/changeset/200298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.