Bug 146278

II was meaning bug 146235

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?

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?

(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.

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.

(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.

Created attachment 257922 [details] Patch

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

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.

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

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

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

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

Comment on attachment 257922 [details] Patch EWS is failing.

Created attachment 258052 [details] Fixing crash

Created attachment 258054 [details] Simplifying patch

(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...

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?

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.

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

ReadableStream will move to JS Builtins