Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948143002:
Add an async method to QuicChromiumClientSession::Handle for (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. |
DescriptionAdd an async method to QuicChromiumClientSession::Handle for
rendezvousing with promised streams. This fixes a bug where
a session could be closed, and the push handle deleted
before the QuicHttpStream was notified resulting in
a use-after-free of the push handle.
BUG=719461
Review-Url: https://codereview.chromium.org/2948143002
Cr-Commit-Position: refs/heads/master@{#481886}
Committed: https://chromium.googlesource.com/chromium/src/+/56ec40a93b88fe42be155062fa681a36fbace4a1
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 34
Patch Set 3 : Address comments #
Total comments: 8
Patch Set 4 : Address more comments #
Total comments: 4
Patch Set 5 : Cleanup #Patch Set 6 : ASAN #
Messages
Total messages: 35 (18 generated)
rch@chromium.org changed reviewers: + xunjieli@chromium.org
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...
I think this looks good. I have a couple of nits and a few questions about how the test works. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:341: HttpResponseInfo promise_response_info; optional nit: Move this closer to where it's needed (after line 347). https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:366: void QuicChromiumClientSession::Handle::OnRendezvousResult( nit: these four new methods should in the same order as they are declared w.r.t. to other methods in the header file. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:384: QuicChromiumClientSession::Handle::ReleasePromisedStream() { nit: Add a DCHECK(push_stream_) to make sure that the caller is calling this correctly https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:92: // eventuallly completed |callback| will be called. nit: s/eventuallly/eventually https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:106: // Releases |stream_| to the caller. nit: s/stream_/push_stream_ https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:150: base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr())); Should this be "base::Unretained(this)" given that QuicHttpStream owns the handle? https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:157: if (rv < 0) { nit: This branch is not needed. We can go directly to STATE_HANDLE_PROMISE_COMPLETE, because DoHandlePromiseComplete() handles the error case. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:167: int QuicHttpStream::DoHandlePromiseComplete(int rv) { optional nit: add a DCHECK_NE(ERR_IO_PENDING, rv) DCHECK_LE(rv, OK) here just to make the expected range of values for |rv| clearer. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4304: // is closed before the pushed header arrive, but after the connection nit: s/arrive/arrives https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4324: GetResponseHeaders("200 OK"), &server_header_offset)); Sorry I am not familiar with how push promise works. Is this the response code for GET / or GET /pushed.jpg ? https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4327: 3, GetNthClientInitiatedStreamId(0), false, true, 0, "hello!")); Could you annotate these data packets? Is this the response body of GET / or GET /pushed.jpg ? https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4339: // Start a push transaction that will be cancelled after the conneciton nit: typo in connection. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4343: TestCompletionCallback callback; |callback| should still be notified, right? Can we check its result at the end of this test? https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4351: request_.url = GURL("https://mail.example.org/"); nit: do not reuse |request_| for |trans2|. The Transaction.Start() takes in the pointer address of |request_|. I am nervous about changing the HttpRequestInfo while it is used by another transaction. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4358: } why do we need this extra scope {}?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, Helen! https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:341: HttpResponseInfo promise_response_info; On 2017/06/21 20:55:28, xunjieli wrote: > optional nit: Move this closer to where it's needed (after line 347). Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:366: void QuicChromiumClientSession::Handle::OnRendezvousResult( On 2017/06/21 20:55:28, xunjieli wrote: > nit: these four new methods should in the same order as they are declared w.r.t. > to other methods in the header file. Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:384: QuicChromiumClientSession::Handle::ReleasePromisedStream() { On 2017/06/21 20:55:28, xunjieli wrote: > nit: Add a DCHECK(push_stream_) to make sure that the caller is calling this > correctly Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.h (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:92: // eventuallly completed |callback| will be called. On 2017/06/21 20:55:28, xunjieli wrote: > nit: s/eventuallly/eventually Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.h:106: // Releases |stream_| to the caller. On 2017/06/21 20:55:28, xunjieli wrote: > nit: s/stream_/push_stream_ Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:150: base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr())); On 2017/06/21 20:55:28, xunjieli wrote: > Should this be "base::Unretained(this)" given that QuicHttpStream owns the > handle? Hm! Interesting. I think that's possible! But since all the other calls to the handles base::Bind() with a WeakPtr, I'm inclined to leave this as-is in this CL and then do a follow-up to switch over. Though now that I think about it.. The HttpStream could pass an Unretained() callback into the Handle. Could the Handle post a task to execute that callback, which would end up running after the Handle is destroyed? I believe the Handle does not actually do that, but I'm curious about the contract... https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:157: if (rv < 0) { On 2017/06/21 20:55:28, xunjieli wrote: > nit: This branch is not needed. We can go directly to > STATE_HANDLE_PROMISE_COMPLETE, because DoHandlePromiseComplete() handles the > error case. Hah! Clever! In fact, we don't need any of these branches. Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:167: int QuicHttpStream::DoHandlePromiseComplete(int rv) { On 2017/06/21 20:55:28, xunjieli wrote: > optional nit: add a > DCHECK_NE(ERR_IO_PENDING, rv) > DCHECK_LE(rv, OK) > here just to make the expected range of values for |rv| clearer. Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4304: // is closed before the pushed header arrive, but after the connection On 2017/06/21 20:55:28, xunjieli wrote: > nit: s/arrive/arrives Done. (Well, s/header/headers/) https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4324: GetResponseHeaders("200 OK"), &server_header_offset)); On 2017/06/21 20:55:28, xunjieli wrote: > Sorry I am not familiar with how push promise works. Is this the response code > for GET / or GET /pushed.jpg ? It's the response headers for the first request ("/"). The response body for /pushed.jpg never actually arrives. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4327: 3, GetNthClientInitiatedStreamId(0), false, true, 0, "hello!")); On 2017/06/21 20:55:28, xunjieli wrote: > Could you annotate these data packets? Done. > Is this the response body of GET / or GET /pushed.jpg ? It's the response for the first request ("/"). The response for /pushed.jpg never actually arrives. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4339: // Start a push transaction that will be cancelled after the conneciton On 2017/06/21 20:55:28, xunjieli wrote: > nit: typo in connection. Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4343: TestCompletionCallback callback; On 2017/06/21 20:55:28, xunjieli wrote: > |callback| should still be notified, right? > Can we check its result at the end of this test? No, it will not be notified because the transaction is destroyed before the callback runs. (Which is the particular corner case which leads to this crash) https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4351: request_.url = GURL("https://mail.example.org/"); On 2017/06/21 20:55:28, xunjieli wrote: > nit: do not reuse |request_| for |trans2|. The Transaction.Start() takes in the > pointer address of |request_|. I am nervous about changing the HttpRequestInfo > while it is used by another transaction. Done. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4358: } On 2017/06/21 20:55:28, xunjieli wrote: > why do we need this extra scope {}? Ah. We need this so that |trans| will be closed. There's no Cancel() method on HttpNetworkTransaction. I could stuff it into a unique_ptr if you'd prefer?
https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:150: base::Bind(&QuicHttpStream::OnIOComplete, weak_factory_.GetWeakPtr())); On 2017/06/21 22:24:51, Ryan Hamilton wrote: > On 2017/06/21 20:55:28, xunjieli wrote: > > Should this be "base::Unretained(this)" given that QuicHttpStream owns the > > handle? > > Hm! Interesting. I think that's possible! But since all the other calls to the > handles base::Bind() with a WeakPtr, I'm inclined to leave this as-is in this CL > and then do a follow-up to switch over. > > Though now that I think about it.. The HttpStream could pass an Unretained() > callback into the Handle. Could the Handle post a task to execute that callback, > which would end up running after the Handle is destroyed? I believe the Handle > does not actually do that, but I'm curious about the contract... Acknowledged. Let's do it as a follow-up. If the Handle wants to take the callback and post it as a task, it should wrap it in a base::Bind(). The handle can use weak_ptr or base::Unretained() as appropriate for the wrapped callback. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4343: TestCompletionCallback callback; On 2017/06/21 22:24:51, Ryan Hamilton wrote: > On 2017/06/21 20:55:28, xunjieli wrote: > > |callback| should still be notified, right? > > Can we check its result at the end of this test? > > No, it will not be notified because the transaction is destroyed before the > callback runs. (Which is the particular corner case which leads to this crash) Acknowledged. https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4358: } On 2017/06/21 22:24:51, Ryan Hamilton wrote: > On 2017/06/21 20:55:28, xunjieli wrote: > > why do we need this extra scope {}? > > Ah. We need this so that |trans| will be closed. There's no Cancel() method on > HttpNetworkTransaction. I could stuff it into a unique_ptr if you'd prefer? |trans| is going out of scope anyway, right? This is the last line of the test. If I remove the scope, the test still crashes at ResetStream() without the patch. https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:210: auto* push_handle = push_handle_; Is saving |push_handle_| on to the stack still necessary? |this| won't be destroyed when session is closed. Is there a re-entrancy in Cancel() that I am not seeing? The following should be enough, right? if (push_handle_) { push_handle_->Cancel(); push_handle_ = nullptr; } https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4349: HttpNetworkTransaction trans(DEFAULT_PRIORITY, session_.get()); Minor nit: could you name this trans2, callback2? https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4356: HttpNetworkTransaction trans2(DEFAULT_PRIORITY, session_.get()); Minor nit: could you name this trans3, callback3 and request3? It took me a while to realize this is the third request and there is another one in SendRequestAndExpectQuicResponse(). https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4365: base::RunLoop().RunUntilIdle(); |callback2| has completed at this point, right? To make it more explicit, should we wait for callback2 to complete instead of using RunUntilIdle()?
Thanks! https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/20001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4358: } On 2017/06/22 14:43:30, xunjieli wrote: > On 2017/06/21 22:24:51, Ryan Hamilton wrote: > > On 2017/06/21 20:55:28, xunjieli wrote: > > > why do we need this extra scope {}? > > > > Ah. We need this so that |trans| will be closed. There's no Cancel() method on > > HttpNetworkTransaction. I could stuff it into a unique_ptr if you'd prefer? > > |trans| is going out of scope anyway, right? This is the last line of the test. > If I remove the scope, the test still crashes at ResetStream() without the > patch. Hahahhahahahahaha! Yes, you're obviously right, of course. Done. https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_chromium_client_session.cc (right): https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_chromium_client_session.cc:210: auto* push_handle = push_handle_; On 2017/06/22 14:43:30, xunjieli wrote: > Is saving |push_handle_| on to the stack still necessary? |this| won't be > destroyed when session is closed. Is there a re-entrancy in Cancel() that I am > not seeing? > > The following should be enough, right? > if (push_handle_) { > push_handle_->Cancel(); > push_handle_ = nullptr; > } So it "should" work, but I'm nervous. As you point out, if there is any reentrancy, this could be problematic. Since Cancel() can result in a write, it's not immediately obvious what bad things can happen. Since we're getting crashes in this code before the Handle code, I'm super nervous. I'll revert it if you feel strongly, but I'm nervous :) https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4349: HttpNetworkTransaction trans(DEFAULT_PRIORITY, session_.get()); On 2017/06/22 14:43:30, xunjieli wrote: > Minor nit: could you name this trans2, callback2? Done. https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4356: HttpNetworkTransaction trans2(DEFAULT_PRIORITY, session_.get()); On 2017/06/22 14:43:30, xunjieli wrote: > Minor nit: could you name this trans3, callback3 and request3? It took me a > while to realize this is the third request and there is another one in > SendRequestAndExpectQuicResponse(). Done. https://codereview.chromium.org/2948143002/diff/40001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4365: base::RunLoop().RunUntilIdle(); On 2017/06/22 14:43:30, xunjieli wrote: > |callback2| has completed at this point, right? To make it more explicit, should > we wait for callback2 to complete instead of using RunUntilIdle()? > Sure. Good point. In the process of doing this, I ended up switching to using a unique_ptr for the 2nd request in order to ensure that it is deleted before we WaitForResult on callback3. I think it's clearer now. PTAL.
Sounds good. LGTM https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4366: base::RunLoop().RunUntilIdle(); Maybe remove this base::RunLoop().RunUntilIdle();
Thanks! https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4366: base::RunLoop().RunUntilIdle(); On 2017/06/22 20:50:46, xunjieli wrote: > Maybe remove this base::RunLoop().RunUntilIdle(); So I initially tried replacing that with: EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR)) Thinking that they should both be equivalent. Namely, they spin the message loop to cause the write to complete and invoke callback3. However they turn out not to be equivalent in that, with the old code at least, if I use WaitForResult here, then the test fails when the pushed stream is cancelled after the session is already closed, which hits a DCHECK. It doesn't fail because of the crash. Of course, if I just remove RunUntilIdle, then the connection has not yet been closed when trans2 is destroyed and the old code does not crash. So I think I need it.
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...
still lgtm https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4366: base::RunLoop().RunUntilIdle(); On 2017/06/22 21:54:35, Ryan Hamilton wrote: > On 2017/06/22 20:50:46, xunjieli wrote: > > Maybe remove this base::RunLoop().RunUntilIdle(); > > So I initially tried replacing that with: > > EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR)) > > Thinking that they should both be equivalent. Namely, they spin the message loop > to cause the write to complete and invoke callback3. However they turn out not > to be equivalent in that, with the old code at least, if I use WaitForResult > here, then the test fails when the pushed stream is cancelled after the session > is already closed, which hits a DCHECK. It doesn't fail because of the crash. > > Of course, if I just remove RunUntilIdle, then the connection has not yet been > closed when trans2 is destroyed and the old code does not crash. > > So I think I need it. Acknowledged. I see. I don't fully understand this part, but I will trust your word for it :)
https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2948143002/diff/60001/net/quic/chromium/quic_... net/quic/chromium/quic_network_transaction_unittest.cc:4366: base::RunLoop().RunUntilIdle(); On 2017/06/22 22:33:04, xunjieli wrote: > On 2017/06/22 21:54:35, Ryan Hamilton wrote: > > On 2017/06/22 20:50:46, xunjieli wrote: > > > Maybe remove this base::RunLoop().RunUntilIdle(); > > > > So I initially tried replacing that with: > > > > EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR)) > > > > Thinking that they should both be equivalent. Namely, they spin the message > loop > > to cause the write to complete and invoke callback3. However they turn out not > > to be equivalent in that, with the old code at least, if I use WaitForResult > > here, then the test fails when the pushed stream is cancelled after the > session > > is already closed, which hits a DCHECK. It doesn't fail because of the crash. > > > > Of course, if I just remove RunUntilIdle, then the connection has not yet been > > closed when trans2 is destroyed and the old code does not crash. > > > > So I think I need it. > > Acknowledged. I see. I don't fully understand this part, but I will trust your > word for it :) That makes two of us :)
The CQ bit was unchecked by rch@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2948143002/#ps100001 (title: "ASAN")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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": 100001, "attempt_start_ts": 1498225108870070,
"parent_rev": "489d28390c0aee4c59b9c2621bb67930b579d007", "commit_rev":
"56ec40a93b88fe42be155062fa681a36fbace4a1"}
Message was sent while issue was closed.
Description was changed from ========== Add an async method to QuicChromiumClientSession::Handle for rendezvousing with promised streams. This fixes a bug where a session could be closed, and the push handle deleted before the QuicHttpStream was notified resulting in a use-after-free of the push handle. BUG=719461 ========== to ========== Add an async method to QuicChromiumClientSession::Handle for rendezvousing with promised streams. This fixes a bug where a session could be closed, and the push handle deleted before the QuicHttpStream was notified resulting in a use-after-free of the push handle. BUG=719461 Review-Url: https://codereview.chromium.org/2948143002 Cr-Commit-Position: refs/heads/master@{#481886} Committed: https://chromium.googlesource.com/chromium/src/+/56ec40a93b88fe42be155062fa68... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/56ec40a93b88fe42be155062fa68... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
