[Blimp] Always invoke callback after a feature finishes processing a message.

Network layer waits for the callback to be invoked with OK before it reads next message. If a feature does not invokes callback, it will block all features from receiving more messages.

BUG=576350

Review URL: https://codereview.chromium.org/1571413003

Cr-Commit-Position: refs/heads/master@{#368722}

10 files changed, 42 insertions, 24 deletions

diff --git a/blimp/client/session/navigation_feature.cc b/blimp/client/session/navigation_feature.cc
index c02931a..86b0a09 100644
--- a/blimp/client/session/navigation_feature.cc
+++ b/blimp/client/session/navigation_feature.cc
@@ -78,6 +78,7 @@ void NavigationFeature::GoBack(int tab_id) {
 void NavigationFeature::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
   DCHECK(message->type() == BlimpMessage::NAVIGATION);
 
   int tab_id = message->target_tab_id();
@@ -85,6 +86,7 @@ void NavigationFeature::ProcessMessage(
   const NavigationMessage& navigation_message = message->navigation();
 
   NavigationFeatureDelegate* delegate = FindDelegate(tab_id);
+  DCHECK(delegate) << "NavigationFeatureDelegate not found for tab " << tab_id;
   switch (navigation_message.type()) {
     case NavigationMessage::NAVIGATION_STATE_CHANGED: {
       const NavigationStateChangeMessage& details =
@@ -112,8 +114,7 @@ void NavigationFeature::ProcessMessage(
       NOTREACHED();
   }
 
-  if (!callback.is_null())
-    callback.Run(net::OK);
+  callback.Run(net::OK);
 }
 
 NavigationFeature::NavigationFeatureDelegate* NavigationFeature::FindDelegate(
diff --git a/blimp/client/session/navigation_feature_unittest.cc b/blimp/client/session/navigation_feature_unittest.cc
index fb154e66..138924e 100644
--- a/blimp/client/session/navigation_feature_unittest.cc
+++ b/blimp/client/session/navigation_feature_unittest.cc
@@ -10,6 +10,7 @@
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/net/test_common.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -50,7 +51,9 @@ void SendMockNavigationStateChangedMessage(BlimpMessageProcessor* processor,
   if (loading)
     state->set_loading(*loading);
 
-  processor->ProcessMessage(std::move(message), net::CompletionCallback());
+  net::TestCompletionCallback cb;
+  processor->ProcessMessage(std::move(message), cb.callback());
+  EXPECT_EQ(net::OK, cb.WaitForResult());
 }
 
 MATCHER_P2(EqualsNavigateToUrlText, tab_id, text, "") {
diff --git a/blimp/client/session/render_widget_feature.cc b/blimp/client/session/render_widget_feature.cc
index c75372d..fddbe47 100644
--- a/blimp/client/session/render_widget_feature.cc
+++ b/blimp/client/session/render_widget_feature.cc
@@ -82,14 +82,14 @@ void RenderWidgetFeature::RemoveDelegate(const int tab_id) {
 void RenderWidgetFeature::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
   DCHECK(message->type() == BlimpMessage::RENDER_WIDGET ||
          message->type() == BlimpMessage::COMPOSITOR);
 
   int target_tab_id = message->target_tab_id();
   RenderWidgetFeatureDelegate* delegate = FindDelegate(target_tab_id);
-  DCHECK(delegate);
-  if (!delegate)
-    return;
+  DCHECK(delegate) << "RenderWidgetFeatureDelegate not found for "
+                   << target_tab_id;
 
   switch (message->type()) {
     case BlimpMessage::RENDER_WIDGET:
@@ -112,9 +112,7 @@ void RenderWidgetFeature::ProcessMessage(
       NOTIMPLEMENTED();
   }
 
-  if (!callback.is_null()) {
-    callback.Run(net::OK);
-  }
+  callback.Run(net::OK);
 }
 
 RenderWidgetFeature::RenderWidgetFeatureDelegate*
diff --git a/blimp/client/session/render_widget_feature_unittest.cc b/blimp/client/session/render_widget_feature_unittest.cc
index 9629fde..49171c6 100644
--- a/blimp/client/session/render_widget_feature_unittest.cc
+++ b/blimp/client/session/render_widget_feature_unittest.cc
@@ -13,6 +13,7 @@
 #include "blimp/net/test_common.h"
 #include "cc/proto/compositor_message.pb.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -54,7 +55,9 @@ void SendRenderWidgetMessage(BlimpMessageProcessor* processor,
   scoped_ptr<BlimpMessage> message = CreateBlimpMessage(&details, tab_id);
   details->set_type(RenderWidgetMessage::INITIALIZE);
   details->set_render_widget_id(rw_id);
-  processor->ProcessMessage(std::move(message), net::CompletionCallback());
+  net::TestCompletionCallback cb;
+  processor->ProcessMessage(std::move(message), cb.callback());
+  EXPECT_EQ(net::OK, cb.WaitForResult());
 }
 
 void SendCompositorMessage(BlimpMessageProcessor* processor,
@@ -63,7 +66,9 @@ void SendCompositorMessage(BlimpMessageProcessor* processor,
   CompositorMessage* details;
   scoped_ptr<BlimpMessage> message = CreateBlimpMessage(&details, tab_id);
   details->set_render_widget_id(rw_id);
-  processor->ProcessMessage(std::move(message), net::CompletionCallback());
+  net::TestCompletionCallback cb;
+  processor->ProcessMessage(std::move(message), cb.callback());
+  EXPECT_EQ(net::OK, cb.WaitForResult());
 }
 
 }  // namespace
diff --git a/blimp/client/session/tab_control_feature.cc b/blimp/client/session/tab_control_feature.cc
index 33c4524..b7b23cd 100644
--- a/blimp/client/session/tab_control_feature.cc
+++ b/blimp/client/session/tab_control_feature.cc
@@ -8,6 +8,7 @@
 #include "blimp/common/proto/blimp_message.pb.h"
 #include "blimp/common/proto/tab_control.pb.h"
 #include "blimp/net/blimp_message_processor.h"
+#include "net/base/net_errors.h"
 
 namespace blimp {
 namespace client {
@@ -61,6 +62,9 @@ void TabControlFeature::CloseTab(int tab_id) {
 void TabControlFeature::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+  callback.Run(net::OK);
+
   NOTIMPLEMENTED();
 }
 
diff --git a/blimp/engine/browser/blimp_engine_session.cc b/blimp/engine/browser/blimp_engine_session.cc
index 6e5f794..d541e89 100644
--- a/blimp/engine/browser/blimp_engine_session.cc
+++ b/blimp/engine/browser/blimp_engine_session.cc
@@ -353,14 +353,16 @@ void BlimpEngineSession::OnCompositorMessageReceived(
 void BlimpEngineSession::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
   DCHECK(message->type() == BlimpMessage::TAB_CONTROL ||
          message->type() == BlimpMessage::NAVIGATION);
 
-  bool result = true;
+  net::Error result = net::OK;
   if (message->type() == BlimpMessage::TAB_CONTROL) {
     switch (message->tab_control().type()) {
       case TabControlMessage::CREATE_TAB:
-        result = CreateWebContents(message->target_tab_id());
+        if (!CreateWebContents(message->target_tab_id()))
+          result = net::ERR_FAILED;
         break;
       case TabControlMessage::CLOSE_TAB:
         CloseWebContents(message->target_tab_id());
@@ -371,6 +373,7 @@ void BlimpEngineSession::ProcessMessage(
         break;
       default:
         NOTIMPLEMENTED();
+        result = net::ERR_NOT_IMPLEMENTED;
     }
   } else if (message->type() == BlimpMessage::NAVIGATION && web_contents_) {
     switch (message->navigation().type()) {
@@ -389,14 +392,14 @@ void BlimpEngineSession::ProcessMessage(
         break;
       default:
         NOTIMPLEMENTED();
+        result = net::ERR_NOT_IMPLEMENTED;
     }
   } else {
-    result = false;
+    DVLOG(1) << "No WebContents for navigation control";
+    result = net::ERR_FAILED;
   }
 
-  if (!callback.is_null()) {
-    callback.Run(result ? net::OK : net::ERR_FAILED);
-  }
+  callback.Run(result);
 }
 
 void BlimpEngineSession::AddNewContents(content::WebContents* source,
diff --git a/blimp/engine/browser/engine_render_widget_feature.cc b/blimp/engine/browser/engine_render_widget_feature.cc
index fe68ae6..35bead3 100644
--- a/blimp/engine/browser/engine_render_widget_feature.cc
+++ b/blimp/engine/browser/engine_render_widget_feature.cc
@@ -85,6 +85,7 @@ void EngineRenderWidgetFeature::RemoveDelegate(const int tab_id) {
 void EngineRenderWidgetFeature::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
   DCHECK(message->type() == BlimpMessage::RENDER_WIDGET ||
          message->type() == BlimpMessage::INPUT ||
          message->type() == BlimpMessage::COMPOSITOR);
@@ -118,9 +119,7 @@ void EngineRenderWidgetFeature::ProcessMessage(
       NOTREACHED();
   }
 
-  if (!callback.is_null()) {
-    callback.Run(net::OK);
-  }
+  callback.Run(net::OK);
 }
 
 EngineRenderWidgetFeature::RenderWidgetMessageDelegate*
diff --git a/blimp/engine/browser/engine_render_widget_feature_unittest.cc b/blimp/engine/browser/engine_render_widget_feature_unittest.cc
index 9eba4c8..6433d8a 100644
--- a/blimp/engine/browser/engine_render_widget_feature_unittest.cc
+++ b/blimp/engine/browser/engine_render_widget_feature_unittest.cc
@@ -14,6 +14,7 @@
 #include "blimp/net/input_message_generator.h"
 #include "blimp/net/test_common.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
@@ -82,8 +83,9 @@ void SendInputMessage(BlimpMessageProcessor* processor,
   message->set_target_tab_id(tab_id);
   message->mutable_input()->set_render_widget_id(rw_id);
 
-  processor->ProcessMessage(std::move(message),
-                            net::CompletionCallback());
+  net::TestCompletionCallback cb;
+  processor->ProcessMessage(std::move(message), cb.callback());
+  EXPECT_EQ(net::OK, cb.WaitForResult());
 }
 
 void SendCompositorMessage(BlimpMessageProcessor* processor,
@@ -94,8 +96,9 @@ void SendCompositorMessage(BlimpMessageProcessor* processor,
   scoped_ptr<BlimpMessage> message = CreateBlimpMessage(&details, tab_id);
   details->set_render_widget_id(rw_id);
   details->set_payload(payload.data(), base::checked_cast<int>(payload.size()));
-  processor->ProcessMessage(std::move(message),
-                            net::CompletionCallback());
+  net::TestCompletionCallback cb;
+  processor->ProcessMessage(std::move(message), cb.callback());
+  EXPECT_EQ(net::OK, cb.WaitForResult());
 }
 
 }  // namespace
diff --git a/blimp/net/blimp_message_demultiplexer.cc b/blimp/net/blimp_message_demultiplexer.cc
index 1444c9fd..8c69702 100644
--- a/blimp/net/blimp_message_demultiplexer.cc
+++ b/blimp/net/blimp_message_demultiplexer.cc
@@ -29,6 +29,7 @@ void BlimpMessageDemultiplexer::AddProcessor(BlimpMessage::Type type,
 void BlimpMessageDemultiplexer::ProcessMessage(
     scoped_ptr<BlimpMessage> message,
     const net::CompletionCallback& callback) {
+  DVLOG(2) << "ProcessMessage : " << *message;
   auto receiver_iter = feature_receiver_map_.find(message->type());
   if (receiver_iter == feature_receiver_map_.end()) {
     DLOG(ERROR) << "No registered receiver for " << *message << ".";
diff --git a/blimp/net/blimp_message_pump.cc b/blimp/net/blimp_message_pump.cc
index ae264e1..c3d6380 100644
--- a/blimp/net/blimp_message_pump.cc
+++ b/blimp/net/blimp_message_pump.cc
@@ -59,6 +59,7 @@ void BlimpMessagePump::OnReadPacketComplete(int result) {
   if (result == net::OK) {
     scoped_ptr<BlimpMessage> message(new BlimpMessage);
     if (message->ParseFromArray(buffer_->StartOfBuffer(), buffer_->offset())) {
+      DVLOG(2) << "OnReadPacketComplete, result=" << *message;
       processor_->ProcessMessage(std::move(message),
                                  process_msg_callback_.callback());
     } else {