HttpStreamFactory and QuicStreamFactory can disagree about QUIC brokeness |
|||||||||
Issue descriptionIf an origin is configured to use a remote alt-svc and a local alt-svc, but the local alt-svc is broken, HttpStreamFactory will attempt to create a QUIC session to the origin via the remote alt-svc. However, the QuicStreamFactory will report that QUIC is broken for that session because it will look up the local alt svc. This is a problem because the request will be failed with QUIC_PROTOCOL_ERROR and may then be retried, resulting in a new QUIC session created, lather-rinse-repeat.
,
Nov 27 2017
,
Nov 27 2017
This bug requires manual review: We are only 7 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 27 2017
rch@ can you provide the rationale as to why this fix should be taken into M63 at this point? We are about to go to stable next week and only critical fixes should be merged. What is the risk to take this fix in M63? How bad will the users be affected if we don't take it in M63?
,
Nov 27 2017
We discovered recently that this bug results in end-user visible page load failures under certain conditions (which is why we didn't realize it earlier). These failures can be avoided by this change to not honor remote-alt-svc entries. FWIW, it's a very low-risk change, though I totally appreciate the desire to be cautious here.
,
Nov 27 2017
In case something unexpected happens with this change, will we be able to disable this feature from the server?
,
Nov 27 2017
Not sure how I managed to add iOS in my previous comment. QUIC is not enabled on iOS 'cause our net stack is not used on iOS. Removing iOS.
,
Nov 27 2017
By server-side do you mean via finch? Yes, we can disable QUIC via finch.
,
Nov 27 2017
Yes I meant finch. Thanks very much! I will approved the merge now. Please make sure to verify the fix on branch 3239 after merging it.
,
Nov 28 2017
Pls merge your change to M63 branch 3239 before 12:30 PM, Tuesday (11/28/17) so we can take it in for this week last Beta release. Thank you.
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5f8b29b059d512aea089215ae08e2f1358a0168a commit 5f8b29b059d512aea089215ae08e2f1358a0168a Author: Ryan Hamilton <rch@chromium.org> Date: Tue Nov 28 18:02:27 2017 [m63 merge] Add a new HttpNetworkSession param quic_allow_remote_alt_svc to determine if we honor QUIC remote alt-svcs. TBR=rch@chromium.org (cherry picked from commit c84473f329399b3e437235cf01e8d7fc8355c150) Bug: 787949 Change-Id: I2f91b6b08a61f098e9f18fee9d986df968e00d27 Reviewed-on: https://chromium-review.googlesource.com/786329 Reviewed-by: Jana Iyengar <jri@chromium.org> Commit-Queue: Ryan Hamilton <rch@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#518838} Reviewed-on: https://chromium-review.googlesource.com/791825 Reviewed-by: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#582} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/5f8b29b059d512aea089215ae08e2f1358a0168a/net/http/http_network_session.cc [modify] https://crrev.com/5f8b29b059d512aea089215ae08e2f1358a0168a/net/http/http_network_session.h [modify] https://crrev.com/5f8b29b059d512aea089215ae08e2f1358a0168a/net/http/http_stream_factory_impl_job_controller.cc [modify] https://crrev.com/5f8b29b059d512aea089215ae08e2f1358a0168a/net/quic/chromium/quic_network_transaction_unittest.cc
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e commit 6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e Author: Ryan Hamilton <rch@chromium.org> Date: Fri Dec 15 02:06:28 2017 Change QuicHttpStream::GetAlternativeService to return the actual alternative service used by this request, not simply the server ID of the QUIC session. These can be different if a remote alt-svc is used, or if session pooling is used. Bug: 787949 Change-Id: I47cdb34cfdd8a06b2b74505a4ff055dac7448459 Reviewed-on: https://chromium-review.googlesource.com/825646 Commit-Queue: Ryan Hamilton <rch@chromium.org> Reviewed-by: Buck Krasic <ckrasic@chromium.org> Cr-Commit-Position: refs/heads/master@{#524286} [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_chromium_client_session.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_chromium_client_session.h [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_chromium_client_session_test.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_http_stream.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_http_stream_test.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_network_transaction_unittest.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_proxy_client_socket_unittest.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_stream_factory.cc [modify] https://crrev.com/6c2a2a8f1bfb8576ffc0ba428db7b4a061b7ed9e/net/quic/chromium/quic_stream_factory.h
,
Jan 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d32636d121706cbee7cfe1d950635b3a38df43d commit 0d32636d121706cbee7cfe1d950635b3a38df43d Author: Ryan Hamilton <rch@chromium.org> Date: Tue Jan 30 06:05:10 2018 Re-enable remote QUIC alt-svc Bug: 787949 Change-Id: I19ff9ce7f046eb177403d5e9350833e1932b98a1 Reviewed-on: https://chromium-review.googlesource.com/884421 Reviewed-by: Buck Krasic <ckrasic@chromium.org> Commit-Queue: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/heads/master@{#532790} [modify] https://crrev.com/0d32636d121706cbee7cfe1d950635b3a38df43d/net/http/http_network_session.cc
,
Jul 24
Looks like this bug is fixed?
,
Jul 31
Agreed! |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 23 2017