Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
417403002:
Remove weak pointers from CastSocket. (Closed)
|
Created:
6 years, 4 months ago by mark a. foltz Modified:
6 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRemove weak pointers from CastSocket by explicitly tracking and resetting callbacks created inside the class.
Ensure all sockets are closed and callbacks reset in all relevant code paths: Close(), CloseWithError(), and the dtor.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287477
Patch Set 1 #Patch Set 2 : Finish reorganizing callbacks. #
Total comments: 24
Patch Set 3 : #Patch Set 4 : Refactor to use CancelableCallback #
Total comments: 24
Patch Set 5 : Respond to Wez comments. Fire callbacks in CloseInternal. #
Total comments: 4
Patch Set 6 : Change when callbacks are run. #Patch Set 7 : Clarify deletion policies. #
Total comments: 8
Patch Set 8 : Update comments. #
Messages
Total messages: 24 (0 generated)
We should be using WeakPtrFactory rather than SupportsWeakPtr here, but what's the rationale for removing WeakPtrs entirely?
On 2014/07/28 20:55:29, Wez wrote: > We should be using WeakPtrFactory rather than SupportsWeakPtr here, but what's > the rationale for removing WeakPtrs entirely? rsleevi@ asked for it in a previous code review. They're also unnecessary as the lifetime of all callbacks is tied to the lifetime of the CastSocket, the TCP socket or the SSL socket.
On 2014/07/28 20:57:31, mark a. foltz wrote: > On 2014/07/28 20:55:29, Wez wrote: > > We should be using WeakPtrFactory rather than SupportsWeakPtr here, but what's > > the rationale for removing WeakPtrs entirely? > > rsleevi@ asked for it in a previous code review. They're also unnecessary as > the lifetime of all callbacks is tied to the lifetime of the CastSocket, the TCP > socket or the SSL socket. OK, understood re lifetimes; what was rsleevi's rationale, though?
On 2014/07/28 20:59:30, Wez wrote: > On 2014/07/28 20:57:31, mark a. foltz wrote: > > On 2014/07/28 20:55:29, Wez wrote: > > > We should be using WeakPtrFactory rather than SupportsWeakPtr here, but > what's > > > the rationale for removing WeakPtrs entirely? > > > > rsleevi@ asked for it in a previous code review. They're also unnecessary as > > the lifetime of all callbacks is tied to the lifetime of the CastSocket, the > TCP > > socket or the SSL socket. > > OK, understood re lifetimes; what was rsleevi's rationale, though? It was part of an offline conversation between Ryan and Munjal. Maybe ping them on IM? https://codereview.chromium.org/35443002/#msg21
https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); Should this be turned into PostNonNestableTask now, or should that be in a different CL? https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:314: // code decoupleld from connect loop code. typo https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:318: base::Bind(&CastSocket::SendCastMessageInternal, 4 space indentation https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:382: // Reset callbacks passed into us. Or should we invoke them with an error? Stop the timer? https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:383: connect_callback_.Reset(); Question: does resetting the callbacks also dequeue them from the message loop? https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:13: #include "base/cancelable_callback.h" Not used?
Not LGTM. We can't remove use of WeakPtr because we currently have several places where we use PostTask() to implement a continuation with a clean stack. We should definitely remove SupportsWeakPtr from this class, and replace it with a base::WeakPtrFactory member - I'm happy to draft a CL to take care of that. :) The two benefits of that are avoiding exposing the WeakPtr-able-ness of the object, and making sure that the WeakPtr is NULL'd before the members of the CastSocket class itself (not the case w/ SupportsWeakPtr, which breaks callbacks into it from destructors; hopefully we don't have any of those, though). However, we should re-write this class completely, to separate out the message framing handling from the connection setup logic - that should make the logic easier to grok so that we can get rid of the PostTask hacks. As a complete aside, we should also use base::NonThreadSafe rather than an explicit ThreadChecker, if possible. :) https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); On 2014/07/28 22:08:16, kmarshall wrote: > Should this be turned into PostNonNestableTask now, or should that be in a > different CL? PostNonNestableTask() isn't required here, since we don't nest MessageLoop::Run() in production code - it should always be safe to call PostTask(), w/out risking re-entrancy within the PostTask itself. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); This will blow up if PostTaskToStartConnectLoop() is called and something deletes the CastSocket before the task is processed. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:324: send_auth_challenge_callback_); This will blow up, as above. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:558: base::MessageLoop::current()->PostTask(FROM_HERE, read_loop_callback_); Boom! https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:74: // Ensures that the socket is closed. Not sure what this comment adds? https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:272: // Closes sockets, frees resources, and cancels pending callbacks. nit: underlying sockets? Does "Closes sockets" not come under "frees resources"?
On 2014/07/28 23:14:57, Wez wrote: > As a complete aside, we should also use base::NonThreadSafe rather than an > explicit ThreadChecker, if possible. :) No! Concrete inheritance to express traits is bad! :P ThreadChecker all the way :)
On 2014/07/28 23:25:42, Ryan Sleevi wrote: > On 2014/07/28 23:14:57, Wez wrote: > > As a complete aside, we should also use base::NonThreadSafe rather than an > > explicit ThreadChecker, if possible. :) > > No! Concrete inheritance to express traits is bad! :P ThreadChecker all the way > :) Hoisted by my own petard!
PTAL https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); On 2014/07/28 23:14:56, Wez wrote: > On 2014/07/28 22:08:16, kmarshall wrote: > > Should this be turned into PostNonNestableTask now, or should that be in a > > different CL? > > PostNonNestableTask() isn't required here, since we don't nest > MessageLoop::Run() in production code - it should always be safe to call > PostTask(), w/out risking re-entrancy within the PostTask itself. Acknowledged. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); On 2014/07/28 23:14:56, Wez wrote: > This will blow up if PostTaskToStartConnectLoop() is called and something > deletes the CastSocket before the task is processed. So Reset()-ing the callback in the dtor is a no-op? Is there any way to cancel a pending task once submitted to the MessageLoop or must we rely on indirect techniques like WeakPtr? https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:314: // code decoupleld from connect loop code. On 2014/07/28 22:08:16, kmarshall wrote: > typo Done. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:318: base::Bind(&CastSocket::SendCastMessageInternal, On 2014/07/28 22:08:16, kmarshall wrote: > 4 space indentation Done. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:324: send_auth_challenge_callback_); On 2014/07/28 23:14:56, Wez wrote: > This will blow up, as above. Acknowledged. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:382: // Reset callbacks passed into us. Or should we invoke them with an error? On 2014/07/28 22:08:16, kmarshall wrote: > Stop the timer? Done. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:383: connect_callback_.Reset(); On 2014/07/28 22:08:16, kmarshall wrote: > Question: does resetting the callbacks also dequeue them from the message loop? Good question. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:558: base::MessageLoop::current()->PostTask(FROM_HERE, read_loop_callback_); On 2014/07/28 23:14:56, Wez wrote: > Boom! Acknowledged. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:13: #include "base/cancelable_callback.h" On 2014/07/28 22:08:17, kmarshall wrote: > Not used? Done. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:74: // Ensures that the socket is closed. On 2014/07/28 23:14:57, Wez wrote: > Not sure what this comment adds? An invariant. https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:272: // Closes sockets, frees resources, and cancels pending callbacks. On 2014/07/28 23:14:56, Wez wrote: > nit: underlying sockets? > > Does "Closes sockets" not come under "frees resources"? Done.
https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:195: base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); On 2014/07/29 00:16:24, mark a. foltz wrote: > On 2014/07/28 23:14:56, Wez wrote: > > This will blow up if PostTaskToStartConnectLoop() is called and something > > deletes the CastSocket before the task is processed. > > So Reset()-ing the callback in the dtor is a no-op? base::Closure is ref-counted, so when you message_loop->PostTask(..., a) and then a.Reset(), the message_loop's queue still holds a reference to the task, even though |a| itself is cleared. > Is there any way to cancel a pending task once submitted to the MessageLoop or > must we rely on indirect techniques like WeakPtr? There is base::CancelableCallback. Internally, though, that just uses WeakPtrFactory. MessageQueues don't support cancellation, AFAIK, presumably because that would complicate them and impact performance of the common case of non-cancelled tasks.
On 2014/07/29 00:41:26, Wez wrote: > https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... > extensions/browser/api/cast_channel/cast_socket.cc:195: > base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); > On 2014/07/29 00:16:24, mark a. foltz wrote: > > On 2014/07/28 23:14:56, Wez wrote: > > > This will blow up if PostTaskToStartConnectLoop() is called and something > > > deletes the CastSocket before the task is processed. > > > > So Reset()-ing the callback in the dtor is a no-op? > > base::Closure is ref-counted, so when you message_loop->PostTask(..., a) and > then a.Reset(), the message_loop's queue still holds a reference to the task, > even though |a| itself is cleared. Ah, so anything submitted to a MessageLoop is by necessity weakly refcounted. > > > Is there any way to cancel a pending task once submitted to the MessageLoop or > > must we rely on indirect techniques like WeakPtr? > > There is base::CancelableCallback. Internally, though, that just uses > WeakPtrFactory. MessageQueues don't support cancellation, AFAIK, presumably > because that would complicate them and impact performance of the common case of > non-cancelled tasks. Not clear to me how explicit cancellation is more complicated or slower than the ref-counting/weak pointer combo we use instead. I guess there's the issue of thread safety. Regardless, these design decisions were made long ago so who am I to question them :) I'll convert CastSocket to use WeakPtrFactory. Note: - We'll lose the DCHECK-enforced invariants that there is at one most pending task to re-enter the Do{Connect,Read} loops. It seems to me we want to enforce this. I'll use booleans instead. - Is it safe to pass a non-weak pointer to the net:: libraries, since the socket objects own the callbacks and don't share them with a message loop? - ISTM there is a legitimate bug where we are not closing both sockets and resetting the callbacks passed into CastSocket via CloseWithError *or* in the dtor, so I want to keep those changes.
On 2014/07/29 18:22:52, mark a. foltz wrote: > On 2014/07/29 00:41:26, Wez wrote: > > > https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... > > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > > > > https://codereview.chromium.org/417403002/diff/20001/extensions/browser/api/c... > > extensions/browser/api/cast_channel/cast_socket.cc:195: > > base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_); > > On 2014/07/29 00:16:24, mark a. foltz wrote: > > > On 2014/07/28 23:14:56, Wez wrote: > > > > This will blow up if PostTaskToStartConnectLoop() is called and something > > > > deletes the CastSocket before the task is processed. > > > > > > So Reset()-ing the callback in the dtor is a no-op? > > > > base::Closure is ref-counted, so when you message_loop->PostTask(..., a) and > > then a.Reset(), the message_loop's queue still holds a reference to the task, > > even though |a| itself is cleared. > > Ah, so anything submitted to a MessageLoop is by necessity weakly refcounted. > > > > > > Is there any way to cancel a pending task once submitted to the MessageLoop > or > > > must we rely on indirect techniques like WeakPtr? > > > > There is base::CancelableCallback. Internally, though, that just uses > > WeakPtrFactory. MessageQueues don't support cancellation, AFAIK, presumably > > because that would complicate them and impact performance of the common case > of > > non-cancelled tasks. > > Not clear to me how explicit cancellation is more complicated or slower than the > ref-counting/weak pointer combo we use instead. I guess there's the issue of > thread safety. Regardless, these design decisions were made long ago so who am > I to question them :) > > I'll convert CastSocket to use WeakPtrFactory. Note: > > - We'll lose the DCHECK-enforced invariants that there is at one most pending > task to re-enter the Do{Connect,Read} loops. It seems to me we want to enforce > this. I'll use booleans instead. Good point; perhaps CancelableCallback would be more appropriate for that reason? > - Is it safe to pass a non-weak pointer to the net:: libraries, since the socket > objects own the callbacks and don't share them with a message loop? No - IIUC the WeakPtr checking is specific to MessageLoop's processing of tasks; normal task.Run() will just end up running the task either with a NULL |this| or a DCHECK if it's NULL. Again, CancelableCallback would address this. > - ISTM there is a legitimate bug where we are not closing both sockets and > resetting the callbacks passed into CastSocket via CloseWithError *or* in the > dtor, so I want to keep those changes. Yes, please do!
Updated to use CancelableCallback. PTAL.
https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:183: base::Unretained(this))); nit: Style guide would prefer wrapping this after "Reset(". https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:272: DCHECK(connect_loop_callback_.IsCancelled()); nit: Check this on entry to DoTcpConnect()? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:272: DCHECK(connect_loop_callback_.IsCancelled()); IIUC this is basically checking that after a PostTaskToRestartConnectLoop we haven't somehow come around DoConnectLoop again, without having first returned to the main loop? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:292: DCHECK(connect_loop_callback_.IsCancelled()); nit: Check this on entry to the method? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:325: base::Unretained(this)))); Ick... do we really need a clean stack for SendCastMessageInternal as well as for the completion callback...? IIUC the problem here is basically with actions that the Delegate might perform in error callbacks, in which case can we simplify all this code to avoid the need for stack-popping via PostTask on operations like this by just using PostTask to deliver the callbacks to the Delegate...? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:381: // |callback| can delete |this| nit: Callers must inherently cope with asynchronous completion of close, so you could just PostTask() this, and deletion wouldn't be an issue? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:397: // Reset callbacks passed into us. Or should we invoke them with an error? If the socket is being closed then there shouldn't be any problem with just dropping write callbacks, I don't think? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:397: // Reset callbacks passed into us. Or should we invoke them with an error? nit: Suggest blank line before this comment, and before the comment below, to visually separate this function into logical blocks. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:332: // Callback invoked to cancel the connection. nit: Invoked by whom? https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:333: base::CancelableClosure cancel_connect_callback_; nit: Consider connect_timeout_callback_ https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:353: // Callback invoked to (re)start the connect loop. nit: Clarify when these callbacks is are in the "cancelled" state? e.g. "Callback posted by PostTaskToStartConnectLoop(). This is cleared when there is no connect loop post-task pending."
https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:183: base::Unretained(this))); On 2014/07/30 19:14:53, Wez wrote: > nit: Style guide would prefer wrapping this after "Reset(". It prefers wrapping at parenthesis https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:272: DCHECK(connect_loop_callback_.IsCancelled()); On 2014/07/30 19:14:53, Wez wrote: > nit: Check this on entry to DoTcpConnect()? Done. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:272: DCHECK(connect_loop_callback_.IsCancelled()); On 2014/07/30 19:14:53, Wez wrote: > IIUC this is basically checking that after a PostTaskToRestartConnectLoop we > haven't somehow come around DoConnectLoop again, without having first returned > to the main loop? There should be at most one pending callback to reenter the connect loop. Having multiple pending callbacks (from both net:: and the MessageLoop) would mean the behavior of the state machine becomes indeterminate. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:292: DCHECK(connect_loop_callback_.IsCancelled()); On 2014/07/30 19:14:53, Wez wrote: > nit: Check this on entry to the method? Done. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:325: base::Unretained(this)))); On 2014/07/30 19:14:53, Wez wrote: > Ick... do we really need a clean stack for SendCastMessageInternal as well as > for the completion callback...? It appears to be "not strictly necessary" but otherwise the control flow for the two loops gets entangled. > > IIUC the problem here is basically with actions that the Delegate might perform > in error callbacks, in which case can we simplify all this code to avoid the > need for stack-popping via PostTask on operations like this by just using > PostTask to deliver the callbacks to the Delegate...? No I think even in the normal flow we wanted to avoid mutual recursion between DoConnectLoop and DoWriteLoop. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:381: // |callback| can delete |this| On 2014/07/30 19:14:53, Wez wrote: > nit: Callers must inherently cope with asynchronous completion of close, so you > could just PostTask() this, and deletion wouldn't be an issue? This comment is out of date and removed. Deletion can happen at any time via the ApiResourceManager if the extension is disabled or the browser shuts down. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:397: // Reset callbacks passed into us. Or should we invoke them with an error? On 2014/07/30 19:14:53, Wez wrote: > nit: Suggest blank line before this comment, and before the comment below, to > visually separate this function into logical blocks. Done. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:397: // Reset callbacks passed into us. Or should we invoke them with an error? On 2014/07/30 19:14:53, Wez wrote: > If the socket is being closed then there shouldn't be any problem with just > dropping write callbacks, I don't think? Actually, there may be a Promise pending on the caller side waiting for the callback to complete, so now I fire them. This required refactoring cast_channel_api.cc since we cannot guarantee the API callbacks are run while the socket still exists in the ApiResourceManager. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:332: // Callback invoked to cancel the connection. On 2014/07/30 19:14:53, Wez wrote: > nit: Invoked by whom? Done. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:333: base::CancelableClosure cancel_connect_callback_; On 2014/07/30 19:14:53, Wez wrote: > nit: Consider connect_timeout_callback_ Done. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:353: // Callback invoked to (re)start the connect loop. On 2014/07/30 19:14:53, Wez wrote: > nit: Clarify when these callbacks is are in the "cancelled" state? > > e.g. "Callback posted by PostTaskToStartConnectLoop(). This is cleared when > there is no connect loop post-task pending." Done.
https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:55: // Do NOT delete the socket in this callback. Is it now always OK to delete the socket from these callbacks, or never OK? :) https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_channel_api.cc:346: if (!socket) { Is there a simple test we can add to verify that this new tolerance to channel-ids not resolving to sockets doesn't regress? https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:399: connect_callback_.Run(net::ERR_CONNECTION_FAILED); So there is no possibility of this callback deleting this object?
Refactored out a bit so I no longer run callbacks in the dtor. They are only run in the normal Close() or CloseWithError() flows. https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/60001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.h:55: // Do NOT delete the socket in this callback. On 2014/07/31 22:57:53, Wez wrote: > Is it now always OK to delete the socket from these callbacks, or never OK? :) Removed. https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_channel_api.cc:346: if (!socket) { On 2014/07/31 22:57:53, Wez wrote: > Is there a simple test we can add to verify that this new tolerance to > channel-ids not resolving to sockets doesn't regress? I tried writing an API test for this case but it is hard to reproduce since I would have to unload the entire extension in the middle of the test to get the ApiResourceManager to delete all sockets. Added a TODO to mock out the ApiResourceManager in cast_channel_apitest, in another patch. https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/80001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:399: connect_callback_.Run(net::ERR_CONNECTION_FAILED); On 2014/07/31 22:57:53, Wez wrote: > So there is no possibility of this callback deleting this object? Yes this is possible and that would be bad. The only callback it is safe to delete the object should be the one for Close() since it is invoked last. The CastChannelAPI owns this object as well as the callbacks, and should know better, but do you suggest a boolean guard against this?
I added some comments to the code to clarify deletion policy to be conservative: deleting the socket is okay in the OnClose callback, but never in the delegate or in other callbacks. This means the client must Close() all sockets that are created, successful or not. (Even if connection fails, it will still get the channel_id of the allocated socket.)
lgtm https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:68: std::numeric_limits<uint16>::max(); unit16_t https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:433: // This will delete the socket. Must not use |socket| from here forward. Consider NULLing |socket| so things will blow up if that gets violated. https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.cc:105: CloseInternal(); If the caller never deletes CastSocket, only calls Close(), and Close() calls CloseInternal(), should we just [D]CHECK that the CastSocket was already Close()d here? https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:49: // not be deleted by the delegate. Who does delete it? The API resource manager? In which case consider changing this do "The CastSocket is owned by the API resource manager; the delegate must therefore call Close() on it rather than deleting it directly." Or must it always be deleted in the Close() callback, as implied in the Close() comment below?
https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_channel_api.cc (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:68: std::numeric_limits<uint16>::max(); On 2014/08/04 21:32:32, Wez wrote: > unit16_t Done. https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_channel_api.cc:433: // This will delete the socket. Must not use |socket| from here forward. On 2014/08/04 21:32:32, Wez wrote: > Consider NULLing |socket| so things will blow up if that gets violated. Done. https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.cc:105: CloseInternal(); On 2014/08/04 21:32:32, Wez wrote: > If the caller never deletes CastSocket, only calls Close(), and Close() calls > CloseInternal(), should we just [D]CHECK that the CastSocket was already > Close()d here? On extension unload/browser close, the ApiResourceManager will just delete the CastSocket without calling Close() first. https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/417403002/diff/120001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:49: // not be deleted by the delegate. On 2014/08/04 21:32:32, Wez wrote: > Who does delete it? The API resource manager? In which case consider changing > this do "The CastSocket is owned by the API resource manager; the delegate must > therefore call Close() on it rather than deleting it directly." > > Or must it always be deleted in the Close() callback, as implied in the Close() > comment below? Both cases are possible, updated comment to mention this.
The CQ bit was checked by mfoltz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mfoltz@chromium.org/417403002/140001
Message was sent while issue was closed.
Change committed as 287477 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
