Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1656673002:
QUIC - Fix for crash in stable channel. Check for null |visitor_| before (Closed)
|
Created:
4 years, 10 months ago by ramant (doing other things) Modified:
4 years, 10 months ago 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 - Fix for crash in stable channel. Check for null |visitor_| before
accessing it.
BUG=546668
R=rch@chromium.org, asvitkine@chromium.org
Committed: https://crrev.com/3a731f0d7d50c4ec479f71ca3076a09576b44def
Cr-Commit-Position: refs/heads/master@{#372752}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added histogram #
Total comments: 2
Patch Set 3 : Updated comment to delete histogram #
Total comments: 2
Depends on Patchset: Messages
Total messages: 25 (10 generated)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656673002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656673002/1
Hi Ryan, Would like to request to merge this check for nullptr to M48. Will revert this change from the tip after it is merged to fix the top QUIC crash bug. In windbg, for most of the crashes, |visitor_| seems to be nullptr. Crash could be due to someone corrupting the memory. Would like to see if this fixes the crash or not. PTAL. thanks raman
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1656673002/diff/1/net/quic/quic_connection.cc File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/1/net/quic/quic_connection.cc... net/quic/quic_connection.cc:2080: } Would it be worth adding an: } else { UMA_HISTOGRAM("Something bad happened, so tell UMA"); }
Description was changed from ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org ========== to ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org, asvitkine@chromium.org ==========
rtenneti@chromium.org changed reviewers: + asvitkine@chromium.org
Thanks Ryan for the suggestion. Added a histogram to track it. https://codereview.chromium.org/1656673002/diff/1/net/quic/quic_connection.cc File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/1/net/quic/quic_connection.cc... net/quic/quic_connection.cc:2080: } On 2016/02/01 03:39:31, Ryan Hamilton wrote: > Would it be worth adding an: > > } else { > UMA_HISTOGRAM("Something bad happened, so tell UMA"); > } Good idea. Done.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656673002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656673002/20001
lgtm (normally I'd suggest for a boolean histogram to have both true and false logged, but if this is temporary just to see the extent of the effect, then I'm ok with it) https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... net/quic/quic_connection.cc:2078: // |visitor_| to fix crash bug. Delete it after fix is merged. Planning to delete the histogram at that point too? If so, mention it here/
Updated the comment to delete histogram also. Will revert this CL after it is merged to M48. thanks raman https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... net/quic/quic_connection.cc:2078: // |visitor_| to fix crash bug. Delete it after fix is merged. On 2016/02/01 19:03:14, Alexei Svitkine (very slow) wrote: > Planning to delete the histogram at that point too? If so, mention it here/ Done.
The CQ bit was checked by rtenneti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1656673002/#ps40001 (title: "Updated comment to delete histogram")
On 2016/02/01 19:03:14, Alexei Svitkine (very slow) wrote: > lgtm > > (normally I'd suggest for a boolean histogram to have both true and false > logged, but if this is temporary just to see the extent of the effect, then I'm > ok with it) Correct. > > https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... > File net/quic/quic_connection.cc (right): > > https://codereview.chromium.org/1656673002/diff/20001/net/quic/quic_connectio... > net/quic/quic_connection.cc:2078: // |visitor_| to fix crash bug. Delete it > after fix is merged. > Planning to delete the histogram at that point too? If so, mention it here/
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656673002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656673002/40001
Message was sent while issue was closed.
Description was changed from ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org, asvitkine@chromium.org ========== to ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org, asvitkine@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org, asvitkine@chromium.org ========== to ========== QUIC - Fix for crash in stable channel. Check for null |visitor_| before accessing it. BUG=546668 R=rch@chromium.org, asvitkine@chromium.org Committed: https://crrev.com/3a731f0d7d50c4ec479f71ca3076a09576b44def Cr-Commit-Position: refs/heads/master@{#372752} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3a731f0d7d50c4ec479f71ca3076a09576b44def Cr-Commit-Position: refs/heads/master@{#372752}
Message was sent while issue was closed.
glider@chromium.org changed reviewers: + glider@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1656673002/diff/40001/net/quic/quic_connectio... File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/40001/net/quic/quic_connectio... net/quic/quic_connection.cc:2080: if (visitor_) { Drive-by nit: this check is inconsistent with other nullptr checks in the file.
Message was sent while issue was closed.
https://codereview.chromium.org/1656673002/diff/40001/net/quic/quic_connectio... File net/quic/quic_connection.cc (right): https://codereview.chromium.org/1656673002/diff/40001/net/quic/quic_connectio... net/quic/quic_connection.cc:2080: if (visitor_) { On 2016/02/07 08:40:18, Alexander Potapenko wrote: > Drive-by nit: this check is inconsistent with other nullptr checks in the file. Done. Uploaded the following CL. Thanks. https://codereview.chromium.org/1679113002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
