Bug 138968

Summary: [Fetch API] Consume HTTP data as a ReadableStream
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, adam, ap, buildbot, calvaris, commit-queue, darin, eric.carlson, jer.noble, rniwa, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155637    
Bug Blocks: 151937    
Attachments:
Description Flags
WIP
none
WIP
none
Patch
none
Archive of layout-test-results from ews101 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Updated according ReadableStream implementation in JSC
none
WIP
none
Rebasing
none
Rebased and more tests
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Adding conditional compilation and fixing tests
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-yosemite
none
Fixing stream-response.html
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Trying to fix response-stream-disturbed-4.html
none
Patch
none
After review none

Description youenn fablet 2014-11-21 09:13:43 PST
A web app should be able to consume HTTP response data as a stream as soon as possible.
Amongst others, MSE may benefit from such feature.
Comment 1 youenn fablet 2014-11-24 01:19:02 PST
Created attachment 242148 [details]
WIP
Comment 2 youenn fablet 2014-11-24 01:20:51 PST
Created attachment 242149 [details]
WIP
Comment 3 youenn fablet 2014-11-24 01:25:23 PST
(In reply to comment #2)
> Created attachment 242149 [details]
> WIP

This patch illustrates how ReadableStream (as implemented in latest WIP patch from bug 138967) can be instantiated from XHR response.
Comment 4 Xabier Rodríguez Calvar 2014-11-24 01:46:51 PST
This depends on bug 138967, as this patch can't live without the other.
Comment 5 youenn fablet 2014-12-10 07:01:23 PST
Created attachment 243013 [details]
Patch
Comment 6 youenn fablet 2014-12-10 07:02:03 PST
(In reply to comment #5)
> Created attachment 243013 [details]
> Patch

Patch updates XHR integration according the latest ReadableStream API.
Comment 7 Build Bot 2014-12-10 07:45:52 PST
Comment on attachment 243013 [details]
Patch

Attachment 243013 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5988914837848064

New failing tests:
http/tests/xmlhttprequest/streams/streams-read-api-closed.html
http/tests/xmlhttprequest/streams/streams-read-api.html
fast/xmlhttprequest/xmlhttprequest-responsetype-stream.html
http/tests/xmlhttprequest/streams/streams-read.html
http/tests/xmlhttprequest/streams/streams-read-api-cancelled.html
Comment 8 Build Bot 2014-12-10 07:45:56 PST
Created attachment 243020 [details]
Archive of layout-test-results from ews101 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 9 Build Bot 2014-12-10 08:19:31 PST
Comment on attachment 243013 [details]
Patch

Attachment 243013 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5859343190720512

New failing tests:
http/tests/xmlhttprequest/streams/streams-read-api-closed.html
http/tests/xmlhttprequest/streams/streams-read-api.html
fast/xmlhttprequest/xmlhttprequest-responsetype-stream.html
http/tests/xmlhttprequest/streams/streams-read.html
http/tests/xmlhttprequest/streams/streams-read-api-cancelled.html
Comment 10 Build Bot 2014-12-10 08:19:37 PST
Created attachment 243024 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 youenn fablet 2014-12-11 02:51:28 PST
Comment on attachment 243013 [details]
Patch

Was not meant to be r?
Comment 12 youenn fablet 2015-08-21 03:40:09 PDT
Created attachment 259598 [details]
Updated according ReadableStream implementation in JSC
Comment 13 youenn fablet 2015-08-21 07:35:27 PDT
(In reply to comment #12)
> Created attachment 259598 [details]
> Updated according ReadableStream implementation in JSC

This is a mock-up version of how native ReadableStream sources could be implemented, not a real patch but it gives an idea.

Basically, native sources would have an IDL that would implement the ReadableStream source API (start, pull and cancel).

Similarly to promises, a WebCore wrapper around controller is introduced to more easily enqueue and error the streams with non-JS objects.

Controller is kept as a cached attribute as this is not observable from JS scripts and it allows not using JSC::Strong.
Comment 14 youenn fablet 2016-03-21 11:51:28 PDT
Created attachment 274614 [details]
WIP
Comment 15 youenn fablet 2016-03-24 09:13:23 PDT
Created attachment 274835 [details]
Rebasing
Comment 16 youenn fablet 2016-03-24 09:13:58 PDT
(In reply to comment #15)
> Created attachment 274835 [details]
> Rebasing

Patch probably needs more tests.
Early feedback most welcome though.
Comment 17 Alex Christensen 2016-03-24 10:54:47 PDT
Comment on attachment 274835 [details]
Rebasing

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

Just a quick glance:

> Source/WebCore/Modules/fetch/FetchBody.cpp:191


This is what a switch statement is for.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:52
> +void FetchBodyOwner::stopBlobLoading()

This change doesn't seem useful.

> Source/WebCore/Modules/fetch/FetchLoader.cpp:107
> +    RefPtr<SharedBuffer> data = WTFMove(m_data);

No need for a named variable here
Comment 18 WebKit Commit Bot 2016-03-24 10:58:18 PDT
Attachment 274835 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchResponse.h:122:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:43:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 youenn fablet 2016-04-05 02:05:44 PDT
Created attachment 275650 [details]
Rebased and more tests
Comment 20 WebKit Commit Bot 2016-04-05 02:10:06 PDT
Attachment 275650 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Build Bot 2016-04-05 03:01:05 PDT
Comment on attachment 275650 [details]
Rebased and more tests

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed.html
Comment 22 Build Bot 2016-04-05 03:01:09 PDT
Created attachment 275653 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 23 Build Bot 2016-04-05 03:16:08 PDT
Comment on attachment 275650 [details]
Rebased and more tests

Attachment 275650 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1102760

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed.html
Comment 24 Build Bot 2016-04-05 03:16:13 PDT
Created attachment 275654 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 25 Build Bot 2016-04-05 03:24:01 PDT
Comment on attachment 275650 [details]
Rebased and more tests

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html
imported/w3c/web-platform-tests/fetch/api/basic/stream-response.html
Comment 26 Build Bot 2016-04-05 03:24:06 PDT
Created attachment 275656 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 27 youenn fablet 2016-04-05 06:11:14 PDT
Created attachment 275657 [details]
Adding conditional compilation and fixing tests
Comment 28 WebKit Commit Bot 2016-04-05 06:13:58 PDT
Attachment 275657 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Build Bot 2016-04-05 07:06:00 PDT
Comment on attachment 275657 [details]
Adding conditional compilation and fixing tests

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html
Comment 30 Build Bot 2016-04-05 07:06:05 PDT
Created attachment 275661 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 31 Build Bot 2016-04-05 07:10:57 PDT
Comment on attachment 275657 [details]
Adding conditional compilation and fixing tests

Attachment 275657 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1103670

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html
Comment 32 Build Bot 2016-04-05 07:11:03 PDT
Created attachment 275664 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 33 Build Bot 2016-04-05 07:25:56 PDT
Comment on attachment 275657 [details]
Adding conditional compilation and fixing tests

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/stream-response.html
Comment 34 Build Bot 2016-04-05 07:26:01 PDT
Created attachment 275666 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 35 youenn fablet 2016-04-05 15:26:18 PDT
Created attachment 275703 [details]
Fixing stream-response.html
Comment 36 WebKit Commit Bot 2016-04-05 15:29:33 PDT
Attachment 275703 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Build Bot 2016-04-05 16:22:47 PDT
Comment on attachment 275703 [details]
Fixing stream-response.html

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html
Comment 38 Build Bot 2016-04-05 16:22:53 PDT
Created attachment 275712 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 39 Build Bot 2016-04-05 16:26:46 PDT
Comment on attachment 275703 [details]
Fixing stream-response.html

Attachment 275703 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1105867

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-4.html
Comment 40 Build Bot 2016-04-05 16:26:52 PDT
Created attachment 275713 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 41 youenn fablet 2016-04-06 02:40:38 PDT
Created attachment 275765 [details]
Trying to fix response-stream-disturbed-4.html
Comment 42 WebKit Commit Bot 2016-04-06 02:43:02 PDT
Attachment 275765 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchResponseSource.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Xabier Rodríguez Calvar 2016-04-06 02:55:08 PDT
Comment on attachment 275765 [details]
Trying to fix response-stream-disturbed-4.html

Considering that Fetch will need the Source and the ReadableStreamController, it might make more sense to split this patch into at least two, one for the RS and the other for Fetch code.

Said this, I also think there were some changes in the spec lately that would be interesting to have a look at, just in case we are doing things that changed already, etc.
Comment 44 youenn fablet 2016-04-06 06:19:54 PDT
Created attachment 275772 [details]
Patch
Comment 45 WebKit Commit Bot 2016-04-06 06:21:59 PDT
Attachment 275772 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Eric Carlson 2016-04-06 08:47:04 PDT
Comment on attachment 275772 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        A createReadableStream function is introduced to allow DOM classes createa ReadableStream.

Nit: typo "createa"

> Source/WebCore/Modules/fetch/FetchBody.cpp:159
> +        source.enqueue(ArrayBuffer::tryCreate(data.data(), data.size()));

Should this have a FIXME like that in BodyLoader::didReceiveData about what to do if ArrayBuffer::tryCreate returns null?

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:209
> +    m_readableStreamSource->enqueue(ArrayBuffer::tryCreate(data, size));

Ditto.
Comment 47 youenn fablet 2016-04-08 01:26:21 PDT
Comment on attachment 275772 [details]
Patch

Thanks for the feedback.

>> Source/WebCore/Modules/fetch/FetchBody.cpp:159
>> +        source.enqueue(ArrayBuffer::tryCreate(data.data(), data.size()));
> 
> Should this have a FIXME like that in BodyLoader::didReceiveData about what to do if ArrayBuffer::tryCreate returns null?

Yes, in that case, the stream will get errored and calling stream.close() afterwards will raise an exception.
Either we should check it here or handle this at FetchResponseSource::close level.

I will add a FIXME.
The annoying thing is that I do not know how to write a test for this code path.

>> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:209
>> +    m_readableStreamSource->enqueue(ArrayBuffer::tryCreate(data, size));
> 
> Ditto.

Right, we should add the same FIXME as for FetchResponse::BodyLoader::didReceiveData.
Comment 48 Alex Christensen 2016-04-08 13:58:33 PDT
Comment on attachment 275772 [details]
Patch

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

The code looks good and it makes many tests pass, which is good.  I can't say I can wrap my head completely around the design of these objects, though.

> Source/WebCore/ChangeLog:8
> +        This patch introduces ReadableStreamSource and ReadableStreamController which allow feeding a ReadableStream from DOM classes.

I'm not sure about this design.  It might be great, but a quick check shows Chromium has no such classes.  Could you describe why you went with this approach?

> Source/WebCore/Modules/fetch/FetchBody.cpp:164
> +    case Type::Blob:
> +        owner.loadBlob(*m_blob, FetchLoader::Type::Stream);

At least assert m_blob

> Source/WebCore/bindings/js/ReadableStreamController.cpp:70
> +    JSC::ExecState& state = *globalObject()->globalExec();

This is a lot of unchecked pointer dereferencing.
Comment 49 youenn fablet 2016-04-10 06:07:10 PDT
ReadableStream needs a JS source object with some of start, pull and cancel methods.
Parameter passed to start and pull is a controller which has enqueue/error/close methods to control the stream state.

The controller (JSReadableStreamController) is a JS built-in object. To help calling enqueue/close/error JS methods from dom classes, it is convenient to add a wrapper. This is the purpose of ReadableStreamController. It can also be used to query some stream internals (locked, disturbed).

To implement the source, using IDL seems like the easiest choice.
Currently ReadableStreamSource can handle any type of sources. Since fetch is push type, we could make it less general and probably more simple: no pull method, cancel returning void (sync cancel). In that case, ReadableStreamPushSource could be introduced instead.

For fetch, the start promise is never resolved. This disables any call to pull. This also allows to enqueue/close/error at anytime. FetchResponse is marked as pending activity when stream is started until the stream gets closed, errored or canceled.

I am not sure about chromium, I haven't checked its status lately. At some point streams were Dom/c++ based and thus directly tied to fetch. I think there were discussions to go to JS built-in but cannot say more than that.
Comment 50 youenn fablet 2016-04-17 09:37:44 PDT
Created attachment 276592 [details]
After review
Comment 51 WebKit Commit Bot 2016-04-17 09:40:44 PDT
Attachment 276592 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStreamController.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 youenn fablet 2016-04-17 09:41:33 PDT
(In reply to comment #50)
> Created attachment 276592 [details]
> After review

After thinking more about it, I removed the pull method from ReadableStreamSource since it is never called by FetchResponse. I also made cancel returning void since it is cancelled synchronously.
Comment 53 WebKit Commit Bot 2016-04-17 11:04:07 PDT
Comment on attachment 276592 [details]
After review

Clearing flags on attachment: 276592

Committed r199641: <http://trac.webkit.org/changeset/199641>
Comment 54 WebKit Commit Bot 2016-04-17 11:04:16 PDT
All reviewed patches have been landed.  Closing bug.