Destroy all rtc peer connection handlers before shutting down blink

Destroy all rtc peer connection handlers before shutting down blink

The connection handlers will otherwise continue to call into blink which
is bound to crash after it was shut down.

BUG=337572
R=perkj@chromium.org,adamk@chromium.org

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

[adamk, 2014-05-20 11:00:15]

This seems super-weird, since there are still OwnPtrs referencing these objects
on the Blink side. How are we guaranteed that we won't crash due to
double-frees?

It seems like it would be safer if we could somehow mark these as "dead" without
affecting the actual object lifetimes.

[adamk, 2014-05-20 11:16:23]

On 2014/05/20 11:00:15, adamk wrote:
> This seems super-weird, since there are still OwnPtrs referencing these
objects
> on the Blink side. How are we guaranteed that we won't crash due to
> double-frees?
> 
> It seems like it would be safer if we could somehow mark these as "dead"
without
> affecting the actual object lifetimes.

Alternatively, we could add something to blink::WebRTCPeerConnectionClient so
that the handler can ask the WebCore::RTCPeerConnection to release its pointer.

[jochen (gone - plz use gerrit), 2014-05-20 12:21:30]

On 2014/05/20 11:16:23, adamk wrote:
> On 2014/05/20 11:00:15, adamk wrote:
> > This seems super-weird, since there are still OwnPtrs referencing these
> objects
> > on the Blink side. How are we guaranteed that we won't crash due to
> > double-frees?
> > 
> > It seems like it would be safer if we could somehow mark these as "dead"
> without
> > affecting the actual object lifetimes.
> 
> Alternatively, we could add something to blink::WebRTCPeerConnectionClient so
> that the handler can ask the WebCore::RTCPeerConnection to release its
pointer.

will do that

[perkj_chrome, 2014-05-22 06:45:25]

Thanks a lot for doing this.

https://codereview.chromium.org/292163002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/292163002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:569:
RTCPeerConnectionHandler::DestructAllHandlers();
This should be done before |media_stream_factory_| is reset within the #if
defined(ENABLE_WEBRTC), see content_renderer.gypi rtc_peer_connection_handler.cc
. (And it looks like I should rename |media_stream_factory_| to
|peer_connection_factory_| here as well.

[jochen (gone - plz use gerrit), 2014-05-22 07:53:44]

https://codereview.chromium.org/292163002/diff/20001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/292163002/diff/20001/content/renderer/render_...
content/renderer/render_thread_impl.cc:569:
RTCPeerConnectionHandler::DestructAllHandlers();
On 2014/05/22 06:45:26, perkj wrote:
> This should be done before |media_stream_factory_| is reset within the #if
> defined(ENABLE_WEBRTC), see content_renderer.gypi
rtc_peer_connection_handler.cc
> . (And it looks like I should rename |media_stream_factory_| to
> |peer_connection_factory_| here as well.  

Done.

[perkj_chrome, 2014-05-22 07:59:55]

lgtm

[adamk, 2014-05-22 08:03:22]

lgtm

[jochen (gone - plz use gerrit), 2014-05-23 09:08:43]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-05-23 09:09:39]

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

[commit-bot: I haz the power, 2014-05-23 12:18:45]

Change committed as 272504