New issue
Advanced search Search tips

Issue 853189 link

Starred by 2 users
Status: Assigned
Owner:
ricea@chromium.org
Cc:
yhirano@chromium.org
domenic@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Use ExceptionStatus& for methods in ReadableStreamOperations and their callers

Project Member Reported by ricea@chromium.org, Jun 15 2018 
https://chromium-review.googlesource.com/c/chromium/src/+/1097047 made methods in ReadableStreamOperations return base::nullopt when an exception is thrown. However, the wrapper methods in BodyStreamBuffer hide this state from their callers, so the callers will not be able to detect if an exception is pending and return. We want the exceptions to be visible to Javascript so that someone looking up response.bodyUsed with a full stack gets a RangeError exception rather than just getting a bogus return value.

Exception handling needs to be explicit for maintainability. So the methods in ReadableStreamOperations that return Optional<bool> and their callers and their callers' callers need to take an ExceptionState& argument.
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0598444f7467071501c71290248bf8758d501fde

commit 0598444f7467071501c71290248bf8758d501fde
Author: Adam Rice <ricea@chromium.org>
Date: Wed Jun 27 01:45:55 2018

Make IsDisturbed take an ExceptionState parameter

Modify blink::ReadableStreamOperations::IsDisturbed() to take an
ExceptionState& parameter. Modify all callers so that the ExceptionState
is passed through all the way from the bindings layer. Where
IsDisturbed() is called from code that runs asynchronously, the
context_type, interface_name and property_name are cached and used to
create a new ExceptionState object in the asynchronous context.

Also change methods in the implementation of the payment API to use the
cached context_type, interface_name and property_name instead of
hard-coding the values.

Add the extended attribute "RaisesException" to all the asynchronous
body-reading methods in body.idl so that they receive an ExceptionState&
parameter which is used to handle exceptions thrown when calling
IsDisturbed() inside RejectInvalidConsumption().
RejectInvalidConsumption() now always rejects by throwing an exception
and no longer returns a promise.

An exception-ignoring IsBodyUsedForDCheck() method is added which
doesn't change the exception state.

Also add the [RaisesException] attribute to the bodyUsed accessor. Since
it now throws if it has ever failed, fix the
call-extra-crash-is-disturbed.html layout test to reflect that.

Bug: 853189
Change-Id: Ib6df486a5f4ea7c58ecc123a716ee76ace0c92ed
Reviewed-on: https://chromium-review.googlesource.com/1107578
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Jinho Bang <jinho.bang@samsung.com>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570621}
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-is-disturbed.html
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/public/mojom/service_worker/service_worker_error_type.mojom
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body.idl
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/body_stream_buffer_test.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/response.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/fetch/response.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/cache_storage/cache.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/abort_payment_respond_with_observer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/can_make_payment_respond_with_observer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/payment_handler_utils.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/payments/payment_request_respond_with_observer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/serviceworkers/fetch_respond_with_observer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/serviceworkers/fetch_respond_with_observer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/serviceworkers/respond_with_observer.cc
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/third_party/blink/renderer/modules/serviceworkers/respond_with_observer.h
[modify] https://crrev.com/0598444f7467071501c71290248bf8758d501fde/tools/metrics/histograms/enums.xml
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac

commit 86baffc40b5c95f1eb0e5ade193c4b61fadf4aac
Author: Adam Rice <ricea@chromium.org>
Date: Wed Jun 27 03:30:58 2018

Make IsReadable() take an ExceptionState& argument

Modify blink::ReadableStreamOperations::IsReadable() to take an
ExceptionState& parameter. Modify all callers so that the ExceptionState
is passed through all the way from the bindings layer.

Bug: 853189
Change-Id: Icd29564d9dc869d3e91ca0fd981c0f75362f1581
Reviewed-on: https://chromium-review.googlesource.com/1114518
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570653}
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/body.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/body_stream_buffer_test.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/fetch_manager.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/fetch_manager.h
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/fetch_request_data.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/fetch_request_data.h
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/global_fetch.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/request.h
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/fetch/response_test.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/86baffc40b5c95f1eb0e5ade193c4b61fadf4aac/third_party/blink/renderer/modules/serviceworkers/fetch_respond_with_observer.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/beffe645c2e93de1f9090d73011080da9f0e06dd

commit beffe645c2e93de1f9090d73011080da9f0e06dd
Author: Adam Rice <ricea@chromium.org>
Date: Thu Jun 28 04:04:53 2018

Make IsLocked() take an ExceptionState& argument

Modify blink::ReadableStreamOperations::IsLocked() to take an
ExceptionState& parameter. Modify all callers so that the ExceptionState
is passed through all the way from the bindings layer.

Make Body::IsBodyLocked() return an enum { kLocked, kUnlocked, kBroken }
similar to IsBodyUsed.

Bug: 853189
Change-Id: I3b2350c845bfa57c0123c4426ef8c0b2e3eb6ddc
Reviewed-on: https://chromium-review.googlesource.com/1114938
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571025}
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/body.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/body.h
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/body_stream_buffer_test.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/fetch/response.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/modules/cache_storage/cache.cc
[modify] https://crrev.com/beffe645c2e93de1f9090d73011080da9f0e06dd/third_party/blink/renderer/modules/serviceworkers/fetch_respond_with_observer.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70a1658a8eec82c6d1e321a3ca09cc054b06792c

commit 70a1658a8eec82c6d1e321a3ca09cc054b06792c
Author: Adam Rice <ricea@chromium.org>
Date: Thu Jun 28 05:45:00 2018

Make IsClosed() and IsErrored() take an ExceptionState& argument

Modify IsClosed() and IsErrored() in blink::ReadableStreamOperations to
take an ExceptionState& parameters. Modify the wrappers in
BodyStreamBuffer to also take ExceptionState&, and make the callers
handle exceptions from them correctly.

Bug: 853189
Change-Id: I64a806571bd2c080de3ee2b5966cad6142ba0e15
Reviewed-on: https://chromium-review.googlesource.com/1116634
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571038}
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/fetch/body_stream_buffer_test.cc
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/70a1658a8eec82c6d1e321a3ca09cc054b06792c/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30be752da476e69e9e72b55e7eab08b02d06bb9a

commit 30be752da476e69e9e72b55e7eab08b02d06bb9a
Author: Adam Rice <ricea@chromium.org>
Date: Thu Jun 28 11:59:00 2018

Make IsReadableStreamDefaultReader() take an ExceptionState&

Also remove the unsafe version of IsReadableStream().

Remove BooleanOperation() helper from readable_stream_operations.cc as
it is no longer used.

Modify the tests to pass an ExceptionState& argument to
IsReadableStream() and IsReadableStreamDefaultReader().

Bug: 853189
Change-Id: I26393861760e3bdab6991e522565067aabae5fb4
Reviewed-on: https://chromium-review.googlesource.com/1118089
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571086}
[modify] https://crrev.com/30be752da476e69e9e72b55e7eab08b02d06bb9a/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/30be752da476e69e9e72b55e7eab08b02d06bb9a/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/30be752da476e69e9e72b55e7eab08b02d06bb9a/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc

Comment 6 by ricea@chromium.org, Jun 28 2018 

Issue 857432 ("ReadableStreamDefaultControllerWrapper uses ToLocalChecked") is related.
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 29 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/003ce9aead7292bb082c10fe6af5831d3a10634d

commit 003ce9aead7292bb082c10fe6af5831d3a10634d
Author: Adam Rice <ricea@chromium.org>
Date: Fri Jun 29 04:53:24 2018

Make ReadableStreamOperations::GetReader() take an ExceptionState

blink::ReadableStreamOperations::GetReader() should throw an exception
when the operation failed. Take an ExceptionState& parameter and rethrow
the exception on failure.

BodyStreamBuffer::CloseAndLockAndDisturb used the exception from
GetReader() to detect when the stream was already locked. Since it
there's no straightforward way to distinguish between the expected
exception and some other exception, instead check IsLocked() beforehand
to eliminate the possibility of an expected exception from GetReader().

Bug: 853189
Change-Id: Ib7bdf1cda9d392e4434ceff352c8f10ddf8ff951
Reviewed-on: https://chromium-review.googlesource.com/1118205
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571392}
[modify] https://crrev.com/003ce9aead7292bb082c10fe6af5831d3a10634d/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/003ce9aead7292bb082c10fe6af5831d3a10634d/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer_test.cc
[modify] https://crrev.com/003ce9aead7292bb082c10fe6af5831d3a10634d/third_party/blink/renderer/core/streams/readable_stream_operations.cc
[modify] https://crrev.com/003ce9aead7292bb082c10fe6af5831d3a10634d/third_party/blink/renderer/core/streams/readable_stream_operations.h
[modify] https://crrev.com/003ce9aead7292bb082c10fe6af5831d3a10634d/third_party/blink/renderer/core/streams/readable_stream_operations_test.cc

Comment 8 by ricea@chromium.org, Jun 29 2018

Cc: yhirano@chromium.org
That's all of them done except for CreateReadableStream and CreateCountQueuingStrategy. Those two are only called by the variant of the BodyStreamBuffer constructor that doesn't take an existing ReadableStream as an argument.

That constructor currently catches the exception and sets stream_broken_. It would be better to expose the exception to JavaScript, but that constructor is called in many places that don't have an ExceptionState object available, including on network responses.

Rather than try to create an ExceptionState in all those places, it might be better to delay the construction of the ReadableStream to the first time it is accessed from JavaScript.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5aa10741b61a7e6fee932b221d9a8e657dd77601

commit 5aa10741b61a7e6fee932b221d9a8e657dd77601
Author: Adam Rice <ricea@chromium.org>
Date: Wed Jul 11 09:04:28 2018

Test Response body after constructor failure

Constructing a Response from a non-stream will currently succeed even if
creating the ReadableStream for the body failed. In this case response.body will
be undefined. Add a test to verify that it is undefined, and touching it does
not crash the renderer.

Bug: 853189

Change-Id: I7fcf5ebc8b347c859b40b3d403d40bc048504211
Reviewed-on: https://chromium-review.googlesource.com/1132833
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574111}
[add] https://crrev.com/5aa10741b61a7e6fee932b221d9a8e657dd77601/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/call-extra-crash-construct-and-look.html
►

Sign in to add a comment