Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
494243002:
Removes setting destroyed_flag_ in NotifyError (Closed)
|
Created:
6 years, 4 months ago by sky Modified:
6 years, 4 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemoves setting destroyed_flag_ in NotifyError
I'm changing two things to Connector:
. Removing setting destroyed_flag in NotifyError. This seems wrong
since Connector hasn't been destroyed. If destoyed_flag_ was
non-NULL and we set it to true and we're nested, then when the stack
unravels destroyed_flag_ won't be unset even though Connector may
not have been destroyed. This would leave Connector in a very bad
state.
. Makes NotifyError cancel a wait. If there was a wait outstanding or
a notification was in flight than if we don't cancel the wait we may
very well end up back in OnHandleReady trying to do something.
BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291354
Patch Set 1 #
Total comments: 8
Messages
Total messages: 20 (0 generated)
LGTM https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (left): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:204: *destroyed_flag_ = true; // Propagate flag. The "Propagate flag" comment makes me wonder if there isn't something we should be doing related to nesting. The current code seems obviously bogus though.
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/494243002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sky@chromium.org/494243002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
Message was sent while issue was closed.
Committed patchset #1 (1) as 291354
Message was sent while issue was closed.
Do you have an example where there is a problem with the code as it was? The only thing that setting the flag does is having early returns so that we do not call any more method on connector during a dispatch (it is only used in ReadSingleMessage to allow early return without touching any state). As the code is now, it will break if an error handler destroys the connector on a nested call. I think the code as it was is correct.
Message was sent while issue was closed.
https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (right): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); What is the code path where we can be waiting? From what I can see, we are never in the waiting state when calling this method. https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:203: error_handler_->OnConnectionError(); I think we need to handle the case where the error handler destroy this class in a nested call.
Message was sent while issue was closed.
I think I've addressed your earlier comment qsr. Let me know if I'm missing something. Regardless of that I'm going to convert connector to a weakref. https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (right): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); On 2014/08/22 08:28:59, qsr wrote: > What is the code path where we can be waiting? From what I can see, we are never > in the waiting state when calling this method. WaitForIncomingMessage is public and can be invoked by anyone. WaitForIncomingMessage does not attempt to cancel the AsycWait request. If there is an outstanding wait and we get here it means we'll notify of the connection error and then get a callback on OnHandleReady and potentially end up right back in here. https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:203: error_handler_->OnConnectionError(); On 2014/08/22 08:28:59, qsr wrote: > I think we need to handle the case where the error handler destroy this class in > a nested call. Unless I'm missing something, that's fine. The destructor sets destroyed_flag_ as appropriate. The problem with setting destroyed_flag_ here is that if we're not in the destructor than the code around 158 is going to early out and leave destroyed_flag_ pointing to something on the stack. When the destructor eventually runs it'll set destroyed_flag_, which is now pointing at some random memory location. Badness. Using a destroyed_flag_ is fragile and error prone. We should really use a WeakRef. I'll see about converting to that.
Message was sent while issue was closed.
https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (right): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:203: error_handler_->OnConnectionError(); On 2014/08/22 15:06:00, sky wrote: > On 2014/08/22 08:28:59, qsr wrote: > > I think we need to handle the case where the error handler destroy this class > in > > a nested call. > > Unless I'm missing something, that's fine. The destructor sets destroyed_flag_ > as appropriate. The problem with setting destroyed_flag_ here is that if we're > not in the destructor than the code around 158 is going to early out and leave > destroyed_flag_ pointing to something on the stack. When the destructor > eventually runs it'll set destroyed_flag_, which is now pointing at some random > memory location. Badness. Sorry about this. You are right. > Using a destroyed_flag_ is fragile and error prone. We should really use a > WeakRef. I'll see about converting to that.
Message was sent while issue was closed.
https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (right): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); On 2014/08/22 15:06:00, sky wrote: > On 2014/08/22 08:28:59, qsr wrote: > > What is the code path where we can be waiting? From what I can see, we are > never > > in the waiting state when calling this method. > > WaitForIncomingMessage is public and can be invoked by anyone. > WaitForIncomingMessage does not attempt to cancel the AsycWait request. If there > is an outstanding wait and we get here it means we'll notify of the connection > error and then get a callback on OnHandleReady and potentially end up right back > in here. Do we have a test for this case?
Message was sent while issue was closed.
On 2014/08/22 15:30:57, viettrungluu wrote: > https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... > File mojo/public/cpp/bindings/lib/connector.cc (right): > > https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... > mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); > On 2014/08/22 15:06:00, sky wrote: > > On 2014/08/22 08:28:59, qsr wrote: > > > What is the code path where we can be waiting? From what I can see, we are > > never > > > in the waiting state when calling this method. > > > > WaitForIncomingMessage is public and can be invoked by anyone. > > WaitForIncomingMessage does not attempt to cancel the AsycWait request. If > there > > is an outstanding wait and we get here it means we'll notify of the connection > > error and then get a callback on OnHandleReady and potentially end up right > back > > in here. > > Do we have a test for this case? Not that I'm aware of.
On Fri, Aug 22, 2014 at 8:36 AM, <sky@chromium.org> wrote: > On 2014/08/22 15:30:57, viettrungluu wrote: > > https://codereview.chromium.org/494243002/diff/1/mojo/ > public/cpp/bindings/lib/connector.cc > >> File mojo/public/cpp/bindings/lib/connector.cc (right): >> > > > https://codereview.chromium.org/494243002/diff/1/mojo/ > public/cpp/bindings/lib/connector.cc#newcode201 > >> mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); >> On 2014/08/22 15:06:00, sky wrote: >> > On 2014/08/22 08:28:59, qsr wrote: >> > > What is the code path where we can be waiting? From what I can see, >> we are >> > never >> > > in the waiting state when calling this method. >> > >> > WaitForIncomingMessage is public and can be invoked by anyone. >> > WaitForIncomingMessage does not attempt to cancel the AsycWait request. >> If >> there >> > is an outstanding wait and we get here it means we'll notify of the >> > connection > >> > error and then get a callback on OnHandleReady and potentially end up >> right >> back >> > in here. >> > > Do we have a test for this case? >> > > Not that I'm aware of. > Probably we should add one? > > https://codereview.chromium.org/494243002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Aug 22, 2014 at 8:53 AM, Viet-Trung Luu <viettrungluu@chromium.org> wrote: > On Fri, Aug 22, 2014 at 8:36 AM, <sky@chromium.org> wrote: >> >> On 2014/08/22 15:30:57, viettrungluu wrote: >> >> >> https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... >>> >>> File mojo/public/cpp/bindings/lib/connector.cc (right): >> >> >> >> >> https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... >>> >>> mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); >>> On 2014/08/22 15:06:00, sky wrote: >>> > On 2014/08/22 08:28:59, qsr wrote: >>> > > What is the code path where we can be waiting? From what I can see, >>> > > we are >>> > never >>> > > in the waiting state when calling this method. >>> > >>> > WaitForIncomingMessage is public and can be invoked by anyone. >>> > WaitForIncomingMessage does not attempt to cancel the AsycWait request. >>> > If >>> there >>> > is an outstanding wait and we get here it means we'll notify of the >> >> connection >>> >>> > error and then get a callback on OnHandleReady and potentially end up >>> > right >>> back >>> > in here. >> >> >>> Do we have a test for this case? >> >> >> Not that I'm aware of. > > > Probably we should add one? Yes please. > >> >> >> https://codereview.chromium.org/494243002/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
public/cpp/bindings/tests/connector_unittest.cc could probably be adapted to test the case of WaitForIncomingMessage triggering OnConnectionError. https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... File mojo/public/cpp/bindings/lib/connector.cc (right): https://codereview.chromium.org/494243002/diff/1/mojo/public/cpp/bindings/lib... mojo/public/cpp/bindings/lib/connector.cc:201: CancelWait(); On 2014/08/22 15:06:00, sky wrote: > On 2014/08/22 08:28:59, qsr wrote: > > What is the code path where we can be waiting? From what I can see, we are > never > > in the waiting state when calling this method. > > WaitForIncomingMessage is public and can be invoked by anyone. > WaitForIncomingMessage does not attempt to cancel the AsycWait request. If there > is an outstanding wait and we get here it means we'll notify of the connection > error and then get a callback on OnHandleReady and potentially end up right back > in here. Would probably be good to add a comment explaining this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
