[Cast Channel] Fix "leaky" CastChannelOpenFunction.

extensions/browser/api/cast_channel/cast_channel_api.cc

 // Copyright 2014 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/cast_channel_api.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 328 matching lines @@
     delegate.reset(keep_alive);
   }
 
   socket->Connect(std::move(delegate),
                   base::Bind(&CastChannelOpenFunction::OnOpen, this));
 }
 
 void CastChannelOpenFunction::OnOpen(cast_channel::ChannelError result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   VLOG(1) << "Connect finished, OnOpen invoked.";
-  CastSocket* socket = GetSocket(new_channel_id_);
-  if (!socket) {
+  if (result == cast_channel::CHANNEL_ERROR_NONE) {

[imcheng, 2017/02/17 22:34:45]

I changed the behavior slightly. It's not clear to me if GetSocket will ever
returns nullptr here, except maybe the case when CastSocket is being destroyed
(which I avoid accessing it anyway so I don't have to deal with re-entrancy). 

As a side note, since this code does not clean up the CastSocket object on
failure to open, it is up to the extension to call close() to free up resources
(even though the channelInfo from the open() call is already CLOSED?). This
seems a bit fragile, but maybe there's a reason for this design that I am not
aware of?

[mark a. foltz, 2017/02/21 18:30:01]

IIRC, GetSocket should never be nullptr.  AddSocket() in AsyncWorkStart creates
and adds it to the ApiResourceManager, and it is only removed by calling the
close() function.
 
I honestly don't recall.  I think cleaning up sockets more aggressively is fine,
as long as the socket_id can be set in the callbacks and the error event so the
client can know which socket failed to open.

[imcheng, 2017/02/21 22:55:47]

Yeah. I reasoned that since CastSocket is always owned by ApiResourceManager
after calling AddSocket(), this should never return nullptr.
ACK

+    CastSocket* socket = GetSocket(new_channel_id_);
+    CHECK(socket);

[mark a. foltz, 2017/02/21 18:30:01]

SECURITY_CHECK?

[imcheng, 2017/02/21 22:55:47]

Done.

[imcheng, 2017/02/21 23:34:20]

Turns out SECURITY_CHECK is defined in Blink codebase. I reverted this to a
CHECK.

+    SetResultFromSocket(*socket);
+  } else {
     SetResultFromError(new_channel_id_, result);

[mark a. foltz, 2017/02/21 18:33:20]

If you want to aggressively destroy sockets on failure to open, then add calls
to 

RemoveSocket()

and

api_->GetLogger()->ClearLastErrors(channel_id)

here.

[imcheng, 2017/02/21 22:55:47]

I will save it for a separate patch as it's a riskier change. I added a NOTE
here.

-  } else {
-    SetResultFromSocket(*socket);
   }
+
   AsyncWorkCompleted();
 }
 
 CastChannelSendFunction::CastChannelSendFunction() { }
 
 CastChannelSendFunction::~CastChannelSendFunction() { }
 
 bool CastChannelSendFunction::Prepare() {
   params_ = Send::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params_.get());
@@ skipping 147 matching lines @@
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(ui_dispatch_cb_, socket_->owner_extension_id(),
                  base::Passed(std::move(event))));
 }
 
 void CastChannelOpenFunction::CastMessageHandler::Start() {
 }
 
 }  // namespace extensions