Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163883004:
Do not invoke QuicHttpStream::DoCallback when in DoLoop (Closed)
|
Created:
4 years, 5 months ago by xunjieli Modified:
4 years, 5 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not invoke QuicHttpStream::DoCallback when in DoLoop
Latest crash dumps suggest that callback is invoked
re-entrantly during QuicHttpStream::DoLoop(). This can be
problematic because the caller can delete the
QuicHttpStream when the callback is run. This CL changes
QuicHttpStream::OnClose() to not invoke callback when
already in DoLoop().
This CL also adds a unit test which will crash without the
change or the CHECKs.
BUG=629043
Committed: https://crrev.com/8dff50bad755b439be74e98225647699f555ee96
Cr-Commit-Position: refs/heads/master@{#407152}
Patch Set 1 #Patch Set 2 : Add a test which crashes with the exact call stack without the patch #
Total comments: 2
Messages
Total messages: 25 (15 generated)
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org, rch@chromium.org
What do you think about this approach instead? I am not sure how to fix the APIs, but this might be a short-term fix.
On 2016/07/20 15:10:07, xunjieli wrote: > What do you think about this approach instead? I am not sure how to fix the > APIs, but this might be a short-term fix. I'll defer to rch on this - the state machine is enough of a mess that I don't want to dig through it and figure out if this is correct. There should be a test for this.
On 2016/07/20 15:14:43, mmenke wrote: > On 2016/07/20 15:10:07, xunjieli wrote: > > What do you think about this approach instead? I am not sure how to fix the > > APIs, but this might be a short-term fix. > > I'll defer to rch on this - the state machine is enough of a mess that I don't > want to dig through it and figure out if this is correct. > > There should be a test for this. SGTM. I just want to make sure this is okay. I will work on a test now.
On 2016/07/20 15:20:23, xunjieli wrote: > On 2016/07/20 15:14:43, mmenke wrote: > > On 2016/07/20 15:10:07, xunjieli wrote: > > > What do you think about this approach instead? I am not sure how to fix the > > > APIs, but this might be a short-term fix. > > > > I'll defer to rch on this - the state machine is enough of a mess that I don't > > want to dig through it and figure out if this is correct. > > > > There should be a test for this. > > SGTM. I just want to make sure this is okay. I will work on a test now. Even if rch disagrees with this approach, any test you write should still be usable.
Patchset #1 (id:1) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done. I added a test which will crash with the same callstack without applying the change. Ryan: PTAL.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Description was changed from ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback might be invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). BUG=629043 ========== to ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback might be invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). BUG=629043 ==========
xunjieli@chromium.org changed reviewers: - mmenke@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback might be invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). BUG=629043 ========== to ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ==========
Description was changed from ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ========== to ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ==========
Since this fixes a crash, I'm happy to see it land now but I think this needs a bit more thought. Do you have a pointer to the crash logs? lgtm https://codereview.chromium.org/2163883004/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2163883004/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:516: } Do you know what state in DoLoop we're in when this happens?
> Since this fixes a crash, I'm happy to see it land now but I think this needs a > bit more thought. Do you have a pointer to the crash logs? SGTM. I think the long-term fix should be in the API. The linked bug (crbug.com/629043) has links to a few sample crashes logs. https://codereview.chromium.org/2163883004/diff/40001/net/quic/quic_http_stre... File net/quic/quic_http_stream.cc (right): https://codereview.chromium.org/2163883004/diff/40001/net/quic/quic_http_stre... net/quic/quic_http_stream.cc:516: } On 2016/07/21 22:29:33, Ryan Hamilton wrote: > Do you know what state in DoLoop we're in when this happens? According to the crash log on crbug/629043, we are in DoSendBody().
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ========== to ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 ========== to ========== Do not invoke QuicHttpStream::DoCallback when in DoLoop Latest crash dumps suggest that callback is invoked re-entrantly during QuicHttpStream::DoLoop(). This can be problematic because the caller can delete the QuicHttpStream when the callback is run. This CL changes QuicHttpStream::OnClose() to not invoke callback when already in DoLoop(). This CL also adds a unit test which will crash without the change or the CHECKs. BUG=629043 Committed: https://crrev.com/8dff50bad755b439be74e98225647699f555ee96 Cr-Commit-Position: refs/heads/master@{#407152} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8dff50bad755b439be74e98225647699f555ee96 Cr-Commit-Position: refs/heads/master@{#407152} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
