New issue
Advanced search Search tips

Issue 716563 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Eliminate re-entrancy in QUIC code between the core and chromium layer

Project Member Reported by rch@chromium.org, Apr 28 2017

Issue description

Reentrancy is extremely hard to reason about, and leads to all kinds of terrible behavior. Alas, the current core QUIC code make extensive use of synchronous up-calls (via Vistors and Observers). We should eliminate this. Any time the chromium layer needs to call into the core QUIC layer, this should happen with chrome-style methods that take a CompletionCallback and return a net-error. If they fail synchronously, the chromium layer checks the net-error. Otherwise, the chromium layer awaits the callback, which is guaranteed to not be called reentrantly. 
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 29 2017

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

commit 94c26d6895a4e7db8b276d79189cab14dee4cc6a
Author: rch <rch@chromium.org>
Date: Sat Apr 29 02:49:27 2017

Cancel pending stream requests as soon as a QUIC session is closed
instead of waiting for it be be destroyed.

Add tests for the cases where a connection is close before a StreamRequest
completes (both sync and async).

BUG= 716563 

Review-Url: https://codereview.chromium.org/2854443003
Cr-Commit-Position: refs/heads/master@{#468216}

[modify] https://crrev.com/94c26d6895a4e7db8b276d79189cab14dee4cc6a/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/94c26d6895a4e7db8b276d79189cab14dee4cc6a/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/94c26d6895a4e7db8b276d79189cab14dee4cc6a/net/quic/chromium/quic_chromium_client_session_test.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 2 2017

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

commit d606b6c39dbce82e048ed0a737d56dc7d8b0ea32
Author: rch <rch@chromium.org>
Date: Tue May 02 17:32:57 2017

Move the "wait for QUIC handshake confirmation" logic to QuicChromiumClientSession::StreamRequest and out of the individual stream subclasses.

This will allow a subsequent CL to remove the OnHandshakeConfirmed upcall.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2848923004
Cr-Commit-Position: refs/heads/master@{#468699}

[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/d606b6c39dbce82e048ed0a737d56dc7d8b0ea32/net/quic/chromium/quic_http_stream.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 2 2017

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

commit 5bc140b118c615e92e0393d8baf846d1a9bcbdba
Author: rch <rch@chromium.org>
Date: Tue May 02 18:43:01 2017

Change the way BidirectionalStreamQuicImpl detects that a stream/session
was closed abnormally.

Currently BidirectionalStreamQuicImpl::OnClose() does not invoke
NotifyError if the stream and connection error codes are both
NO_ERROR. However, this is not guaranteeded to be true when a stream
shuts down normally. For example, if the connection is closed with
QUIC_NO_ERROR (which is not expected behavior, but is possible) then
NotifyError will not be invoked. Now, it turns out that the current code
handles this case, perhaps inadvertantly, by invoking NotifyError
in OnSessionClosed.

As part of  crbug.com/716563 , I'm attempting to remove the upcalls from
the QUIC layer to the HTTP layer, so I will be removing OnSessionClosed.
In order to handle the NO_ERROR case, we need a better way to detect
that a stream was closed normally. Thankfully, we can do that by
checking that a fin was sent and a fin was received.

This change also clears session_ in ResetStream which ensures that
OnSessionClosed is not invoked after OnClose is called (and makes it
more obvious that the follow-up CL to remove OnSessionClosed is safe).

BUG= 716563 

Review-Url: https://codereview.chromium.org/2851293002
Cr-Commit-Position: refs/heads/master@{#468714}

[modify] https://crrev.com/5bc140b118c615e92e0393d8baf846d1a9bcbdba/net/quic/chromium/bidirectional_stream_quic_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 5 2017

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

commit f0b18c8a8efc9b2dd335a0b29bd243b0711919a6
Author: rch <rch@chromium.org>
Date: Fri May 05 19:31:57 2017

The Handle is owned by the holder of the Handle and can outlive the session. In the event that it does outlive the session, it will record state from the session in member variables so that the holder of a session need not be aware of the lifetime of the underlying session.

As a consequence, this change eliminates the need for the QuicChromiumClientSession::Observer interface, and hence eliminates the upcalls from the session to the streams. There are still upcalls from the QuicStream to the QuicHttpStream (and friends) but those can be removed in subsequent CLs by following the same pattern.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2334943002
Cr-Commit-Position: refs/heads/master@{#469756}

[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_http_stream.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_http_stream_test.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/spdy/chromium/multiplexed_http_stream.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/spdy/chromium/multiplexed_http_stream.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/spdy/chromium/multiplexed_session.cc
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/spdy/chromium/multiplexed_session.h
[modify] https://crrev.com/f0b18c8a8efc9b2dd335a0b29bd243b0711919a6/net/spdy/chromium/spdy_http_stream.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 8 2017

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

commit bd73b32db882801efd8e45642d2136cdccc70945
Author: rch <rch@chromium.org>
Date: Mon May 08 21:26:18 2017

Fix QuicChromiumClientStreamTest.HeadersBeforeDelegate to actually set
the delegate after the headers arrive. Also add a test for headers and
data arriving before the delegate.

I verified that the old test was passing "by accident" by adding a
LOG(FATAL) in SetDelegate() where it runs the buffered tasks which
was not hit when running this test.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2870483002
Cr-Commit-Position: refs/heads/master@{#470134}

[modify] https://crrev.com/bd73b32db882801efd8e45642d2136cdccc70945/net/quic/chromium/quic_chromium_client_stream_test.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 9 2017

Project Member

Comment 9 by bugdroid1@chromium.org, May 10 2017

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

commit 43535f1beb4135a9f860fdae5b99ec0a5e4e9374
Author: rch <rch@chromium.org>
Date: Wed May 10 23:50:40 2017

Minor cleanup of QuicChromiumClientStreamTest
* Remove an unused MOCK_METHOD
* Create a "real" stream instead of a "test" stream.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2880483002
Cr-Commit-Position: refs/heads/master@{#470755}

[modify] https://crrev.com/43535f1beb4135a9f860fdae5b99ec0a5e4e9374/net/quic/chromium/quic_chromium_client_stream_test.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 21 2017

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

commit fb47f71aa9f2ad207139f5bb03269eba31a0be67
Author: rch <rch@chromium.org>
Date: Sun May 21 03:24:00 2017

Add an async ReadInitialHeaders method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors. That will happen after all the upcalls are replaced by async
methods.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2873963003
Cr-Commit-Position: refs/heads/master@{#473458}

[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_chromium_client_stream.cc
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_http_stream.h
[modify] https://crrev.com/fb47f71aa9f2ad207139f5bb03269eba31a0be67/net/quic/chromium/quic_http_stream_test.cc

Project Member

Comment 11 by bugdroid1@chromium.org, May 24 2017

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

commit 97827ee523ac3f3f1d3b8b7c84b0c1fc13fc651b
Author: rch <rch@chromium.org>
Date: Wed May 24 23:49:12 2017

Change QuicHttpStream to Reset the upload data stream even if the
underlying QUIC stream is null.

Fix some tests in which the UploadDataStream did not outlive the QuicHttpStream.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2899403002
Cr-Commit-Position: refs/heads/master@{#474479}

[modify] https://crrev.com/97827ee523ac3f3f1d3b8b7c84b0c1fc13fc651b/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/97827ee523ac3f3f1d3b8b7c84b0c1fc13fc651b/net/quic/chromium/quic_http_stream_test.cc

Project Member

Comment 12 by bugdroid1@chromium.org, May 26 2017

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

commit 27da045892e94846096a6549a178b34ee4d8f9d3
Author: rch <rch@chromium.org>
Date: Fri May 26 22:54:54 2017

Add an async ReadBody method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors. That will happen after all the upcalls are replaced by async
methods.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2877063002
Cr-Commit-Position: refs/heads/master@{#475172}

[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_chromium_client_stream.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_http_stream.h
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/net/quic/chromium/quic_http_stream_test.cc
[modify] https://crrev.com/27da045892e94846096a6549a178b34ee4d8f9d3/tools/metrics/histograms/histograms.xml

Project Member

Comment 13 by bugdroid1@chromium.org, May 28 2017

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

commit 2ab4d878e867d169a3cae4c1f006955ce0439357
Author: rch <rch@chromium.org>
Date: Sun May 28 15:15:40 2017

Move the async write handling from QuicChromiumClientStream to the Handle
in order to put all the async logic in the same place.

Does not actually change semantics of the Handle's write methods,
just changes the implementation.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2880643004
Cr-Commit-Position: refs/heads/master@{#475261}

[modify] https://crrev.com/2ab4d878e867d169a3cae4c1f006955ce0439357/net/quic/chromium/quic_chromium_client_stream.cc
[modify] https://crrev.com/2ab4d878e867d169a3cae4c1f006955ce0439357/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/2ab4d878e867d169a3cae4c1f006955ce0439357/net/quic/chromium/quic_chromium_client_stream_test.cc

Project Member

Comment 14 by bugdroid1@chromium.org, May 30 2017

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

commit ce2464110221db485c26fd738c097c95c2269c13
Author: rch <rch@chromium.org>
Date: Tue May 30 13:51:50 2017

Add an async ReadTrailers method to QuicChromiumClientStream::Handle

This does not change OnClose or OnError and the callback is not yet invoked
on errors, but that can happen in the next CL since all upcalls have now
been replaced by async methods.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2900533002
Cr-Commit-Position: refs/heads/master@{#475517}

[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_chromium_client_stream.cc
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_http_stream.h
[modify] https://crrev.com/ce2464110221db485c26fd738c097c95c2269c13/net/quic/chromium/quic_http_stream_test.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 1 2017

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

commit 4f176c786029e5b60a5c579efc752d3586a6c594
Author: rch <rch@chromium.org>
Date: Thu Jun 01 04:53:19 2017

Change BidirectionalStreamQuicImplTest to verify the correct callback
is invoked instead of relying on comments.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2914853002
Cr-Commit-Position: refs/heads/master@{#476193}

[modify] https://crrev.com/4f176c786029e5b60a5c579efc752d3586a6c594/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Jun 3 2017

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

commit 1bcfddf2df553b180d0253f26527f94d38710b0e
Author: rch <rch@chromium.org>
Date: Sat Jun 03 00:26:29 2017

Remove QuicChromiumClientStream::Delegate in favor of async methods.

This removes OnClose and OnError from QuicHttpStream and
BidirectionalStreamQuicImpl.

BUG= 716563 

Review-Url: https://codereview.chromium.org/2908243002
Cr-Commit-Position: refs/heads/master@{#476842}

[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_stream.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_stream.h
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_http_stream.h
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_http_stream_test.cc
[modify] https://crrev.com/1bcfddf2df553b180d0253f26527f94d38710b0e/net/quic/chromium/quic_network_transaction_unittest.cc

Issue 592652 has been merged into this issue.

Comment 18 by rch@chromium.org, Nov 10 2017

Status: Fixed (was: Started)

Sign in to add a comment