Removes setting destroyed_flag_ in NotifyError

mojo/public/cpp/bindings/lib/connector.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/public/cpp/bindings/lib/connector.h"
 
 #include <stddef.h>
 
 #include "mojo/public/cpp/bindings/error_handler.h"
 #include "mojo/public/cpp/environment/logging.h"
@@ skipping 180 matching lines @@
 void Connector::CancelWait() {
   if (!async_wait_id_)
     return;
 
   waiter_->CancelWait(async_wait_id_);
   async_wait_id_ = 0;
 }
 
 void Connector::NotifyError() {
   error_ = true;
-  // The error handler might destroyed |this|. Also, after an error, all method
-  // should end early.
-  if (destroyed_flag_) {
-    *destroyed_flag_ = true;  // Propagate flag.

[darin (slow to review), 2014/08/21 23:47:26]

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.

-  }
+  CancelWait();

[qsr, 2014/08/22 08:28:59]

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.

[sky, 2014/08/22 15:06:00]

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.

[viettrungluu, 2014/08/22 15:30:57]

Do we have a test for this case?

[darin (slow to review), 2014/08/22 16:11:27]

Would probably be good to add a comment explaining this.

   if (error_handler_)
     error_handler_->OnConnectionError();

[qsr, 2014/08/22 08:28:59]

I think we need to handle the case where the error handler destroy this class in
a nested call.

[sky, 2014/08/22 15:06:00]

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.

[qsr, 2014/08/22 15:21:25]

 Sorry about this. You are right.

 }
 
 }  // namespace internal
 }  // namespace mojo