Bug 144790

Created attachment 252719 [details] Patch

(In reply to comment #1) > Created attachment 252719 [details] > Patch Clean-up of read callbacks should be aligned with https://bugs.webkit.org/show_bug.cgi?id=144799 This may be done at landing/rebasing time.

Comment on attachment 252719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252719&action=review > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:141 > + m_readRequests.clear(); Plan is to do cleaning in releaseStream once bug 144799 is landed. > LayoutTests/streams/reference-implementation/bad-underlying-sources.html:64 > +var test3 = async_test('Underlying source: throwing pull getter (second pull)', { timeout: 50 }); Putting timeout to 50 now as test is timeouting. Plan is to remove this timeout once test is passing.

Comment on attachment 252719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252719&action=review > Source/JavaScriptCore/runtime/IteratorOperations.h:40 > +JS_EXPORT_PRIVATE JSObject* createIterResultObject(ExecState*, JSValue, bool done); If we are going to export this, I suggest we make it more like the functions above: 1) give the JSValue argument the name "iterator" 2) use "iterator", not "iter", in the function name > Source/WebCore/ForwardingHeaders/runtime/IteratorOperations.h:5 > + No need for this extra blank line > Source/WebCore/Modules/streams/ReadableStream.h:39 > +#include <runtime/JSCJSValue.h> This isn’t needed. A file can define a function returning a class type with only a forward declaration of that type, so we should forward-declare JSValue here, not include the header. > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:39 > +using namespace JSC; We normally don’t use this in new code for short names like these. > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:118 > + size_t readRequestCount = m_readRequests.size(); > + for (size_t i = 0; i < readRequestCount; ++i) { > + ReadSuccessCallback successCallback = WTF::move(m_readRequests[i].first); > + successCallback(jsUndefined(), true); > + } No need to move these callbacks into locals as we call them. Is there something that protects us from getting called re-entrantly that thus prevents m_readRequests from being modified from underneath us? If so, this should be written much more tightly like this: for (auto& request : m_readRequests) request.first(jsUndefined(), true); But also, this points out that the use of a pair for the request object is not good for clarity. We should instead just use a simple struct so the two functions have names instead of being named first and last. If there is nothing that prevents the reentrant calls, then we have a problem either way. > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:140 > + size_t readRequestCount = m_readRequests.size(); > + for (size_t i = 0; i < readRequestCount; ++i) { > + ReadErrorCallback errorCallback = WTF::move(m_readRequests[i].second); > + errorCallback(); > + } As above, should be done much more simply: for (auto& request : m_readRequests) request.second(); > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:160 > + m_readRequests.append(std::make_pair(WTF::move(successCallback), WTF::move(errorCallback))); Might also try this more terse syntax (if it works): m_readRequests.append({ WTF::move(successCallback), WTF::move(errorCallback) }); > Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:51 > + auto successCallback = [wrapper, this](const JSValue& chunk, bool done) mutable { What prevents the lambda from outliving the JSReadableStreamReader object in "this"? I don’t think this is safe or correct. Instead of capturing "this", I think we need to do something else. We should change DeferredWrapper to add a function that returns ExecState& for the globalExec we need. > Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:53 > + JSValue result = createIterResultObject(globalObject()->globalExec(), chunk, done); > + wrapper.resolve(result); No need for the local variable here. Reads better without it, I think. > Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:55 > + auto failureCallback = [wrapper, this]() mutable { What prevents the lambda from outliving the JSReadableStreamReader object in "this"? I don’t think this is safe or correct. JSReadableStreamReader::closed seems to have the same mistake. Instead of capturing "this", I think we need to capture a reference to the implementation so we don’t need to call impl() inside the lambda. I believe the lifetime of the reader is guaranteed. Or to make it even clearer we could add a ReadableStreamReader& argument to the ReadableStreamReader callback function types and use that. > Source/WebCore/bindings/js/ReadableJSStream.cpp:189 > + return JSValue(); This is dangerous. The JavaScript engine doesn’t deal well with these. Better to return jsUndefined(). > Source/WebCore/bindings/js/ReadableJSStream.h:87 > + virtual bool hasValue() override; > + virtual JSC::JSValue read() override; Can these overrides be private please? Any reason to call them non-polymorphically? Can this class be marked final?

Comment on attachment 252719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252719&action=review >> Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:55 >> + auto failureCallback = [wrapper, this]() mutable { > > What prevents the lambda from outliving the JSReadableStreamReader object in "this"? I don’t think this is safe or correct. JSReadableStreamReader::closed seems to have the same mistake. Instead of capturing "this", I think we need to capture a reference to the implementation so we don’t need to call impl() inside the lambda. I believe the lifetime of the reader is guaranteed. Or to make it even clearer we could add a ReadableStreamReader& argument to the ReadableStreamReader callback function types and use that. Very good catch. The first "ref" approach should work. The second "argument" probably not, as the reader would be disposed when its js counterpart is disposed. This would lead to releasing the stream and the closed promise callback to never been called when the stream gets closed (hence I believe no crash in the current implementation). I filed 144869 for that purpose and will file a quick fix for it. I wonder though whether we should go further than that. Maybe we could reuse the same reader/stream link as for controller/stream (reader lifetime controlled by stream, reader delegating ref counting to the stream). This would remove the need for ReadableStream::release/releaseButKeepLocked/lock as well as removing the need to keep a ref of the reader within the lambdas. One downside is that we may need specialized stream-independent readers to cover the case of closed/errored streams. Major difference would be that these stream-independent readers would need to refcount themselves. Any thoughts?

(In reply to comment #5) > Any thoughts? That sounds like a promising direction. Fewer separate reference counted objects is likely better.

(In reply to comment #5) > Comment on attachment 252719 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252719&action=review > > >> Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:55 > >> + auto failureCallback = [wrapper, this]() mutable { > > > > What prevents the lambda from outliving the JSReadableStreamReader object in "this"? I don’t think this is safe or correct. JSReadableStreamReader::closed seems to have the same mistake. Instead of capturing "this", I think we need to capture a reference to the implementation so we don’t need to call impl() inside the lambda. I believe the lifetime of the reader is guaranteed. Or to make it even clearer we could add a ReadableStreamReader& argument to the ReadableStreamReader callback function types and use that. > > Very good catch. > The first "ref" approach should work. > The second "argument" probably not, as the reader would be disposed when its > js counterpart is disposed. > This would lead to releasing the stream and the closed promise callback to > never been called when the stream gets closed (hence I believe no crash in > the current implementation). > > I filed 144869 for that purpose and will file a quick fix for it. > > I wonder though whether we should go further than that. > Maybe we could reuse the same reader/stream link as for controller/stream > (reader lifetime controlled by stream, reader delegating ref counting to the > stream). > This would remove the need for > ReadableStream::release/releaseButKeepLocked/lock as well as removing the > need to keep a ref of the reader within the lambdas. Filed bug 144907 for that purpose.

Created attachment 253325 [details] Version matching bug 145110 and 145116 patches

Comment on attachment 253325 [details] Version matching bug 145110 and 145116 patches View in context: https://bugs.webkit.org/attachment.cgi?id=253325&action=review > Source/WebCore/Modules/streams/ReadableStream.h:78 > + typedef std::function<void(const JSC::JSValue&)> FailureCallback; Should not be const&, just JSC::JSValue. > Source/WebCore/Modules/streams/ReadableStream.h:83 > + typedef std::function<void(const JSC::JSValue&)> ReadSuccessCallback; Should not be const&, just JSC::JSValue. > Source/WebCore/Modules/streams/ReadableStream.h:112 > + class ReadCallbacks { > + public: > + ReadCallbacks(ReadSuccessCallback&& successCallback, FailureCallback&& failureCallback) > + : m_successCallback(successCallback) > + , m_failureCallback(failureCallback) { } > + ReadSuccessCallback m_successCallback; > + FailureCallback m_failureCallback; > + }; Should use a struct, not as class, and not use the m_ prefixes and not use a constructor, just use struct construction syntax.

Created attachment 253392 [details] Rebasing patch, removing const JSValue& and using struct

Comment on attachment 253392 [details] Rebasing patch, removing const JSValue& and using struct View in context: https://bugs.webkit.org/attachment.cgi?id=253392&action=review > Source/WebCore/Modules/streams/ReadableStream.cpp:151 > + ReadCallbacks callbacks = { WTF::move(successCallback), WTF::move(failureCallback) }; > + m_readRequests.append(callbacks); Can also write this as a one-liner if you prefer: m_readRequests.append(ReadCallbacks { WTF::move(successCallback), WTF::move(failureCallback) }); I wish you could write this; I don’t know precisely why it doesn’t work or if Vector could be changed to make it work: m_readRequests.append({ WTF::move(successCallback), WTF::move(failureCallback) }); > Source/WebCore/bindings/js/ReadableJSStream.h:76 > + Unneeded extra blank line here.

Created attachment 253436 [details] Updating according review

Created attachment 253442 [details] Using JSValue() to convey done=true

(In reply to comment #13) > Created attachment 253442 [details] > Using JSValue() to convey done=true Compared to previous last two patches, the only behavorial change is to use JSValue() to convey the case of an iterator with no value and done being true. It might be possible to actually enqueue an undefined value using the controller enqueue method. This also raises the question of the controller error method, for which we create a default error if there is none passed to it. We should check whether we want to support undefined as possible error value.

Ping review?

Comment on attachment 253442 [details] Using JSValue() to convey done=true View in context: https://bugs.webkit.org/attachment.cgi?id=253442&action=review I still need to check this more thoroughly, but changing those tests seems wrong. > LayoutTests/streams/reference-implementation/bad-underlying-sources.html:34 > - get pull() { > + pull: function() { If you remove this, the test is useless and duplicated. The intention here is to test the getter. > LayoutTests/streams/reference-implementation/bad-underlying-sources.html:69 > - get pull() { > + pull: function() { Ditto. > LayoutTests/streams/reference-implementation/bad-underlying-sources.html:122 > - get cancel() { > + cancel: function() { Ditto. > LayoutTests/streams/reference-implementation/bad-underlying-sources.html:162 > - get strategy() { > + strategy: function() { Ditto.

Comment on attachment 253442 [details] Using JSValue() to convey done=true View in context: https://bugs.webkit.org/attachment.cgi?id=253442&action=review >> LayoutTests/streams/reference-implementation/bad-underlying-sources.html:34 >> + pull: function() { > > If you remove this, the test is useless and duplicated. The intention here is to test the getter. I see. Will fix that, thanks.

Created attachment 253790 [details] Patch Fixed some style issues and readded the getters at bad-underlying-tests

Comment on attachment 253790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253790&action=review Thanks for the improvements. > LayoutTests/streams/reference-implementation/readable-stream-templated.html:618 > +// FIXME: Activate these tests once Promise handling is supported in start function. Just some background why we need to disable these tests. Some of these tests are passing, some are timeouting. If we set timeout: 50 to handle timeouting tests, the other passing templated tests might become flaky. Since, we do not support promise as returned value within start yet, it is ok to comment out these tests anyway. Maybe we could add this explanation to the LayoutTests changelog? As a side note, I think the way WebKit produces output from testharness tests is not optimal. I filed bug 145313 for handling good test outputs for timeouting testharness tests. In short, testharness.js should timeout before WebKit test runner timeouts so as to produce PASS/FAIL/TIMEOUT for each individual test. Once done, we will be able to remove timeout: 50 for all of our tests. This may require adding to the test runner a timeout() method to get WKR timeout() from testharness.js and set testharness.js timeout to a value a bit smaller. Patch in bug 145313 is much simpler than that as a first try. It may also be the case that output for erroneous/badly formatted tests could be improved.

(In reply to comment #11) > Comment on attachment 253392 [details] > Rebasing patch, removing const JSValue& and using struct > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253392&action=review > > I wish you could write this; I don’t know precisely why it doesn’t work or > if Vector could be changed to make it work: > > m_readRequests.append({ WTF::move(successCallback), > WTF::move(failureCallback) }); I believe it is not possible without changing Vector. IIUC, append is a templated method, and the method template type cannot be retrieved from "{ ... }". To solve this, we could add a regular append method, something like: void append(ValueType&& value) { append<ValueType>(std::forward<ValueType>(value)); } I think this can be done as a follow-up patch.

Comment on attachment 253790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253790&action=review > Source/WebCore/Modules/streams/ReadableStream.h:84 > + void read(ReadSuccessCallback, FailureCallback); A small improvement might be to pass callbacks as && for read() and closed() methods of ReadableStream and ReadableStreamReader. This might be done when landing this patch.

Comment on attachment 253790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253790&action=review >> Source/WebCore/Modules/streams/ReadableStream.h:84 >> + void read(ReadSuccessCallback, FailureCallback); > > A small improvement might be to pass callbacks as && for read() and closed() methods of ReadableStream and ReadableStreamReader. > This might be done when landing this patch. For move-only objects like std::function, std::unique_ptr, and many others, there is no reason to pass as &&. Doesn’t improve clarity or efficiency.

Comment on attachment 253790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253790&action=review >>> Source/WebCore/Modules/streams/ReadableStream.h:84 >>> + void read(ReadSuccessCallback, FailureCallback); >> >> A small improvement might be to pass callbacks as && for read() and closed() methods of ReadableStream and ReadableStreamReader. >> This might be done when landing this patch. > > For move-only objects like std::function, std::unique_ptr, and many others, there is no reason to pass as &&. Doesn’t improve clarity or efficiency. Oops, I was wrong. std::function is not move-only. Please disregard my comment. > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:51 > + successCallback(JSC::JSValue()); This use of a JSValue() is extremely unpleasant; this is not how you are supposed to use JSValue. Can we come up with a cleaner way of doing this?

(In reply to comment #23) > This use of a JSValue() is extremely unpleasant; this is not how you are > supposed to use JSValue. Can we come up with a cleaner way of doing this? Could you please be more specific?

Comment on attachment 253790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253790&action=review >> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:51 >> + successCallback(JSC::JSValue()); > > This use of a JSValue() is extremely unpleasant; this is not how you are supposed to use JSValue. Can we come up with a cleaner way of doing this? The fact that a special null JSValue() exists that is distinct from the JavaScript “null” is an implementation detail that we won‘t keep like this forever. We should avoid this with one of these: 1) A separate callback for the case without a value. 2) A separate boolean that indicates the special case of no value. 3) Optional<JSValue> instead of using the special JSValue null that is different from JavaScript null.

(In reply to comment #25) > Comment on attachment 253790 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253790&action=review > > >> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:51 > >> + successCallback(JSC::JSValue()); > > > > This use of a JSValue() is extremely unpleasant; this is not how you are supposed to use JSValue. Can we come up with a cleaner way of doing this? > > The fact that a special null JSValue() exists that is distinct from the > JavaScript “null” is an implementation detail that we won‘t keep like this > forever. We should avoid this with one of these: > > 1) A separate callback for the case without a value. > 2) A separate boolean that indicates the special case of no value. > 3) Optional<JSValue> instead of using the special JSValue null that is > different from JavaScript null. In a previous version of the patch, 2 was implemented but it looked not that good: when the boolean was true, the JSValue was disregarded. 1 and 2 are not consistent with the one value callback that could be used for default binding code. I am not sure of 3. Imho, the best thing would be to stop using callbacks and directly call specific DeferredWrapper routines, dedicated to iterator and end-iterator in that case. This would simplify life of native stream sources that know about strings, array buffers or other type values but nothing about JSValue. DeferredWrapper would nicely handle the conversions. Currently DeferredWrapper is only used in binding code, hence my hesitation to follow that path... Note this discussion is related to bug 145223.

(In reply to comment #25) > Comment on attachment 253790 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253790&action=review > > >> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:51 > >> + successCallback(JSC::JSValue()); > > > > This use of a JSValue() is extremely unpleasant; this is not how you are supposed to use JSValue. Can we come up with a cleaner way of doing this? > > The fact that a special null JSValue() exists that is distinct from the > JavaScript “null” is an implementation detail that we won‘t keep like this > forever. We should avoid this with one of these: > > 1) A separate callback for the case without a value. > 2) A separate boolean that indicates the special case of no value. > 3) Optional<JSValue> instead of using the special JSValue null that is > different from JavaScript null. I will prepare a patch with option 1, including the additional improvements described in messages above.

Created attachment 254060 [details] Option 1, adding Vector::append method and removing unnecessary callback copies

Comment on attachment 254060 [details] Option 1, adding Vector::append method and removing unnecessary callback copies Clearing flags on attachment: 254060 Committed r185114: <http://trac.webkit.org/changeset/185114>

All reviewed patches have been landed. Closing bug.