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"
@@ skipping 158 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)) {
-    VLOG(2) << "Received PING.";
-    SendKeepAliveMessage(pong_message_, kHeartbeatPongType);
-  } else if (NestedPayloadTypeEquals(kHeartbeatPongType, message)) {
-    VLOG(2) << "Received PONG.";
+    if (NestedPayloadTypeEquals(kHeartbeatPingType, message)) {
+      VLOG(2) << "Received PING.";
+      SendKeepAliveMessage(pong_message_, kHeartbeatPongType);
+      return;
+    } else if (NestedPayloadTypeEquals(kHeartbeatPongType, message)) {
+      VLOG(2) << "Received PONG.";
+      return;
+    }
   } else {
-    // PING and PONG messages are intentionally suppressed from layers above.
-    inner_delegate_->OnMessage(message);
+    if (NestedPayloadTypeEquals(kHeartbeatPingType, message) ||
+        NestedPayloadTypeEquals(kHeartbeatPongType, message)) {
+      VLOG(2) << "Received PING or PONG before starting or after stopping.";
+      return;
+    }
   }
+
+  // PING and PONG messages are intentionally suppressed from layers above.

[Wez, 2017/01/04 00:10:17]

nit: This comment is not very clear, especially after the rewrite - perhaps
rephrase to "If we reach here then the message is neither PING nor PONG, so pass
it on for handling by the layer above."

IMO it'd be more straightforward to read wrt the suppression of ping/pongs if
you have the top-level conditionals be on the payload type, and then the
internal behaviour conditional on |started_|, though I appreciate that that
means checking |started_| in two distinct cases.

[miu, 2017/01/12 22:31:27]

Done.

Looking at this code again, I also couldn't help myself: The multiple calls to
NestedPayloadTypeEquals() were very wasteful! Each does lots of string copying
and JSON parsing and then throws away all that work after doing a simple string
equals. So, I changed it to a function that just does the parsing once and
extracts the needed field, and now we can compare it multiple times cheaply.

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

Nice :)

+  inner_delegate_->OnMessage(message);
 }
 
 void KeepAliveDelegate::Stop() {
   if (started_) {
     started_ = false;
     ping_timer_->Stop();
     liveness_timer_->Stop();
   }
 }
 
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions