[Cast Channel] Fix "leaky" CastChannelOpenFunction.

extensions/browser/api/cast_channel/cast_channel_api.cc

Index: extensions/browser/api/cast_channel/cast_channel_api.cc
diff --git a/extensions/browser/api/cast_channel/cast_channel_api.cc b/extensions/browser/api/cast_channel/cast_channel_api.cc
index 4355e0e418463847d6305a24e9cb592eff2b41a9..690c1972ae9072093e7b481e76db878e5eb25105 100644
--- a/extensions/browser/api/cast_channel/cast_channel_api.cc
+++ b/extensions/browser/api/cast_channel/cast_channel_api.cc
@@ -346,12 +346,14 @@ void CastChannelOpenFunction::AsyncWorkStart() {
 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) {
-    SetResultFromError(new_channel_id_, result);
-  } else {
+  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.

   }
+
   AsyncWorkCompleted();
 }