BidirectionalStream::OnHttpsProxyTunnelResponse() needs test coverage |
||||||||
Issue descriptionProblem ======= HttpProxyClientSocket::CreateConnectResponseStream() creates a ProxyConnectRedirectHttpStream (derived class of HttpStream) instance using new, and gives up ownership, as reflected by its return value HttpStream*. HttpStreamFactoryImpl::Job::RunLoop() takes this raw HttpStream pointer and passes it to HttpStreamFactoryImpl::Job::OnHttpsProxyTunnelResponseCallback() (asynchronously, via PostTask(Bind())). Passing a raw pointer means passing ownership. HttpStreamFactoryImpl::Job::OnHttpsProxyTunnelResponseCallback() passes it to delegate_->OnHttpsProxyTunnelResponse(), again, passing ownership. HttpStreamFactoryImpl::JobController implements HttpStreamFactoryImpl::Job::Delegate. HttpStreamFactoryImpl::JobController::OnHttpsProxyTunnelResponse() passes ownership when calling request_->OnHttpsProxyTunnelResponse(). HttpStreamFactoryImpl::Request::OnHttpsProxyTunnelResponse() passes ownership when calling delegate_->OnHttpsProxyTunnelResponse(). One HttpStreamRequest::Delegate::OnHttpsProxyTunnelResponse() implementation, HttpNetworkTransaction::OnHttpsProxyTunnelResponse(), correctly takes ownership when resetting HttpNetworkTransaction::stream_, a unique_ptr<HttpStream>, to |stream|. However, another HttpStreamRequest::Delegate::OnHttpsProxyTunnelResponse() implementation, BidirectionalStream::OnHttpsProxyTunnelResponse(), drops |stream| on the floor. The HttpStream instance leaks. Proposed solution ================= Change HttpProxyClientSocket::CreateConnectResponseStream() return value to unique_ptr<HttpStream>. This will automagically ensure that the instance is destroyed after the BidirectionalStream::OnHttpsProxyTunnelResponse() call finishes. Since solving this issue would conflict with https://crrev.com/2814633003 (in progress), I'll wait for that CL to land before starting to work on this.
,
May 15 2017
Uh-oh, BidirectionalStream::OnHttpsProxyTunnelResponse() is not exercised by any unittests! If it was, maybe MSAN would have caught this. Shall I sweat to write a unittest or shall we just add DCHECK or CHECK or DumpWithoutCrashing to BidirectionalStream::OnHttpsProxyTunnelResponse()?
,
May 15 2017
Someone really should write a unit test, though who should do it, or whether they should sweat in doing so, is left as an exercise to the reader.
,
May 15 2017
,
May 16 2017
I agree that a unittest would be desirable if BidirectionalStream::OnHttpsProxyTunnelResponse() is a method that can be called under some circumstances. However, it would be possible that this method was never called, in which case it might be better to document it with a NOTREACHED() or similar. However, xunjieli@ believes that it is possible to use BidirectionalStream in a manner that is allowed by its API and receive bytes from the wire in such a configuration that BidirectionalStream::OnHttpsProxyTunnelResponse() is called, in which case, a unit test should indeed cover this case. I have not figured out quite yet how to do this though.
,
May 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26 commit 6d05ebdc419a34aa72afdf1a6577b7f7c945ed26 Author: Bence Béky <bnc@chromium.org> Date: Tue May 16 13:19:00 2017 Return unique_ptr from CreateConnectResponseStream(). Return unique_ptr<HttpStream> instead of raw HttpStream pointer from ProxyClientSocket::CreateConnectResponseStream() and its overrides. Pass unique_ptr<HttpStream> instead of raw HttpStream pointer to: * HttpStreamFactoryImpl::Job::OnHttpsProxyTunnelResponseCallback() * HttpStreamFactoryImpl::Job::Delegate::OnHttpsProxyTunnelResponse() * HttpStreamFactoryImpl::Request::OnHttpsProxyTunnelResponse() * HttpStreamRequest::Delegate::OnHttpsProxyTunnelResponse() and their overrides. This not only solves the problem of BidirectionalStream::OnHttpsProxyTunnelResponse() silenty leaking an HttpStream instance, but also makes it more difficult to do the wrong thing in the future. Also add some includes, one explicit keyword, and remove one semicolon, to make git cl lint happy. BUG= 722361 Change-Id: Ie01023c8e077fe691df9daaa2ad6158befd1dd44 Reviewed-on: https://chromium-review.googlesource.com/505394 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Bence Béky <bnc@chromium.org> Cr-Commit-Position: refs/heads/master@{#472076} [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/bidirectional_stream.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/bidirectional_stream.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_network_transaction.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_network_transaction.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_proxy_client_socket.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_proxy_client_socket.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_proxy_client_socket_wrapper.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_proxy_client_socket_wrapper.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_job.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_job.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_job_controller.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_job_controller.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_request.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_request.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_impl_unittest.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/http_stream_factory_test_util.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/http/proxy_client_socket.h [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/spdy/chromium/spdy_proxy_client_socket.cc [modify] https://crrev.com/6d05ebdc419a34aa72afdf1a6577b7f7c945ed26/net/spdy/chromium/spdy_proxy_client_socket.h
,
May 18 2017
Leaving this bug open because of the need for a unittest.
,
Apr 18 2018
This seems beyond the scope of second pass triage.
,
Apr 19 2018
Changing issue summary to reflect remaining action item (see comments #2 and #5 for more details).
,
Jul 6
,
Jul 6
,
Jan 3
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by xunji...@chromium.org
, May 15 2017