[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded callbacks on iOS.

[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect object to be deleted upon return.

BUG=650782
Committed: https://crrev.com/210d6b6d067016d9535dd6ff2b149a07e33851bf
Cr-Commit-Position: refs/heads/master@{#422224}

[mef, 2016-09-27 19:42:07]

mef@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[kapishnikov, 2016-09-27 21:29:31]

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:251: if (stream_done_)
Is it possible for this method to be called when another completion method has
already been called? I think there is a bug in the underlying implementation
then.

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:262: if (stream_done_)
Should it be an error when StartOnNetworkThread is called when the stream is
done?

[mef, 2016-09-28 00:44:29]

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:251: if (stream_done_)
On 2016/09/27 21:29:30, kapishnikov wrote:
> Is it possible for this method to be called when another completion method has
> already been called? I think there is a bug in the underlying implementation
> then.

Yes, for example racing 'cancel' and network error, but I need to think a little
more about proper handling of such situation.

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:262: if (stream_done_)
On 2016/09/27 21:29:30, kapishnikov wrote:
> Should it be an error when StartOnNetworkThread is called when the stream is
> done?

Done.

[mef, 2016-09-28 22:29:53]

PTAL. Deleting |bidi_stream_| asynchronously simplifies things, but I think
there is still possibility for racing between asynchronous deletion and network
error. WDYT?

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/2371053003/diff/20001/components/cronet/ios/c...
components/cronet/ios/cronet_bidirectional_stream.cc:251: if (stream_done_)
On 2016/09/28 00:44:28, mef wrote:
> On 2016/09/27 21:29:30, kapishnikov wrote:
> > Is it possible for this method to be called when another completion method
has
> > already been called? I think there is a bug in the underlying implementation
> > then.
> 
> Yes, for example racing 'cancel' and network error, but I need to think a
little
> more about proper handling of such situation.

So, on Android BidirectionalStream is deleted asynchronously, posted from user
executor thread onto network thread. I've tried do similar thing here, posting
deletion of |bidi_stream_| onto network thread, but it is still somewhat racy.

[mef, 2016-09-30 17:36:16]

Description was changed from

==========
[Cronet] Don't delete net::BidirectionalStream in OnFailed callback on iOS.

BUG=650782
==========

to

==========
[Cronet] Delete net::BidirectionalStream in OnFailed and MaybeOnSucceeded
callback on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

[kapishnikov, 2016-09-30 17:43:57]

https://codereview.chromium.org/2371053003/diff/80001/components/cronet/ios/c...
File components/cronet/ios/cronet_c_for_grpc.cc (right):

https://codereview.chromium.org/2371053003/diff/80001/components/cronet/ios/c...
components/cronet/ios/cronet_c_for_grpc.cc:192: adapter->stream_ = nullptr;
We set adapter->stream_ to null here on the client thread. Could it happen that
at the same time the network thread executes one of the
CronetBidirectionalStreamAdapter callbacks, e.g. OnDataRead() that reads this
variable. It may happen that the callback is executed with stream_ == null. This
would probably lead to a crash.

[mef, 2016-09-30 19:20:43]

PTAL

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (left):

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:569:
TEST_P(CronetBidirectionalStreamTest, ReadFailsBeforeRequestStarted) {
No callbacks before request started.

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:618:
TEST_P(CronetBidirectionalStreamTest, WriteFailsBeforeRequestStarted) {
No callbacks before request started.

[mef, 2016-09-30 19:30:52]

PTAL.

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (left):

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:569:
TEST_P(CronetBidirectionalStreamTest, ReadFailsBeforeRequestStarted) {
On 2016/09/30 19:20:43, mef wrote:
> No callbacks before request started.

Done.

https://codereview.chromium.org/2371053003/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:618:
TEST_P(CronetBidirectionalStreamTest, WriteFailsBeforeRequestStarted) {
On 2016/09/30 19:20:43, mef wrote:
> No callbacks before request started.

Done.

[mef, 2016-09-30 19:48:10]

Description was changed from

==========
[Cronet] Delete net::BidirectionalStream in OnFailed and MaybeOnSucceeded
callback on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

to

==========
[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded
callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

[kapishnikov, 2016-09-30 20:40:10]

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.cc:282: if (!bidi_stream_ ||
read_state_ != WAITING_FOR_READ) {
Is this change necessary? Is it possible that bidi_stream_ == NULL &&
read_state_ == WAITING_FOR_READ?

It shouldn't since the read state is changed in CancelOnNetworkThread().

If it is still possible, should we log a different error message when
bidi_stream_ == NULL?

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.cc:370: void
CronetBidirectionalStream::DestroyOnNetworkThread() {
Can it happen that DestroyOnNetworkThread is called but the underlying
bidi-stream is still doing something? I don't see anything that would prevent
the bidi-stream to call its delegate.

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
File components/cronet/ios/cronet_c_for_grpc.cc (right):

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.cc:135:
DCHECK(c_callback_->on_response_headers_received);
Should be DCHECK(c_callback_->on_stream_ready)

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.cc:224: int
cronet_bidirectional_stream_destroy(cronet_bidirectional_stream* stream) {
We should document that this method should not be called until one of the
terminal callbacks is called.

[mef, 2016-09-30 21:05:09]

Thanks, PTAL.

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
File components/cronet/ios/cronet_bidirectional_stream.cc (right):

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.cc:282: if (!bidi_stream_ ||
read_state_ != WAITING_FOR_READ) {
On 2016/09/30 20:40:10, kapishnikov wrote:
> Is this change necessary? Is it possible that bidi_stream_ == NULL &&
> read_state_ == WAITING_FOR_READ?
> 
> It shouldn't since the read state is changed in CancelOnNetworkThread().
> 
> If it is still possible, should we log a different error message when
> bidi_stream_ == NULL?

Done.

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_bidirectional_stream.cc:370: void
CronetBidirectionalStream::DestroyOnNetworkThread() {
On 2016/09/30 20:40:10, kapishnikov wrote:
> Can it happen that DestroyOnNetworkThread is called but the underlying
> bidi-stream is still doing something? I don't see anything that would prevent
> the bidi-stream to call its delegate.

It will delete bidi_stream_ via unique_ptr.

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
File components/cronet/ios/cronet_c_for_grpc.cc (right):

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.cc:135:
DCHECK(c_callback_->on_response_headers_received);
On 2016/09/30 20:40:10, kapishnikov wrote:
> Should be DCHECK(c_callback_->on_stream_ready)

Done.

https://codereview.chromium.org/2371053003/diff/140001/components/cronet/ios/...
components/cronet/ios/cronet_c_for_grpc.cc:224: int
cronet_bidirectional_stream_destroy(cronet_bidirectional_stream* stream) {
On 2016/09/30 20:40:10, kapishnikov wrote:
> We should document that this method should not be called until one of the
> terminal callbacks is called.

Will address this in the followup cl.

[kapishnikov, 2016-09-30 21:11:59]

lgtm

[mef, 2016-09-30 21:13:36]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2016-09-30 21:14:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 21:25:16]

Description was changed from

==========
[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded
callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

to

==========
[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded
callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

[commit-bot: I haz the power, 2016-09-30 21:25:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-30 21:27:42]

Description was changed from

==========
[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded
callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782
==========

to

==========
[Cronet] Delete bidi stream asynchronously in OnFailed and MaybeOnSucceeded
callbacks on iOS.

Callbacks are invoked synchronously from underlying net layers, and don't expect
object to be deleted upon return.

BUG=650782

Committed: https://crrev.com/210d6b6d067016d9535dd6ff2b149a07e33851bf
Cr-Commit-Position: refs/heads/master@{#422224}
==========

[commit-bot: I haz the power, 2016-09-30 21:27:43]

Patchset 9 (id:??) landed as
https://crrev.com/210d6b6d067016d9535dd6ff2b149a07e33851bf
Cr-Commit-Position: refs/heads/master@{#422224}