Changed MessageReader so that it doesn't read from the socket if there are

remoting/protocol/client_message_dispatcher.cc

Index: remoting/protocol/client_message_dispatcher.cc
diff --git a/remoting/protocol/client_message_dispatcher.cc b/remoting/protocol/client_message_dispatcher.cc
index 95686850f895e60ba3ea62e3fc949840b77da8a5..d04bb38fbd5c7f3c6234fd812baa9920a73d69d1 100644
--- a/remoting/protocol/client_message_dispatcher.cc
+++ b/remoting/protocol/client_message_dispatcher.cc
@@ -11,7 +11,6 @@
 #include "remoting/protocol/client_stub.h"
 #include "remoting/protocol/input_stub.h"
 #include "remoting/protocol/message_reader.h"
-#include "remoting/protocol/ref_counted_message.h"
 #include "remoting/protocol/session.h"
 
 namespace remoting {
@@ -39,18 +38,17 @@ void ClientMessageDispatcher::Initialize(
 }
 
 void ClientMessageDispatcher::OnControlMessageReceived(
-    ControlMessage* message) {
-  scoped_refptr<RefCountedMessage<ControlMessage> > ref_msg =
-      new RefCountedMessage<ControlMessage>(message);
+    ControlMessage* message, Task* done_task) {
   if (message->has_notify_resolution()) {
     client_stub_->NotifyResolution(
-        &message->notify_resolution(), NewDeleteTask(ref_msg));
+        &message->notify_resolution(), done_task);
   } else if (message->has_begin_session_response()) {
     client_stub_->BeginSessionResponse(
-        &message->begin_session_response().login_status(),
-        NewDeleteTask(ref_msg));
+        &message->begin_session_response().login_status(), done_task);
   } else {
-    NOTREACHED() << "Invalid control message received";
+    LOG(ERROR) << "Invalid control message received.";

[awong, 2011/01/20 20:06:38]

LOG(WARNING) please.

Let's try to keep LOG(ERROR) for events that are more like "omg! no clue what is
happening!  I'd crash right now, but I can still limp along...maybe"  kind of
states. :)

[Alpha Left Google, 2011/01/20 20:54:52]

If it's something really unexpected like this then NOTREACHED is better, it's
bad enough to receive a corrupted message.

[awong, 2011/01/20 21:01:56]

Is the API contract that the control message is valid before being passed to
this function?  If so, then I agree with alpha that NOTREACHED is better.

Then, I would also expect an "IsValidControlMessage" function that we can
unittest, and which guards the callpoint to this handler.

[Sergey Ulanov, 2011/01/20 21:55:57]

Done.

[Sergey Ulanov, 2011/01/20 21:55:57]

If you put NOTREACHED here, it will be very easy to DoS a host: just send an
invalid message to it.
|message| is received from the network, so we don't have any guarantees about it
whatsoever.

[Sergey Ulanov, 2011/01/20 21:55:57]

No, it is not API contract here. |message| is just raw message we've received
from the network. We don't know if it is valid or not.
Yeah, we probably should add more validation code here (e.g. verify that
NotifyResulution contains resolution, etc), but it is not part of this CL.

[awong, 2011/01/20 22:08:02]

Yep...completley understood.  Can you file a P1 bug for M11, and add a TODO?

[Sergey Ulanov, 2011/01/21 03:45:05]

Added TODO, and filed a bug. I don't agree that this is P1: message stubs should
be protected from invalid messages anyway. Feel free to update 70335 if you
disagree.

+    done_task->Run();
+    delete done_task;
   }
 }