Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044923003:
QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. (Closed)
|
Created:
4 years, 6 months ago by ramant (doing other things) Modified:
4 years, 6 months ago Reviewers:
mef CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr.
R=mef@chromium.org
Committed: https://crrev.com/10c34f79c450f00cc12ee21cb69bba154dfeca53
Cr-Commit-Position: refs/heads/master@{#398392}
Patch Set 1 #
Total comments: 3
Depends on Patchset: Messages
Total messages: 17 (7 generated)
Description was changed from ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. QUIC - fix crash bug in cronet. R=mef@chromium.org ========== to ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org ==========
On 2016/06/07 19:54:10, ramant wrote: Hi Misha, This is a temporary fix and would like to see if this fixes the cronet crash issue. If it doesn't will rollback. thanks raman
lgtm
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044923003/1
Message was sent while issue was closed.
Description was changed from ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org ========== to ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org ========== to ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org Committed: https://crrev.com/10c34f79c450f00cc12ee21cb69bba154dfeca53 Cr-Commit-Position: refs/heads/master@{#398392} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/10c34f79c450f00cc12ee21cb69bba154dfeca53 Cr-Commit-Position: refs/heads/master@{#398392}
Message was sent while issue was closed.
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.cc File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.c... net/quic/quic_http_stream.cc:178: if (session_.get() == nullptr) { Could just do "if (!session_)". It's more consistent with the other null checks in this file.
Message was sent while issue was closed.
Description was changed from ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org Committed: https://crrev.com/10c34f79c450f00cc12ee21cb69bba154dfeca53 Cr-Commit-Position: refs/heads/master@{#398392} ========== to ========== QUIC/cronet - Fix crash bug b/28676259. Handle session_ being a nullptr. R=mef@chromium.org Committed: https://crrev.com/10c34f79c450f00cc12ee21cb69bba154dfeca53 Cr-Commit-Position: refs/heads/master@{#398392} ==========
Message was sent while issue was closed.
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.cc File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.c... net/quic/quic_http_stream.cc:178: if (session_.get() == nullptr) { On 2016/06/07 23:46:25, xunjieli wrote: > Could just do "if (!session_)". It's more consistent with the other null checks > in this file. Regarding WeakPtr, the following discussion could be interesting. https://mail.google.com/mail/u/0/#search/label%3Achromium-dev+weakptr/155189b... Wanted to see if there is a platform specific bug. Will change it to (!session_) and will add UMA histograms to track how many times we hit this condition.
Message was sent while issue was closed.
On 2016/06/07 23:58:33, ramant wrote: > https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.cc > File net/quic/quic_http_stream.cc (right): > > https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.c... > net/quic/quic_http_stream.cc:178: if (session_.get() == nullptr) { > On 2016/06/07 23:46:25, xunjieli wrote: > > Could just do "if (!session_)". It's more consistent with the other null > checks > > in this file. > > Regarding WeakPtr, the following discussion could be interesting. > > https://mail.google.com/mail/u/0/#search/label%3Achromium-dev+weakptr/155189b... > > Wanted to see if there is a platform specific bug. Will change it to (!session_) > and will add UMA histograms to track how many times we hit this condition. Good to know. Looks like the new recommendation prefers explicit conversion. It will be good to change other call sites in this file down the road then. https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Recomm...
Message was sent while issue was closed.
https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.cc File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2044923003/diff/1/net/quic/quic_http_stream.c... net/quic/quic_http_stream.cc:178: if (session_.get() == nullptr) { On 2016/06/07 23:58:32, ramant wrote: > On 2016/06/07 23:46:25, xunjieli wrote: > > Could just do "if (!session_)". It's more consistent with the other null > checks > > in this file. > > Regarding WeakPtr, the following discussion could be interesting. > > https://mail.google.com/mail/u/0/#search/label%3Achromium-dev+weakptr/155189b... > > Wanted to see if there is a platform specific bug. Will change it to (!session_) > and will add UMA histograms to track how many times we hit this condition. > Uploaded CL: https://codereview.chromium.org/2073323003/ to collect histogram data. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
