Close all active PeerConnections upon OS suspend

Close all active PeerConnections upon OS suspend

BUG=403618
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290125

[vrk (LEFT CHROMIUM), 2014-08-14 01:22:54]

This uses PeerConnectionTracker/Host to handle communication between browser
process and RTCPeerConnectionHandler in addition to WebRTCInternals stuff. 

IMO this isn't hacky - even though PeerConnectionTracker/Host were added for
WebRTCInternals only, I don't think there's a reason to keep it
WebRTCInternals-only from a design perspective. Jiayang/Justin, lmk if you
disagree.

[jiayl, 2014-08-14 16:57:41]

LG overall. The only thing is that the blink state will not be updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So JS will not get the proper states and notifications.

It could be fixed by adding a method to  WebRTCPeerConnectionHandlerClient, to
close PC from the blink level.

[vrk (LEFT CHROMIUM), 2014-08-15 00:22:00]

On 2014/08/14 16:57:41, jiayl wrote:
> LG overall. The only thing is that the blink state will not be updated:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> So JS will not get the proper states and notifications.
> 
> It could be fixed by adding a method to  WebRTCPeerConnectionHandlerClient, to
> close PC from the blink level.

Thanks Jiayang! I created two follow-on CLs:
https://codereview.chromium.org/472163002/    Blink change to client
https://codereview.chromium.org/475263002/    Chrome change to use client

Do you want me to merge this CL into 475263002? Otherwise I was thinking I'd
land this first, then the two above.

[juberti2, 2014-08-15 00:33:06]

lgtm for me as well. I think it's important to update blink state, or else the
page will be surprised when its PCs are now unresponsive but not in the closed
state.

[jiayl, 2014-08-15 16:14:13]

lgtm.

[vrk (LEFT CHROMIUM), 2014-08-15 17:08:42]

+nasko security OWNERS content/common/media/peer_connection_tracker_messages.h

[nasko, 2014-08-15 19:14:19]

IPC LGTM and a couple of comments.

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/peer_connection_tracker_host.cc
(right):

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/peer_connection_tracker_host.cc:44: void
PeerConnectionTrackerHost::OnSuspend() {
nit: I'd move this below with the rest of the methods that deal with IPCs.

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/peer_connection_tracker_host.cc:47: if
(host)
This object is created and owned by RenderProcessHost, right? Is there a case
where host will be NULL? If not, it is better to use CHECK(host) to ensure this
is never violated.

[vrk (LEFT CHROMIUM), 2014-08-15 23:35:26]

Thanks for the reviews!

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/peer_connection_tracker_host.cc
(right):

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/peer_connection_tracker_host.cc:44: void
PeerConnectionTrackerHost::OnSuspend() {
On 2014/08/15 19:14:19, nasko wrote:
> nit: I'd move this below with the rest of the methods that deal with IPCs. 

Done.

https://codereview.chromium.org/465153003/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/peer_connection_tracker_host.cc:47: if
(host)
On 2014/08/15 19:14:18, nasko wrote:
> This object is created and owned by RenderProcessHost, right? Is there a case
> where host will be NULL? If not, it is better to use CHECK(host) to ensure
this
> is never violated.

Good point, Done.

[vrk (LEFT CHROMIUM), 2014-08-15 23:35:46]

The CQ bit was checked by vrk@chromium.org

[commit-bot: I haz the power, 2014-08-15 23:39:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vrk@chromium.org/465153003/60001

[commit-bot: I haz the power, 2014-08-16 08:05:25]

Committed patchset #3 (60001) as 290125

[scottmg, 2014-08-25 23:03:05]

A revert of this CL (patchset #3) has been created in
https://codereview.chromium.org/488693003/ by scottmg@chromium.org.

The reason for reverting is: Causing browser crashes due to race, BUG=404767.