Fix cast_channel::KeepAliveDelegate DCHECK failure.

extensions/browser/api/cast_channel/keep_alive_delegate.cc

 // Copyright 2015 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 "extensions/browser/api/cast_channel/keep_alive_delegate.h"
 
 #include <string>
 #include <utility>
 
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
-#include "extensions/browser/api/cast_channel/cast_message_util.h"
 #include "extensions/browser/api/cast_channel/cast_socket.h"
 #include "extensions/browser/api/cast_channel/logger.h"
 #include "extensions/common/api/cast_channel/cast_channel.pb.h"
 #include "extensions/common/api/cast_channel/logging.pb.h"
 #include "net/base/net_errors.h"
 
 namespace extensions {
 namespace api {
 namespace cast_channel {
 namespace {
 
 const char kHeartbeatNamespace[] = "urn:x-cast:com.google.cast.tp.heartbeat";
 const char kPingSenderId[] = "chrome";
 const char kPingReceiverId[] = "receiver-0";
 const char kTypeNodeId[] = "type";
 
-// Determines if the JSON-encoded payload is equivalent to
-// { "type": |chk_type| }
-bool NestedPayloadTypeEquals(const std::string& chk_type,
-                             const CastMessage& message) {
-  MessageInfo message_info;
-  CastMessageToMessageInfo(message, &message_info);
-  std::string type_json;
-  if (!message_info.data->GetAsString(&type_json)) {
-    return false;
-  }
-  std::unique_ptr<base::Value> type_value(base::JSONReader::Read(type_json));
-  if (!type_value.get()) {
-    return false;
-  }
-
-  base::DictionaryValue* type_dict;
-  if (!type_value->GetAsDictionary(&type_dict)) {
-    return false;
-  }
-
+// Parses the JSON-encoded payload of |message| and returns the value in the
+// "type" field or the empty string if the parse fails or the field is not
+// found.

[Wez, 2017/01/13 22:37:11]

Sorry, I completely missed that this was parsing JSON - is JSONReader::Read()
safe to use in the browser process?

[miu, 2017/01/15 02:17:08]

I'm not sure. It was already doing that. I'll mention this to folks I know are
maintaining this.

+std::string ParseForPayloadType(const CastMessage& message) {
+  std::unique_ptr<base::Value> parsed_payload(
+      base::JSONReader::Read(message.payload_utf8()));
+  base::DictionaryValue* payload_as_dict;
+  if (!parsed_payload || !parsed_payload->GetAsDictionary(&payload_as_dict))
+    return std::string();
   std::string type_string;
-  return (type_dict->HasKey(kTypeNodeId) &&
-          type_dict->GetString(kTypeNodeId, &type_string) &&
-          type_string == chk_type);
+  if (!payload_as_dict->GetString(kTypeNodeId, &type_string))
+    return std::string();
+  return type_string;
 }
 
 }  // namespace
 
 // static
 const char KeepAliveDelegate::kHeartbeatPingType[] = "PING";
 
 // static
 const char KeepAliveDelegate::kHeartbeatPongType[] = "PONG";
 
@@ skipping 106 matching lines @@
 
 // CastTransport::Delegate interface.
 void KeepAliveDelegate::OnError(ChannelError error_state) {
   DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "KeepAlive::OnError: " << error_state;
   inner_delegate_->OnError(error_state);
   Stop();
 }
 
 void KeepAliveDelegate::OnMessage(const CastMessage& message) {
-  DCHECK(started_);
   DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(2) << "KeepAlive::OnMessage : " << message.payload_utf8();
 
-  ResetTimers();
+  if (started_)
+    ResetTimers();
 
-  if (NestedPayloadTypeEquals(kHeartbeatPingType, message)) {
+  // PING and PONG messages are intercepted and handled by KeepAliveDelegate
+  // here. All other messages are passed through to |inner_delegate_|.
+  const std::string payload_type = ParseForPayloadType(message);
+  if (payload_type == kHeartbeatPingType) {
     VLOG(2) << "Received PING.";
-    SendKeepAliveMessage(pong_message_, kHeartbeatPongType);
-  } else if (NestedPayloadTypeEquals(kHeartbeatPongType, message)) {
+    if (started_)
+      SendKeepAliveMessage(pong_message_, kHeartbeatPongType);
+    return;
+  } else if (payload_type == kHeartbeatPongType) {
     VLOG(2) << "Received PONG.";
+    return;

[Wez, 2017/01/13 22:37:11]

nit: Either remove the return's here, since they're unnecessary given the
else's, or move the OnMessage callback back out of an else block, change this
block to a plain if, and keep the return's as early-exits.

[miu, 2017/01/15 02:17:08]

Done. That was an oops on my part (left-over from prior patch set, I meant to
remove them since they are redundant now).

   } else {
-    // PING and PONG messages are intentionally suppressed from layers above.
     inner_delegate_->OnMessage(message);
   }
 }
 
 void KeepAliveDelegate::Stop() {
   if (started_) {
     started_ = false;
     ping_timer_->Stop();
     liveness_timer_->Stop();
   }
 }
 
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions