Fix leak of RTCPeerConnectionHandler if PeerConnection.close() is called from js.

Fix leak of RTCPeerConnectionHandler if PeerConnection.close() is called from js.
This fixes a bug where RTCPeerConnectionHandler::client_ is set to null when RTCPeerConnectionHandler.stop() is called.
RTCPeerConnectionHandler.stop() is a pretty bad name (override from blink::WebRTCPeerConnectionHandler) since it is triggered when JS or the browser process want to close a PeerConnection.
Since client_ was set to null, RTCPeerConnectionHandler::DestructAllHandlers did not delete RTCPeerConnectionHandler.

BUG=542132
Committed: https://crrev.com/9596af111d9e82b0e27c9fab01fa688515623b1f
Cr-Commit-Position: refs/heads/master@{#359334}

[perkj_chrome, 2015-11-12 09:57:06]

perkj@chromium.org changed reviewers:
+ tommi@chromium.org

[perkj_chrome, 2015-11-12 09:57:07]

Can you please take a look?

[tommi (sloooow) - chröme, 2015-11-12 14:53:19]

lgtm with a few nits

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:822:
std::set<RTCPeerConnectionHandler*> handlers(
do we actually need this copy?  Could this simply be a reference instead?
e.g.

std::set<RTCPeerConnectionHandler*>& handlers =
    g_peer_connection_handlers.Get();

or even do:

for (auto* handler : g_peer_connection_handlers.Get())
  handler->client_->releasePeerConnectionHandler();

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:825: for (auto handler :
handlers) {
nit: could make it explicit that handler is always a pointer (never copy
by-value)
nit: also remove braces since it's only a single line now

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1393: is_closed_ = true;
now that client_ is never set to nullptr, can we make it const?

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1441: if(!is_closed_)
nit: add space

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.h (right):

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.h:233: // After
PeerConnection has been closed, this object may no longer forward
nit: After the PeerConnection has been ...

[perkj_chrome, 2015-11-12 15:48:38]

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:822:
std::set<RTCPeerConnectionHandler*> handlers(
On 2015/11/12 14:53:19, tommi wrote:
> do we actually need this copy?  Could this simply be a reference instead?
> e.g.
> 
> std::set<RTCPeerConnectionHandler*>& handlers =
>     g_peer_connection_handlers.Get();
> 
> or even do:
> 
> for (auto* handler : g_peer_connection_handlers.Get())
>   handler->client_->releasePeerConnectionHandler();

needs copy since  g_peer_connection_handlers is modified, ie, the item is
removed from g_peer_connection_handlers as a consequence of calling
releasePeerConnectionHandler.

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:825: for (auto handler :
handlers) {
On 2015/11/12 14:53:18, tommi wrote:
> nit: could make it explicit that handler is always a pointer (never copy
> by-value)
> nit: also remove braces since it's only a single line now

Done.

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1393: is_closed_ = true;
On 2015/11/12 14:53:19, tommi wrote:
> now that client_ is never set to nullptr, can we make it const?

Done.

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1441: if(!is_closed_)
On 2015/11/12 14:53:18, tommi wrote:
> nit: add space

Done.

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.h (right):

https://codereview.chromium.org/1438153002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.h:233: // After
PeerConnection has been closed, this object may no longer forward
On 2015/11/12 14:53:19, tommi wrote:
> nit: After the PeerConnection has been ...

Done.

[perkj_chrome, 2015-11-12 15:49:02]

The CQ bit was checked by perkj@chromium.org

[perkj_chrome, 2015-11-12 15:49:03]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1438153002/#ps40001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-11-12 15:49:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1438153002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1438153002/40001

[commit-bot: I haz the power, 2015-11-12 17:08:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-12 20:08:03]

Patchset 3 (id:??) landed as
https://crrev.com/9596af111d9e82b0e27c9fab01fa688515623b1f
Cr-Commit-Position: refs/heads/master@{#359334}

[perkj_chrome, 2015-11-13 16:19:10]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1442063002/ by perkj@chromium.org.

The reason for reverting is: Blamed for causing/surfacing a worse bug.
Tentatively reverting.

crbug/555575.

[Taylor_Brandstetter, 2015-11-13 18:45:28]

deadbeef@chromium.org changed reviewers:
+ deadbeef@chromium.org

[Taylor_Brandstetter, 2015-11-13 18:45:29]

https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1385: if (!is_closed_ ||
!native_peer_connection_.get())
Should be "is_closed", not "!is_closed". I think this is the source of the
clusterfuzz crashes. The PC is never actually being closed because of this, and
so observer objects are being dereferenced after being destroyed.

[tommi (sloooow) - chröme, 2015-11-13 18:57:23]

https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:1385: if (!is_closed_ ||
!native_peer_connection_.get())
On 2015/11/13 18:45:29, Taylor_Brandstetter wrote:
> Should be "is_closed", not "!is_closed". I think this is the source of the
> clusterfuzz crashes. The PC is never actually being closed because of this,
and
> so observer objects are being dereferenced after being destroyed.

well spotted! :)

[perkj_chrome, 2015-11-15 20:18:20]

On 2015/11/13 18:57:23, tommi wrote:
>
https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
> File content/renderer/media/rtc_peer_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/1438153002/diff/40001/content/renderer/media/...
> content/renderer/media/rtc_peer_connection_handler.cc:1385: if (!is_closed_ ||
> !native_peer_connection_.get())
> On 2015/11/13 18:45:29, Taylor_Brandstetter wrote:
> > Should be "is_closed", not "!is_closed". I think this is the source of the
> > clusterfuzz crashes. The PC is never actually being closed because of this,
> and
> > so observer objects are being dereferenced after being destroyed.
> 
> well spotted! :)

and embarrassing for me. But yes, well spotted.