Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2915973002:
Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders (Closed)
|
Created:
3 years, 6 months ago by Ryan Hamilton Modified:
3 years, 6 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders
resulting in the connection being closed due to a synchronous write error.
BUG=728434
Review-Url: https://codereview.chromium.org/2915973002
Cr-Commit-Position: refs/heads/master@{#476491}
Committed: https://chromium.googlesource.com/chromium/src/+/75548aac0ccababca0640928d2d5dc28afe00bed
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 2
Patch Set 3 : Rebase #
Total comments: 2
Patch Set 4 : Async notification #
Total comments: 5
Patch Set 5 : More comments #
Dependent Patchsets: Messages
Total messages: 24 (15 generated)
https://codereview.chromium.org/2915973002/diff/20001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2915973002/diff/20001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl.cc:108: std::move(headers), request_info_->end_stream_on_headers, nullptr); If this fails synchronously, then the stream will end up calling OnError on the bidi stream which will call NotifyError synchronously. That makes me nervous, because NotifyError() is called via PostTask up at the start of this method. But maybe it's fine to call into the delegate reentrantly? https://codereview.chromium.org/2915973002/diff/20001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (left): https://codereview.chromium.org/2915973002/diff/20001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:167: not_expect_callback_ = true; This scares me, because it sure seems like the class and the tests are going to pains to avoid calling reentrantly into the delegate. We discussed this issue in chat, and you're confident that such reentrancy is not problematic so I'm calling NotifyError directly in WriteHeaders(). But I could easily post a task instead which would be safer but possibly paranoid?
rch@chromium.org changed reviewers: + xunjieli@chromium.org
The CQ bit was checked by rch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rch@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...
The CQ bit was unchecked by rch@chromium.org
Thank you very much for the fix! LGTM https://codereview.chromium.org/2915973002/diff/40001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (left): https://codereview.chromium.org/2915973002/diff/40001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:186: not_expect_callback_ = false; Should we put back these guard checks?
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2915973002/diff/40001/net/quic/chromium/bidir... File net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc (left): https://codereview.chromium.org/2915973002/diff/40001/net/quic/chromium/bidir... net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc:186: not_expect_callback_ = false; On 2017/06/01 14:22:43, xunjieli (Slow.until.6-5) wrote: > Should we put back these guard checks? Done. Of course, putting the back meant that I needed to transform synchronous write errors into asynchronous delegate notification. I think this ended up looking reasonable, but it it's a non-trivial change to the CL (and changes a few test expectations), so PTAL. https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... net/quic/chromium/bidirectional_stream_quic_impl.cc:106: // Sending the request might result in the stream being closed. Since this CL no longer notifies the delegate synchronously on write errors (well, on any errors) we no longer need to use a WeakPtr here to detect |this| being deleted. We do, however, need to handle |stream_| being deleted so we don't dereference it later. It would be possible to do this detection at the various call sites if you prefer, though I think it's cleaner for this method to signal failure with the return value.
https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... net/quic/chromium/bidirectional_stream_quic_impl.cc:106: // Sending the request might result in the stream being closed. On 2017/06/01 17:10:14, Ryan Hamilton wrote: > Since this CL no longer notifies the delegate synchronously on write errors > (well, on any errors) we no longer need to use a WeakPtr here to detect |this| > being deleted. We do, however, need to handle |stream_| being deleted so we > don't dereference it later. > > It would be possible to do this detection at the various call sites if you > prefer, though I think it's cleaner for this method to signal failure with the > return value. Acknowledged. If the upcalls (OnClose and OnError) are removed (https://codereview.chromium.org/2908243002/), WriteHeaders() should only return true, right? If so, should we add a TODO here? https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... net/quic/chromium/bidirectional_stream_quic_impl.cc:110: return false; Could you add a comment here that NotifyError() will be called asynchronously through OnClose()? The caller of WriteHeader() doesn't handle the error when WriteHeader() returns false here. I think it's worth a comment.
https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... File net/quic/chromium/bidirectional_stream_quic_impl.cc (right): https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... net/quic/chromium/bidirectional_stream_quic_impl.cc:106: // Sending the request might result in the stream being closed. On 2017/06/01 21:24:02, xunjieli (Slow.until.6-5) wrote: > On 2017/06/01 17:10:14, Ryan Hamilton wrote: > > Since this CL no longer notifies the delegate synchronously on write errors > > (well, on any errors) we no longer need to use a WeakPtr here to detect |this| > > being deleted. We do, however, need to handle |stream_| being deleted so we > > don't dereference it later. > > > > It would be possible to do this detection at the various call sites if you > > prefer, though I think it's cleaner for this method to signal failure with the > > return value. > > Acknowledged. If the upcalls (OnClose and OnError) are removed > (https://codereview.chromium.org/2908243002/), WriteHeaders() should only return > true, right? > If so, should we add a TODO here? Yeah, either a bool, or an int. TODO added. https://codereview.chromium.org/2915973002/diff/140001/net/quic/chromium/bidi... net/quic/chromium/bidirectional_stream_quic_impl.cc:110: return false; On 2017/06/01 21:24:02, xunjieli (Slow.until.6-5) wrote: > Could you add a comment here that NotifyError() will be called asynchronously > through OnClose()? > > The caller of WriteHeader() doesn't handle the error when WriteHeader() returns > false here. I think it's worth a comment. In both places that this method returns false, a task will have been posted to notify the delegate. I've added a comment to the method declaration.
lgtm
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496353554866430,
"parent_rev": "dcf9a3a88e372fbb99a69df9689fd1d9463c1e81", "commit_rev":
"75548aac0ccababca0640928d2d5dc28afe00bed"}
Message was sent while issue was closed.
Description was changed from ========== Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders resulting in the connection being closed due to a synchronous write error. BUG=728434 ========== to ========== Fix potential crash bug with BidirectionalStreamQuicImpl::SendRequestHeaders resulting in the connection being closed due to a synchronous write error. BUG=728434 Review-Url: https://codereview.chromium.org/2915973002 Cr-Commit-Position: refs/heads/master@{#476491} Committed: https://chromium.googlesource.com/chromium/src/+/75548aac0ccababca0640928d2d5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/75548aac0ccababca0640928d2d5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
