Concurrent reads from ReadableStreamDefaultReader crashes.
Reported by
mbost...@gmail.com,
Sep 5 2016
|
||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.89 Safari/537.36 Steps to reproduce the problem: 1. Initiate a fetch. 2. Wait for the response. 3. Get the response.body reader. 4. Call reader.read. 5. Call reader.read again without waiting for the prior read to finish. What is the expected behavior? I believe the expected behavior is that the reader should yield the same result to all concurrent reads, per these issues: https://github.com/whatwg/streams/issues/5 https://github.com/whatwg/streams/issues/15 Whatever the intended behavior is, I’m pretty sure it’s not to crash the tab. :) What went wrong? Aw, Snap! Something went wrong while displaying this webpage. Did this work before? No Chrome version: 53.0.2785.89 Channel: stable OS Version: OS X 10.11.6 Flash Version: Shockwave Flash 22.0 r0 I’ve attached a small example that reproduces the crash.
,
Sep 6 2016
Thank you for reporting. I can reproduce the crash.
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d commit 561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d Author: yhirano <yhirano@chromium.org> Date: Tue Sep 06 20:51:27 2016 [Fetch API] Call endRead sooner to avoid recursive invocation Currently FetchDataConsumerHandle::Reader::endRead is called after ReadableStreamController::enqueue that can trigger a pull operation. This CL removes such a recursion by calling endRead before enqueue. BUG= 644117 Review-Url: https://codereview.chromium.org/2311063003 Cr-Commit-Position: refs/heads/master@{#416723} [modify] https://crrev.com/561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js [modify] https://crrev.com/561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
,
Sep 7 2016
,
Sep 7 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a356d611880d6825ed8961523fc2c8c54bcc3a30 commit a356d611880d6825ed8961523fc2c8c54bcc3a30 Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Sep 08 04:00:05 2016 [Fetch API] Call endRead sooner to avoid recursive invocation Currently FetchDataConsumerHandle::Reader::endRead is called after ReadableStreamController::enqueue that can trigger a pull operation. This CL removes such a recursion by calling endRead before enqueue. BUG= 644117 Review-Url: https://codereview.chromium.org/2311063003 Cr-Commit-Position: refs/heads/master@{#416723} (cherry picked from commit 561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d) Review URL: https://codereview.chromium.org/2325543002 . Cr-Commit-Position: refs/branch-heads/2840@{#227} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a356d611880d6825ed8961523fc2c8c54bcc3a30/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js [modify] https://crrev.com/a356d611880d6825ed8961523fc2c8c54bcc3a30/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
,
Sep 8 2016
,
Sep 14 2016
@mbostock: Could you please have a look at the attached video and let us know if this is the expected behavior. No crash was observed. Tested it on MAC (10.11.6) for Google Chrome Beta Version - 54.0.2840.27 Thank you.
,
Sep 14 2016
@rnimmagadda That’s only the expected behavior if you’re running out of the local file system, where security restrictions disallow local fetching. You need to run it on a web server. I’ve copied the example to GitHub Gist here: http://bl.ocks.org/mbostock/468df5e2c4f52147869353ae3ea54807 The expected behavior is that it logs to result objects to the console; each object should have a value and a done property. I don’t understand the streams spec well enough to say whether both results should have the same value, or whether the first result should represent the first part of the stream and the second result the second part of the stream. Safari Technology Preview (Release 12, Safari 9.1.2, WebKit 11603.1.3) chooses the latter interpretation; I’ve attached a screenshot.
,
Sep 14 2016
My interpretation is the latter one, too. Are you suggesting that you get the same result twice? Have you examined the contents of Uint8Arrays?
,
Sep 14 2016
I am not an expert on the proposed streams specification so I can’t say what the correct behavior should be regarding the contents of the two result values. But if you are asking for my personal opinion, having the first result contain the first chunk and the second contain the second chunk (despite the read requests happening concurrently) sounds perfectly reasonable.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a356d611880d6825ed8961523fc2c8c54bcc3a30 commit a356d611880d6825ed8961523fc2c8c54bcc3a30 Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Sep 08 04:00:05 2016 [Fetch API] Call endRead sooner to avoid recursive invocation Currently FetchDataConsumerHandle::Reader::endRead is called after ReadableStreamController::enqueue that can trigger a pull operation. This CL removes such a recursion by calling endRead before enqueue. BUG= 644117 Review-Url: https://codereview.chromium.org/2311063003 Cr-Commit-Position: refs/heads/master@{#416723} (cherry picked from commit 561ffb5dd08c8f4af6d33f1caf0ce66ba38f397d) Review URL: https://codereview.chromium.org/2325543002 . Cr-Commit-Position: refs/branch-heads/2840@{#227} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a356d611880d6825ed8961523fc2c8c54bcc3a30/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js [modify] https://crrev.com/a356d611880d6825ed8961523fc2c8c54bcc3a30/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by tkent@chromium.org
, Sep 5 2016