Return false if PeerConnection::GetStats() is called on invalid tracks

Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
Committed: https://crrev.com/e9e94c3fee79b0b590e2e0fd40f8c55398d8f548
Cr-Commit-Position: refs/heads/master@{#14934}

[Zhi Huang, 2016-11-02 21:32:24]

Description was changed from

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.

A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
==========

to

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
==========

[Taylor Brandstetter, 2016-11-03 22:47:00]

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

[Taylor Brandstetter, 2016-11-03 22:47:01]

Looks good, I think we just have to make sure that a null track isn't an error.

https://codereview.webrtc.org/2470023004/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2470023004/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:889: if (!stats_->IsValidTrack(track)) {
May be worth logging a warning here.

Also, you could add a comment explaining "The StatsCollector is used to tell if
a track is valid because it may remember tracks that the PeerConnection
previously removed." I didn't realize this at first.

Oh, and one more thing; a null track *should not* be an error. That should just
equate to "no track selector specified, gather all stats". So you could change
the condition to:

"if (track && !stats_->IsValidTrack(track))"

[Zhi Huang, 2016-11-03 23:10:09]

Patchset #2 (id:20001) has been deleted

[Zhi Huang, 2016-11-03 23:17:27]

Please take another look.

https://codereview.webrtc.org/2470023004/diff/1/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2470023004/diff/1/webrtc/api/peerconnection.cc#...
webrtc/api/peerconnection.cc:889: if (!stats_->IsValidTrack(track)) {
On 2016/11/03 22:47:01, Taylor Brandstetter wrote:
> May be worth logging a warning here.
> 
> Also, you could add a comment explaining "The StatsCollector is used to tell
if
> a track is valid because it may remember tracks that the PeerConnection
> previously removed." I didn't realize this at first.
> 
> Oh, and one more thing; a null track *should not* be an error. That should
just
> equate to "no track selector specified, gather all stats". So you could change
> the condition to:
> 
> "if (track && !stats_->IsValidTrack(track))"

I realized that the nullptr is not an error. The nullptr condition was inside
the IsValidTrack() before. I'll move it here.

Done.

[Taylor Brandstetter, 2016-11-04 00:48:48]

lgtm

https://codereview.webrtc.org/2470023004/diff/40001/webrtc/api/statscollector.cc
File webrtc/api/statscollector.cc (right):

https://codereview.webrtc.org/2470023004/diff/40001/webrtc/api/statscollector...
webrtc/api/statscollector.cc:554: StatsReport::kStatsReportTypeTrack,
track->id())) != nullptr;
nit: Since this only needs the track ID, the argument could just be "const
std::string& track_id". Then you wouldn't need to worry about checking if it's
null here.

[Zhi Huang, 2016-11-04 17:55:57]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-11-04 17:56:01]

The CQ bit was unchecked by zhihuang@webrtc.org

[Zhi Huang, 2016-11-04 18:13:27]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-11-04 18:13:27]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2470023004/#ps60001 (title:
"Change the function parameter.")

[commit-bot: I haz the power, 2016-11-04 18:13:38]

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

[commit-bot: I haz the power, 2016-11-04 18:38:18]

Description was changed from

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
==========

to

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
==========

[commit-bot: I haz the power, 2016-11-04 18:38:18]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-04 18:46:53]

Description was changed from

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652
==========

to

==========
Return false if PeerConnection::GetStats() is called on invalid tracks

Before calling StatsCollctor::GetStats() in PeerConnection::GetStats(), check if
the track is valid. If not, return false.
A track is invalid if it is not a nullptr and there is no report data for it.

BUG=webrtc:6652

Committed: https://crrev.com/e9e94c3fee79b0b590e2e0fd40f8c55398d8f548
Cr-Commit-Position: refs/heads/master@{#14934}
==========

[commit-bot: I haz the power, 2016-11-04 18:46:54]

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