New issue
Advanced search Search tips

Issue 922739 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Populate granular quic connection error code when a connection is closed during connection migration.

Project Member Reported by zhongyi@chromium.org, Jan 16 (6 days ago)

Issue description

When https://chromium-review.googlesource.com/c/chromium/src/+/1016009 is landed, we expect granular quic error code will be populated for reporting when net error is ERR_NETWORK_CHANGED.

However, it looks like the error code is not correctly generated, as data collected by cronet apps indicates this field is always "QUIC_NO_ERROR".
 

Comment 1 by zhongyi@chromium.org, Jan 16 (6 days ago)

The root cause is described as follows:

When a QuicConnection is closed, the granular quic connection error code leading to the connection close will be populated. 
And when QuicSession::OnConnectionClosed is invoked, the error code will be passed to QuicSession. QuicSession will pass that down to QuicChromiumClientSession::Handle when QuicChromiumClientSession::CloseAllHandles. And when the upper layer (URLRequest) decides to fetch the error codes, it will eventually fetch the handle's cached error code. 

This works most of the time, however, if a connection is closed by session via QuicChromiumClientSession::CloseSessionOnError, the code path is a little bit different. Unfortunately we closed connection after closing all the handles. By the time handles are closed, the connection is still alive and there's no connection error. 

void QuicChromiumClientSession::CloseSessionOnError(
    int net_error,
    quic::QuicErrorCode quic_error,
    quic::ConnectionCloseBehavior behavior) {
  base::UmaHistogramSparse("Net.QuicSession.CloseSessionOnError", -net_error);

  if (!callback_.is_null()) {
    base::ResetAndReturn(&callback_).Run(net_error);
  }
  CloseAllStreams(net_error);
  CloseAllHandles(net_error);      <-------------------------- disconnect session with handle. 
  net_log_.AddEvent(NetLogEventType::QUIC_SESSION_CLOSE_ON_ERROR,
                    NetLog::IntCallback("net_error", net_error));

  if (connection()->connected())
    connection()->CloseConnection(quic_error, "net error", behavior);  <--------------------- generate quic error code.
  DCHECK(!connection()->connected());

  NotifyFactoryOfSessionClosed();
}

Comment 2 by zhongyi@chromium.org, Jan 17 (6 days ago)

Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 17 (6 days ago)

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

commit 59aaf078eaa908431e93a31c871f8d93e7821218
Author: Zhongyi Shi <zhongyi@chromium.org>
Date: Thu Jan 17 03:32:13 2019

Close all handles after connection close when session decides to close connection on error.

This change also conditionally override quic connection error
only when the QuicChromiumClientStream contains a valid connection error
code.

Bug:  922739 
Change-Id: Ib83d65cd5c03f89842a344fa599d3f874107c5b7
Reviewed-on: https://chromium-review.googlesource.com/c/1415304
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623555}
[modify] https://crrev.com/59aaf078eaa908431e93a31c871f8d93e7821218/net/quic/quic_chromium_client_session.cc
[modify] https://crrev.com/59aaf078eaa908431e93a31c871f8d93e7821218/net/quic/quic_http_stream.cc
[modify] https://crrev.com/59aaf078eaa908431e93a31c871f8d93e7821218/net/quic/quic_stream_factory_test.cc

Comment 4 by zhongyi@chromium.org, Jan 17 (5 days ago)

Status: Fixed (was: Started)

Sign in to add a comment