New issue
Advanced search Search tips

Issue 748249 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

QuicHttpStream returns QUIC_PROTOCOL_ERROR when reading a solo fin

Project Member Reported by rch@chromium.org, Jul 24 2017

Issue description

If QuicHttpStream::ReadResponseBody is called when there is a solo fin to be read, it will return ERR_QUIC_PROTOCOL_ERROR instead of OK.

This is likely the cause of: https://bugs.chromium.org/p/gerrit/issues/detail?id=6729
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 25 2017

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

commit 2ef0a9c3ed9aa629c0bb7428d4dfed904bfbb2cb
Author: Ryan Hamilton <rch@chromium.org>
Date: Tue Jul 25 03:18:29 2017

QuicHttpStream::ReadResponseBody() should return OK if a solo fin is read, not ERR_QUIC_PROTOCOL_ERROR.

BUG= 748249 

Change-Id: I08e68b5ddf366f0632c49a3ec993ce62e9c8b891
Reviewed-on: https://chromium-review.googlesource.com/583906
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489209}
[modify] https://crrev.com/2ef0a9c3ed9aa629c0bb7428d4dfed904bfbb2cb/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/2ef0a9c3ed9aa629c0bb7428d4dfed904bfbb2cb/net/quic/chromium/quic_http_stream_test.cc

Comment 2 by rch@chromium.org, Jul 26 2017

Labels: Merge-Request-61

Comment 3 by rch@chromium.org, Jul 27 2017

As per govind@'s request for more information:
* This has been on Canary for a couple days now and does not appear to cause any new problems.
* Other than the new regression test, the change is only a single line and as such is expected to be quite safe.
* It would be good to merge this because this bug is making it hard for chrome developers to use PolyGerrit (and quite possibly other users of other Google services though we don't yet have bug reports)
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 27 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

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

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca72ca802b99d0ac05bc85b5429c842d83aa7580

commit ca72ca802b99d0ac05bc85b5429c842d83aa7580
Author: Ryan Hamilton <rch@chromium.org>
Date: Thu Jul 27 21:44:32 2017

[m61 merge] QuicHttpStream::ReadResponseBody() should return OK if a solo fin is read, not ERR_QUIC_PROTOCOL_ERROR.

BUG= 748249 
TBR=rch@chromium.org

(cherry picked from commit 2ef0a9c3ed9aa629c0bb7428d4dfed904bfbb2cb)

Change-Id: I08e68b5ddf366f0632c49a3ec993ce62e9c8b891
Reviewed-on: https://chromium-review.googlesource.com/583906
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Ryan Hamilton <rch@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489209}
Reviewed-on: https://chromium-review.googlesource.com/590753
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#96}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/ca72ca802b99d0ac05bc85b5429c842d83aa7580/net/quic/chromium/quic_http_stream.cc
[modify] https://crrev.com/ca72ca802b99d0ac05bc85b5429c842d83aa7580/net/quic/chromium/quic_http_stream_test.cc

Comment 6 by rch@chromium.org, Jul 29 2017

Cc: rch@chromium.org
 Issue 733196  has been merged into this issue.

Comment 7 by xftroxgpx@gmail.com, Jul 29 2017

Thank your for fixing this! Much appreciated!
Cheers!

Comment 8 by xftroxgpx@gmail.com, Jul 29 2017

you* :)
Issue 751053 has been merged into this issue.

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

Status: Fixed (was: Started)

Sign in to add a comment