Bug 146235 – [Streams API] Implement HighWaterMark

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement high water mark as defined in https://streams.spec.whatwg.org/#rs-constructor and https://streams.spec.whatwg.org/#validate-and-normalize-queuing-strategy

[youenn fablet]

Created attachment 255406 [details]
Patch

[Darin Adler]

Comment on attachment 255406 [details]
Patch

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

> Source/WebCore/bindings/js/ReadableJSStream.cpp:201
> +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy)

WebKIt coding style does not involve the word "get" for a function with a return value like this.

The strategy argument should be a JSObject&.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:205
> +    if (!jsHighWaterMark)
> +        return 1;

This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable.

However, we should return here if we had an exception before running toNumber.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:207
> +    double highWaterMark = jsHighWaterMark.toNumber(&exec);

IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:209
> +        throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN")));

Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings?

> Source/WebCore/bindings/js/ReadableJSStream.cpp:213
> +        throwVMError(&exec, createRangeError(&exec, ASCIILiteral("highWaterMark parameter is not a positive value")));

Same thought here.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:219
>  RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext)

It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:236
> +        highWaterMark = getHighWaterMark(exec, strategyObject);

Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:251
> +    RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction));

Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class.

> Source/WebCore/bindings/js/ReadableJSStream.h:88
> +    JSC::Strong<JSC::Unknown> m_sizeFunction;

I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks.

[Darin Adler]

Comment on attachment 255406 [details]
Patch

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

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:207
>> +    double highWaterMark = jsHighWaterMark.toNumber(&exec);
> 
> IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one.

To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other.

[youenn fablet]

Comment on attachment 255406 [details]
Patch

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

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:201
>> +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy)
> 
> WebKIt coding style does not involve the word "get" for a function with a return value like this.
> 
> The strategy argument should be a JSObject&.

ok

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:205
>> +        return 1;
> 
> This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable.

Good catch, we shoud check whether jsHighWaterMark is undefined or not.

> However, we should return here if we had an exception before running toNumber.

OK

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:207
>>> +    double highWaterMark = jsHighWaterMark.toNumber(&exec);
>> 
>> IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one.
> 
> To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other.

Right.

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:209
>> +        throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN")));
> 
> Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings?

I am not sure to totally understand the point here.
We can remove the ASCIILiteral string, it is not mandatory, that what is doing setDOMException().
Or we could set it to "Value is NaN".
Is it what is suggested?

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:219
>>  RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext)
> 
> It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that.

We could slightly reduce the binding by making the constructor not custom.
But we need anyway an ExecState, so we would use the globalExec retrieved from the source object.
I'll have some thoughts about it later.

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:236
>> +        highWaterMark = getHighWaterMark(exec, strategyObject);
> 
> Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed.

ok

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:251
>> +    RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction));
> 
> Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class.

It is covered by tests, but this is not recommended I guess.
Let's change that to a JSFunction*/nullptr

>> Source/WebCore/bindings/js/ReadableJSStream.h:88
>> +    JSC::Strong<JSC::Unknown> m_sizeFunction;
> 
> I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks.

I share the same concerns.
Let's do that as a follow up patch, which may also encompass source and maybe pull fulfill/reject promise callbacks.
I filed bug 146278 for that purpose.

[youenn fablet]

Comment on attachment 255406 [details]
Patch

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

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:201
>>> +static inline double getHighWaterMark(ExecState& exec, JSObject* strategy)
>> 
>> WebKIt coding style does not involve the word "get" for a function with a return value like this.
>> 
>> The strategy argument should be a JSObject&.

Done

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:205
>>> +        return 1;
>> 
>> This dead code should be removed. The get function can never return an “empty” JSValue like this. I don’t even think we need this local variable.
>> 
>> However, we should return here if we had an exception before running toNumber.
> 
> Good catch, we shoud check whether jsHighWaterMark is undefined or not.

Done

>>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:207
>>>> +    double highWaterMark = jsHighWaterMark.toNumber(&exec);
>>> 
>>> IF we get an exception from toNumber we should return that exception, not the "highWaterMark parameter is NaN" one.
>> 
>> To be clearer, I mean we should return so that exception is not overwritten bythe "highWaterMark parameter is NaN" one or any other.

Done

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:209
>>> +        throwVMError(&exec, createTypeError(&exec, ASCIILiteral("highWaterMark parameter is NaN")));
>> 
>> Isn’t there a more efficient way to do this perhaps with a single function call? It seems overkill here to have a custom strong that includes the argument name. Can we make this more consistent with other bindings?

Changed to "Value is NaN"

>> Source/WebCore/bindings/js/ReadableJSStream.cpp:213
>> +        throwVMError(&exec, createRangeError(&exec, ASCIILiteral("highWaterMark parameter is not a positive value")));
>> 
>> Same thought here.

Changed to "Not a positive value"

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:219
>>>  RefPtr<ReadableJSStream> ReadableJSStream::create(ExecState& exec, ScriptExecutionContext& scriptExecutionContext)
>> 
>> It’s a bad pattern to do so much bindings work here in the DOM class. I wonder if there is some way to improve the factoring. Sam Weinig might have some comments about that.

Added a FIXME

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:236
>>> +        highWaterMark = getHighWaterMark(exec, strategyObject);
>> 
>> Need to return after this if there is an exception before calling getPropertyFromObject below because side effects can be observed.

Done

>>> Source/WebCore/bindings/js/ReadableJSStream.cpp:251
>>> +    RefPtr<ReadableJSStream> readableStream = adoptRef(*new ReadableJSStream(scriptExecutionContext, exec, jsSource, highWaterMark, sizeFunction));
>> 
>> Is it correct to pass an empty JSValue for sizeFunction when there is no function? How about passing "undefined" instead? Also we should change the type to be JSFunction* (or something like that) since we are doing the type checking here outside the class.

Done

>>> Source/WebCore/bindings/js/ReadableJSStream.h:88
>>> +    JSC::Strong<JSC::Unknown> m_sizeFunction;
>> 
>> I’m concerned that this could result in a reference cycle, if the size function has captured a reference to the stream. I guess I should have the same worry about m_errorFunction. Generally speaking I think we need to use GC marking rather than strong references in this class to avoid cycles and leaks.

Added a FIXME referring to the filed bug

[youenn fablet]

Created attachment 255546 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 255546 [details]
Patch for landing

Rejecting attachment 255546 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 255546, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4887571318439936

[youenn fablet]

Created attachment 255547 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 255547 [details]
Patch for landing

Clearing flags on attachment: 255547

Committed r185953: <http://trac.webkit.org/changeset/185953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.