[Cast Channel] Fix "leaky" CastChannelOpenFunction.

[Cast Channel] Fix "leaky" CastChannelOpenFunction.

This patch ensures the callback passed into the CastSocket via
Connect() is always invoked. This ensures CastChannelOpenFunction
always responds before it is destroyed.

BUG=693333
Review-Url: https://codereview.chromium.org/2707543002
Cr-Commit-Position: refs/heads/master@{#451973}
Committed: https://chromium.googlesource.com/chromium/src/+/ec28bf711c7ebeceed2271b2e7836d45b24704d6

[imcheng, 2017-02-17 22:22:17]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 22:24:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[imcheng, 2017-02-17 22:34:44]

imcheng@chromium.org changed reviewers:
+ mfoltz@chromium.org

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

PTAL

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:349: if (result ==
cast_channel::CHANNEL_ERROR_NONE) {
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?

[commit-bot: I haz the power, 2017-02-18 00:02:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 00:02:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

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

LGTM

Thanks for cleaning this up.  Is the send() function also affected?

The CastSocket objects are owned by extensions::ApiResourceManager.  Based on
the bug report it sounds like the ApiResourceManager is destroyed before pending
extension functions on extension unload.

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:349: if (result ==
cast_channel::CHANNEL_ERROR_NONE) {
On 2017/02/17 at 22:34:45, imcheng wrote:
> 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).

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.
 
> 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?

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.

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:351: CHECK(socket);
SECURITY_CHECK?

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:74: // will be invoked with
CHANNEL_ERROR_UNKNOWN. In this case, invoking
CHANNEL_ERROR_SOCKET_ERROR is used elsewhere for socket-level issues IIRC.

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:75: // |callback| must not
result in an re-entrancy behavior.
s/an/any/

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:329: // TODO(imcheng): Change
to OnceCallback? OnceCallback is move-only which
I think the benefit is pretty slight here, not sure it's worth a TODO. 
Personally, I'll probably use the new callbacks for new code but not try to
retrofit existing code.

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

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:354:
SetResultFromError(new_channel_id_, result);
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:15:58]

On 2017/02/21 18:30:01, mark a. foltz wrote:
> LGTM
> 
> Thanks for cleaning this up.  Is the send() function also affected?
> 

send() is not affected -- it already has this invoke-callback-in-dtor behavior
this patch is introducing for open().

> The CastSocket objects are owned by extensions::ApiResourceManager.  Based on
> the bug report it sounds like the ApiResourceManager is destroyed before
pending
> extension functions on extension unload.

Yep. There is a circular dependency ApiResourceManager -> CastSocket ->
CastChannel*Function -> ApiResourceManager. When extension is unloaded,
CastSocket is destroyed by removing from ApiResourceManager.



> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> File extensions/browser/api/cast_channel/cast_channel_api.cc (right):
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> extensions/browser/api/cast_channel/cast_channel_api.cc:349: if (result ==
> cast_channel::CHANNEL_ERROR_NONE) {
> On 2017/02/17 at 22:34:45, imcheng wrote:
> > 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).
> 
> 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.
>  
> > 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?
> 
> 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.
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> extensions/browser/api/cast_channel/cast_channel_api.cc:351: CHECK(socket);
> SECURITY_CHECK?
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> File extensions/browser/api/cast_channel/cast_socket.h (right):
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> extensions/browser/api/cast_channel/cast_socket.h:74: // will be invoked with
> CHANNEL_ERROR_UNKNOWN. In this case, invoking
> CHANNEL_ERROR_SOCKET_ERROR is used elsewhere for socket-level issues IIRC.
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> extensions/browser/api/cast_channel/cast_socket.h:75: // |callback| must not
> result in an re-entrancy behavior.
> s/an/any/
> 
>
https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
> extensions/browser/api/cast_channel/cast_socket.h:329: // TODO(imcheng):
Change
> to OnceCallback? OnceCallback is move-only which
> I think the benefit is pretty slight here, not sure it's worth a TODO. 
> Personally, I'll probably use the new callbacks for new code but not try to
> retrofit existing code.

[imcheng, 2017-02-21 22:55:27]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[imcheng, 2017-02-21 22:55:48]

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:349: if (result ==
cast_channel::CHANNEL_ERROR_NONE) {
On 2017/02/21 18:30:01, mark a. foltz wrote:
> On 2017/02/17 at 22:34:45, imcheng wrote:
> > 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).
> 
> 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.
>  

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



> > 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?
> 
> 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.
> 
> 
> 
> 

ACK

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:351: CHECK(socket);
On 2017/02/21 18:30:01, mark a. foltz wrote:
> SECURITY_CHECK?

Done.

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:354:
SetResultFromError(new_channel_id_, result);
On 2017/02/21 18:33:20, mark a. foltz wrote:
> If you want to aggressively destroy sockets on failure to open, then add calls
> to 
> 
> RemoveSocket()
> 
> and
> 
> api_->GetLogger()->ClearLastErrors(channel_id)
> 
> here.

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

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_socket.h (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:74: // will be invoked with
CHANNEL_ERROR_UNKNOWN. In this case, invoking
On 2017/02/21 18:30:01, mark a. foltz wrote:
> CHANNEL_ERROR_SOCKET_ERROR is used elsewhere for socket-level issues IIRC.

socket_error is used for errors with read/write socket operations. I chose to
use unknown here since it is an edge case and it is unlikely the extension will
receive it anyway (since it is being unloaded).

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:75: // |callback| must not
result in an re-entrancy behavior.
On 2017/02/21 18:30:01, mark a. foltz wrote:
> s/an/any/

Done.

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_socket.h:329: // TODO(imcheng): Change
to OnceCallback? OnceCallback is move-only which
On 2017/02/21 18:30:01, mark a. foltz wrote:
> I think the benefit is pretty slight here, not sure it's worth a TODO. 
> Personally, I'll probably use the new callbacks for new code but not try to
> retrofit existing code.

Ok, removed TODO.

[commit-bot: I haz the power, 2017-02-21 22:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:18:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:18:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

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

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_channel_api.cc (right):

https://codereview.chromium.org/2707543002/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_channel_api.cc:351: CHECK(socket);
On 2017/02/21 22:55:47, imcheng wrote:
> On 2017/02/21 18:30:01, mark a. foltz wrote:
> > SECURITY_CHECK?
> 
> Done.

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

[imcheng, 2017-02-21 23:34:37]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 23:37:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 00:52:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 00:52:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[imcheng, 2017-02-22 01:42:06]

The CQ bit was checked by imcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 01:43:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 03:46:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:46:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[imcheng, 2017-02-22 04:18:44]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2017-02-22 04:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2707543002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2017-02-22 04:20:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 07:10:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 07:10:34]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[imcheng, 2017-02-22 08:33:00]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2017-02-22 08:34:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 10:16:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487752380220930,
"parent_rev": "d1f3fe30040eca73500617f2a401ef2299e2f87f", "commit_rev":
"ec28bf711c7ebeceed2271b2e7836d45b24704d6"}

[commit-bot: I haz the power, 2017-02-22 10:17:12]

Description was changed from

==========
[Cast Channel] Fix "leaky" CastChannelOpenFunction.

This patch ensures the callback passed into the CastSocket via
Connect() is always invoked. This ensures CastChannelOpenFunction
always responds before it is destroyed.

BUG=693333
==========

to

==========
[Cast Channel] Fix "leaky" CastChannelOpenFunction.

This patch ensures the callback passed into the CastSocket via
Connect() is always invoked. This ensures CastChannelOpenFunction
always responds before it is destroyed.

BUG=693333

Review-Url: https://codereview.chromium.org/2707543002
Cr-Commit-Position: refs/heads/master@{#451973}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec28bf711c7ebeceed2271b2e783...
==========

[commit-bot: I haz the power, 2017-02-22 10:17:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ec28bf711c7ebeceed2271b2e783...