New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 689762 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 736063
issue 738232



Sign in to add a comment

Make chrome support multiple versions of QUIC

Project Member Reported by zhongyi@chromium.org, Feb 8 2017

Issue description

Chrome currently supports a single version of QUIC to all the servers while it has code for different versions. We should make Chrome to speak multiple versions. When server also supports multiple versions, Chrome should pick the best version shared. 
 
Cc: alyssar@chromium.org
Project Member

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

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

commit c4de0303f3d7699d558f8042f946a2825e2d248a
Author: zhongyi <zhongyi@chromium.org>
Date: Fri May 19 04:07:34 2017

Change GetAlternativeServies to return alternative service infos.
This will be later used to pass QUIC versions from HttpServerProperties
to QuicStreamFactory in order to support multiple versions of QUIC.

BUG= 689762 

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

[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties.h
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_impl.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_impl.h
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_impl_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_manager.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_manager.h
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_server_properties_manager_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/c4de0303f3d7699d558f8042f946a2825e2d248a/net/spdy/chromium/spdy_session_unittest.cc

Project Member

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

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

commit 422ce3567ed22a4611f0ab5caccbc0cf123040df
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Jun 09 23:28:54 2017

Change AlternativeServiceInfo to a class

This is in preparation for adding a private field: advertised_versions
to AlternativeServiceInfo.

BUG= 689762 

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

[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties.h
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties_impl.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties_impl_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties_manager.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_server_properties_manager_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/422ce3567ed22a4611f0ab5caccbc0cf123040df/net/spdy/chromium/spdy_session_unittest.cc

Blockedon: 736063
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 27 2017

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

commit e537a0070404ceaae5d8ad6b7659a7cd24469996
Author: zhongyi <zhongyi@chromium.org>
Date: Tue Jun 27 16:48:21 2017

Add a new field in AlternativeServiceInfo to list QUIC verisons that are advertised by the server and supported by Chrome.
The list will also be persisted to disk.

BUG= 689762 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/components/cronet/ios/cronet_environment.mm
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/components/grpc_support/test/get_stream_engine.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties.h
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_impl.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_impl.h
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_impl_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_manager.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_manager.h
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_server_properties_manager_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_stream_factory.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/e537a0070404ceaae5d8ad6b7659a7cd24469996/net/spdy/chromium/spdy_session.cc

Blockedon: 738232
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 30 2017

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

commit 86838d5ee0c79c95644607bc0c29a7dcc54482b3
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Jun 30 01:19:44 2017

Fix QuicNetworkTransaction*Tests to set QuicAlternativeService with
current supported versions in the test suite.

Currently we ignore QUIC versions when GetAlternativeService. However,
in order to use the AlternativeService properly, a correct list of
versions in AlternativeServiceInfo should be set to HttpServerProperties
which contains the QUIC version that the running test is supporting.

This is in preparation to fix the tests that would blow up once we take
QUIC versions into account when selecting AlternativeService.

BUG= 689762 

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

[modify] https://crrev.com/86838d5ee0c79c95644607bc0c29a7dcc54482b3/net/quic/chromium/quic_network_transaction_unittest.cc

Project Member

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

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

commit a00ca01914a13a1ff5e5fa5d24c746246dd89449
Author: zhongyi <zhongyi@chromium.org>
Date: Thu Jul 06 23:37:01 2017

Change QuicStreamRequest::Request() to take a preferred QuicVersion so that
the created QuicConnection will use the designated QuicVersion instead
of hardcoded QuicVersion in HttpNetworkSession::Param().quic_supported_versions.

BUG= 689762 

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

[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_network_session.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_server_properties.h
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_test_util.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/http/http_stream_factory_test_util.h
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/a00ca01914a13a1ff5e5fa5d24c746246dd89449/net/quic/chromium/quic_stream_factory_test.cc

Last CL (I assume) broke builder Linux ChromiumOS Tests (dbg):

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/28082

components_unittests
Run on OS: 'Ubuntu-14.04'
failures:
 NetworkSessionConfiguratorTest.SameQuicVersionsFromFieldTrialParams
 NetworkSessionConfiguratorTest.QuicVersionFromFieldTrialParams
 NetworkSessionConfiguratorTest.QuicVersion
 NetworkSessionConfiguratorTest.MultipleQuicVersionFromFieldTrialParams

I was also able to reproduce these failures on my local machine.  Will revert the CL since these tests fail consistently.


Note: Also broke ConfigureParamsFromFieldTrialsAndCommandLineTest.QuicVersionFromCommandLine in unit_tests

Comment 12 by mark@chromium.org, Jul 12 2017

glevin, why didn’t you revert it as you said you would?

This caused persistent failures on these builders (at least):

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29

First failing builds:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/61499
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/28082

Tests failing are

components_unittests NetworkSessionConfiguratorTest.SameQuicVersionsFromFieldTrialParams, NetworkSessionConfiguratorTest.QuicVersionFromFieldTrialParams, NetworkSessionConfiguratorTest.QuicVersion, and NetworkSessionConfiguratorTest.MultipleQuicVersionFromFieldTrialParams
unit_tests ConfigureParamsFromFieldTrialsAndCommandLineTest.QuicVersionFromCommandLine

I will revert 4721652f4038 since it hasn’t been done yet.
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 12 2017

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

commit fec56ab88bb4a91a93d630e3833ae45f11333d2f
Author: Mark Mentovai <mark@chromium.org>
Date: Wed Jul 12 14:06:52 2017

Revert "Change NetworkSessionConfigurator to process multiple versions from quic_version field"

This reverts commit 4721652f40386747294802407b82780518c1a7a8.

Reason for revert:  https://crbug.com/689762#c12 

Original change's description:
> Change NetworkSessionConfigurator to process multiple versions from quic_version field
> 
> Bug:  689762 
> Change-Id: Ic0de2dacd01158d85318794d05f8f763659d9228
> Reviewed-on: https://chromium-review.googlesource.com/565784
> Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
> Reviewed-by: Ryan Hamilton <rch@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#485678}

TBR=rch@chromium.org,zhongyi@chromium.org

Change-Id: If18efa529febe19aa543eb2b6947434daeed7374
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  689762 
Reviewed-on: https://chromium-review.googlesource.com/567283
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485940}
[modify] https://crrev.com/fec56ab88bb4a91a93d630e3833ae45f11333d2f/components/network_session_configurator/browser/network_session_configurator.cc
[modify] https://crrev.com/fec56ab88bb4a91a93d630e3833ae45f11333d2f/components/network_session_configurator/browser/network_session_configurator.h
[modify] https://crrev.com/fec56ab88bb4a91a93d630e3833ae45f11333d2f/components/network_session_configurator/browser/network_session_configurator_unittest.cc

Comment 14 by mark@chromium.org, Jul 12 2017

Revert https://chromium-review.googlesource.com/567283/ landed at fec56ab88bb4.
Re: Comment #12
mark@, sincerest apologies!  We tried to revert the change repeatedly and it kept failing:
https://chromium-review.googlesource.com/#/c/566584/

Thanks for taking care of it!
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 13 2017

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

commit 987bdafbde5149d9ff14c8b8dd34a6796e2a5ac3
Author: Zhongyi Shi <zhongyi@chromium.org>
Date: Thu Jul 13 17:27:34 2017

Reland after fix "Change NetworkSessionConfigurator to process multiple versions from quic_version field"

This is a reland of 4721652f40386747294802407b82780518c1a7a8 after fix the crashing issues.

Original change's description:
> Change NetworkSessionConfigurator to process multiple versions from quic_version field
> 
> Bug:  689762 
> Change-Id: Ic0de2dacd01158d85318794d05f8f763659d9228
> Reviewed-on: https://chromium-review.googlesource.com/565784
> Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
> Reviewed-by: Ryan Hamilton <rch@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#485678}

Bug:  689762 
Change-Id: I1f1c92458263d3a9f013b9e0c51154186caabd17
Reviewed-on: https://chromium-review.googlesource.com/568641
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486429}
[modify] https://crrev.com/987bdafbde5149d9ff14c8b8dd34a6796e2a5ac3/components/network_session_configurator/browser/network_session_configurator.cc
[modify] https://crrev.com/987bdafbde5149d9ff14c8b8dd34a6796e2a5ac3/components/network_session_configurator/browser/network_session_configurator.h
[modify] https://crrev.com/987bdafbde5149d9ff14c8b8dd34a6796e2a5ac3/components/network_session_configurator/browser/network_session_configurator_unittest.cc

Status: Fixed (was: Assigned)
Close this bug as the implementation work has been completed.

Sign in to add a comment