Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511773003:
Fix potential teardown race conditions with TCPEngineTransport's PostTasks. (Closed)
|
Created:
4 years, 1 month ago by Kevin M Modified:
4 years, 1 month ago Reviewers:
Wez CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix potential teardown race conditions with TCPEngineTransport's PostTasks.
* Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion.
* Replace use Unretained() with a weak pointer on async connection completion.
R=wez@chromium.org
BUG=663970
Committed: https://crrev.com/58b2a9cbdd3184364a2fd9173c95e2a907296b77
Cr-Commit-Position: refs/heads/master@{#433260}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use bound callbacks #Patch Set 3 : git cl format #
Total comments: 6
Patch Set 4 : wez feedback #Messages
Total messages: 15 (5 generated)
Description was changed from ========== Fix potential teardown race condition TCPEngineTransport async hopping. * Use WeakPtrFactory to ensure that completion callbacks are only invoked if the transport object is alive. * Replace use of Unretained() for async PostTask with weak pointer. R=wez@chromium.org BUG=663970 ========== to ========== Fix potential teardown race conditions with TCPEngineTransport's PostTasks. * Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion. * Replace use Unretained() with a weak pointer on async connection completion. R=wez@chromium.org BUG=663970 ==========
https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... blimp/net/tcp_engine_transport.cc:48: connect_callback_ = callback; Could you just pass the connect_callback_ as a parameter to OnTCPConnectAccepted using base::Bind? This gets rid of this temporary yet permanent member variable. You could remove the weak pointer in this case. (Case in point: https://codereview.chromium.org/2509163002/diff/20001/blimp/net/tcp_engine_tr...)
https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... blimp/net/tcp_engine_transport.cc:48: connect_callback_ = callback; On 2016/11/17 22:44:22, perumaal wrote: > Could you just pass the connect_callback_ as a parameter to OnTCPConnectAccepted > using base::Bind? This gets rid of this temporary yet permanent member variable. > You could remove the weak pointer in this case. > > (Case in point: > https://codereview.chromium.org/2509163002/diff/20001/blimp/net/tcp_engine_tr...) From the bug: "TCPEngineTransport::Connect() uses PostTask to complete asynchronously in the case of the underlying TCP Accept() call completely synchronously, but posts the completion-callback directly as the task. This means that tearing down the Transport may still leave the completion callback in-flight, which callers would not typically expect to be the case."
https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2511773003/diff/1/blimp/net/tcp_engine_transp... blimp/net/tcp_engine_transport.cc:48: connect_callback_ = callback; On 2016/11/17 23:02:49, Kevin M wrote: > On 2016/11/17 22:44:22, perumaal wrote: > > Could you just pass the connect_callback_ as a parameter to > OnTCPConnectAccepted > > using base::Bind? This gets rid of this temporary yet permanent member > variable. Sorry, I misread. Yes, I can move the callback as a bound param. Can't remove the weak pointer, though - it guarantees that the callback is only invoked if |this| is alive.
Done.
LGTM w/ some suggestions. https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:47: weak_factory_.GetWeakPtr(), callback); nit: Up to you, but I quite liked that the |callback| got saved into the object, and later ResetAndReturn().Run()'d, since there are other stateful aspects to this, e.g. the |accepted_socket_| anyway. It also has the nice property that you can DCHECK(!connect_callback_) at the top of Connect() to trap duplicate calls, if you want. https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:82: server_socket_.reset(); nit: Is this strictly correct? I have vague recollections of rare situations in which you can be woken to accept an incoming TCP connection, only to find that Accept returns nothing, at least under Linux... https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:85: base::ResetAndReturn(&callback).Run(result); nit: If you leave this being passed the callback as a parameter then you don't really need ResetAndReturn :)
https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... File blimp/net/tcp_engine_transport.cc (right): https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:47: weak_factory_.GetWeakPtr(), callback); On 2016/11/18 02:35:47, Wez wrote: > nit: Up to you, but I quite liked that the |callback| got saved into the object, > and later ResetAndReturn().Run()'d, since there are other stateful aspects to > this, e.g. the |accepted_socket_| anyway. It also has the nice property that you > can DCHECK(!connect_callback_) at the top of Connect() to trap duplicate calls, > if you want. Yeah, that's true. I'll revert that change. https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:82: server_socket_.reset(); On 2016/11/18 02:35:47, Wez wrote: > nit: Is this strictly correct? I have vague recollections of rare situations in > which you can be woken to accept an incoming TCP connection, only to find that > Accept returns nothing, at least under Linux... Done. https://codereview.chromium.org/2511773003/diff/40001/blimp/net/tcp_engine_tr... blimp/net/tcp_engine_transport.cc:85: base::ResetAndReturn(&callback).Run(result); On 2016/11/18 02:35:47, Wez wrote: > nit: If you leave this being passed the callback as a parameter then you don't > really need ResetAndReturn :) Acknowledged.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2511773003/#ps60001 (title: "wez feedback")
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 ========== Fix potential teardown race conditions with TCPEngineTransport's PostTasks. * Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion. * Replace use Unretained() with a weak pointer on async connection completion. R=wez@chromium.org BUG=663970 ========== to ========== Fix potential teardown race conditions with TCPEngineTransport's PostTasks. * Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion. * Replace use Unretained() with a weak pointer on async connection completion. R=wez@chromium.org BUG=663970 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix potential teardown race conditions with TCPEngineTransport's PostTasks. * Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion. * Replace use Unretained() with a weak pointer on async connection completion. R=wez@chromium.org BUG=663970 ========== to ========== Fix potential teardown race conditions with TCPEngineTransport's PostTasks. * Use a weak pointer to ensure that TCPEngineTransport is alive when we invoke the completion callback on sync connection completion. * Replace use Unretained() with a weak pointer on async connection completion. R=wez@chromium.org BUG=663970 Committed: https://crrev.com/58b2a9cbdd3184364a2fd9173c95e2a907296b77 Cr-Commit-Position: refs/heads/master@{#433260} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/58b2a9cbdd3184364a2fd9173c95e2a907296b77 Cr-Commit-Position: refs/heads/master@{#433260} |
