Bug 146278 – [Streams API] Remove use of JSC::Strong as much as possible

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

[youenn fablet]

Following on bug 142635, it might be good to remove usage of JSC::Strong for  errorFunction, source and sizeFunction, as we could end up in ref cycles.

[youenn fablet]

II was meaning bug 146235

[youenn fablet]

It is suggested to use GC marking.
I am not yet very familiar with it.
As in the spec, we could put errorFunction, source and sizeFunction in JSReadableStream internalSlots.
This could be extended to pull fullfil/reject JS callbacks whenever created.

Or we may reuse what is behind CachedAttribute.

Any other possibility?

[youenn fablet]

Darin is suggesting to use GC marking.
I am not yet very familiar with it.

As written in the streams spec, we could put errorFunction, source and sizeFunction in JSReadableStream internalSlots.
This could be extended to pull fullfil/reject JS callbacks whenever created.

We might also reuse what is behind CachedAttribute IDL.

Any other possibility?

[Darin Adler]

(In reply to comment #2)
> It is suggested to use GC marking.
> I am not yet very familiar with it.

First of all, I think the main thing to do is to actually make test cases that reproduce these reference cycles to ensure the objects aren’t leaked. I’m not sure exactly what the best way to make such tests is, but it seems even more important to do that than to fix the bugs, so we don’t introduce the bugs in the future.

Marking is relatively straightforward.

The class needs to have the JSCustomMarkFunction attribute in the IDL. Then custom wrapper code needs to implement a visitAdditionalChildren function. The function then needs to call addOpaqueRoot on each object we need to keep alive. Some examples to look at are JSXMLHttpRequest::visitAdditionalChildren and JSTreeWalker::visitAdditionalChildren.

[youenn fablet]

This is not as straightforward as I thought.
I would think using JSCustomMark and passing from JSC::Strong to JSC::WriteBarrier would work, just as done for Cached attributes.

It seems I can make it work for the enqueued chunks but not always for the stored error where I get some crashes related to GC.

I will upload some code probably tomorrow about that.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #2)
> > It is suggested to use GC marking.
> > I am not yet very familiar with it.
> 
> First of all, I think the main thing to do is to actually make test cases
> that reproduce these reference cycles to ensure the objects aren’t leaked.
> I’m not sure exactly what the best way to make such tests is, but it seems
> even more important to do that than to fix the bugs, so we don’t introduce
> the bugs in the future.

I do not think we can easily create tests that show reference cycles.
The GC is expected to not be monitorable from web apps.

We might be able (?) to improve test runner to actually dump a list of JS objects that are not collected at the end of a test.
This might then allow us to generate tests checking reference cycles.

[youenn fablet]

Created attachment 257922 [details]
Patch

[youenn fablet]

(In reply to comment #7)
> Created attachment 257922 [details]
> Patch

[youenn fablet]

This patch tries to remove all JSC::Strong usage from ReadableStream.

The separation between JSReadableStream and ReadableStream makes things more complex than it would be if implemented in JSC.

This patch could probably be simplified by using RefCount instead of adding yet another counter for ensuring JS ReadableStream wrapper is not collected.

[Build Bot]

Comment on attachment 257922 [details]
Patch

Attachment 257922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3116

New failing tests:
streams/reference-implementation/bad-underlying-sources.html
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 257925 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 257922 [details]
Patch

Attachment 257922 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3141

New failing tests:
streams/reference-implementation/bad-underlying-sources.html
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 257926 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

Comment on attachment 257922 [details]
Patch

EWS is failing.

[youenn fablet]

Created attachment 258052 [details]
Fixing crash

[youenn fablet]

Created attachment 258054 [details]
Simplifying patch

[Xabier Rodríguez Calvar]

(In reply to comment #6)
> We might be able (?) to improve test runner to actually dump a list of JS
> objects that are not collected at the end of a test.
> This might then allow us to generate tests checking reference cycles.

I might be wrong, but I don't know if this is even possible, because I don't think GC is completely deterministic, so even if we add the still living objects in the expections, it would create tons of flakiness. Am I right? Maybe a special flag that you can activate for special tests testing it...

[Xabier Rodríguez Calvar]

Comment on attachment 258054 [details]
Simplifying patch

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

Nice patch, nice tests. I asked about some concerts that are probably nothing and some nits about the tests.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:137
> +    RefPtr<ReadableJSStream> protectedStream(this);

I guess the case of this line is ensuring that the method is reentrant, right? I ask because you don't use it anywhere else in rest of the function.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:165
> +    RefPtr<ReadableJSStream> protectedStream(this);

Ditto.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:205
> +    RefPtr<ReadableJSStream> protectedStream(this);

Ditto.

> LayoutTests/streams/readable-stream-gc.html:73
> +            controller.enqueue({ one: "test"});
> +            controller.enqueue({ two: "test"});
> +            controller.enqueue([ 10, { three: 'test' }]);

Nit: It could make sense to store these values in an array inside the test and then checking them below against the value for the index with assert_object_equals. It would make the test just a bit more maintainable, though you would't be able (probably) of though a so thorough type check like the one you do for the object of the [1] element of the third array...

> LayoutTests/streams/readable-stream-gc.html:88
> +       reader.read().then(test4.step_func(function(iterator) {
> +            assert_equals(iterator.value.one, "test");
> +        }));
> +        reader.read().then(test4.step_func(function(iterator) {
> +            assert_equals(iterator.value.two, "test");
> +        }));
> +        reader.read().then(test4.step_func(function(iterator) {
> +            assert_equals(iterator.value[1].three, "test");
> +            test4.done();

<paranoid mode>I would add a counter for read calls to test just before the done and test it for 3. That way you can ensure that the promise was resolve the 3 times and that nothing funny happened inbetween.</paranoid mode>

> LayoutTests/streams/readable-stream-gc.html:93
> +var test4b = async_test('Readable stream enqueued values when pulling should not be collectable');

I would renumber the test here. It is a PITA, but otherwise they could get out of hand with test4b1IPromiseNotToPutAnythingElseInBetween.

> LayoutTests/streams/readable-stream-gc.html:100
> +                controller.enqueue({ one: "test"});
> +                controller.enqueue({ two: "test"});

Same nit as for test4.

> LayoutTests/streams/readable-stream-gc.html:105
> +            controller.enqueue([ 10, { three: 'test' }]);
> +            controller.enqueue([ 10, { four: 'test' }]);

Ditto.

> LayoutTests/streams/readable-stream-gc.html:108
> +            garbageCollectAndDo(function() { });

What do you get with this? Would it make sense to create a function to force GC instead of calling a phony one?

> LayoutTests/streams/readable-stream-gc.html:122
> +       reader.read().then(test4b.step_func(function(iterator) {
> +            assert_equals(iterator.value.one, "test");
> +        }));
> +        reader.read().then(test4b.step_func(function(iterator) {
> +            assert_equals(iterator.value.two, "test");
> +        }));
> +        reader.read().then(test4b.step_func(function(iterator) {
> +            assert_equals(iterator.value[1].three, "test");
> +            test4b.done();

Read counter?

> LayoutTests/streams/readable-stream-gc.html:131
> +            controller.error({ one: "test"});

I would keep the error outsize and create an actual Error that I could check against later.

> LayoutTests/streams/readable-stream-gc.html:143
> +var test5b = async_test('Readable stream error when pulling should not be collectable');

Renumbering?

> LayoutTests/streams/readable-stream-gc.html:147
> +            controller.error({ one: "test"});

Keep error outside and check later against it.

> LayoutTests/streams/readable-stream-gc.html:166
> +            controller.enqueue({ one: "test"});
> +            controller.enqueue({ two: "test"});
> +            controller.enqueue([ 10, { three: 'test' }]);

Again elements in array?

> LayoutTests/streams/readable-stream-gc.html:183
> +       reader.read().then(test6.step_func(function(iterator) {
> +            assert_equals(iterator.value.one, "test");
> +        }));
> +        reader.read().then(test6.step_func(function(iterator) {
> +            assert_equals(iterator.value.two, "test");
> +        }));
> +        reader.read().then(test6.step_func(function(iterator) {
> +            assert_equals(iterator.value[1].three, "test");
> +            assert_equals(sizeCalledCounter, 3);

Read count? Against sizeCalledCounter?

[youenn fablet]

Thanks for the feedback.

> > Source/WebCore/bindings/js/ReadableJSStream.cpp:137
> > +    RefPtr<ReadableJSStream> protectedStream(this);
> 
> I guess the case of this line is ensuring that the method is reentrant,
> right? I ask because you don't use it anywhere else in rest of the function.

It is not related to reentrancy but to GC actually.
It is quite similar to using setPendingActivity/unsetPendingActivity so that the JSReadableStream wrapper is not collected throughout start/pull/cancel.

I decided not to use setPendingActivity/unsetPendingActivity as it does not work well when used inside JS promise callbacks.
If promise is never resolved nor rejected, we would have made a setPendingActivity call but we would not get a corresponding unsetPendingActivity call.
With RefPtr protection, we do not have this issue.

As of the test comments, I agree with most of them.
Renumbering them and improving the value read checks as done in the other reference tests are good points.

If needed, I can provide a patch with updated tests.
But I think the tests can be updated at cq time/at follow-up patch time.

[Xabier Rodríguez Calvar]

(In reply to comment #19)
> But I think the tests can be updated at cq time/at follow-up patch time.

Sounds good.

[youenn fablet]

ReadableStream will move to JS Builtins