Bug 1147857 - Be careful about WebRTC stats query creation. r=jib, a=lmandel
authorRandell Jesup <rjesup@jesup.org>
Fri, 27 Mar 2015 15:52:56 -0400
changeset 258221 2c5d97fcb993
parent 258220 d7bbef9132a4
child 258222 108255910fbf
push id4623
push userryanvm@gmail.com
push date2015-04-03 01:49 +0000
treeherdermozilla-beta@9c755cdc241c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjib, lmandel
bugs1147857
milestone38.0
Bug 1147857 - Be careful about WebRTC stats query creation. r=jib, a=lmandel
media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ -284,24 +284,30 @@ PeerConnectionCtx::EverySecondTelemetryC
   }
   MOZ_ASSERT(stsThread);
 
   nsAutoPtr<RTCStatsQueries> queries(new RTCStatsQueries);
   for (auto p = ctx->mPeerConnections.begin();
         p != ctx->mPeerConnections.end(); ++p) {
     if (p->second->HasMedia()) {
       queries->append(nsAutoPtr<RTCStatsQuery>(new RTCStatsQuery(true)));
-      p->second->BuildStatsQuery_m(nullptr, // all tracks
-                                   queries->back());
+      if (NS_WARN_IF(NS_FAILED(p->second->BuildStatsQuery_m(nullptr, // all tracks
+                                                            queries->back())))) {
+        queries->popBack();
+      } else {
+        MOZ_ASSERT(queries->back()->report);
+      }
     }
   }
-  rv = RUN_ON_THREAD(stsThread,
-                     WrapRunnableNM(&EverySecondTelemetryCallback_s, queries),
-                     NS_DISPATCH_NORMAL);
-  NS_ENSURE_SUCCESS_VOID(rv);
+  if (!queries->empty()) {
+    rv = RUN_ON_THREAD(stsThread,
+                       WrapRunnableNM(&EverySecondTelemetryCallback_s, queries),
+                       NS_DISPATCH_NORMAL);
+    NS_ENSURE_SUCCESS_VOID(rv);
+  }
 }
 #endif
 
 nsresult PeerConnectionCtx::Initialize() {
   initGMP();
 
 #ifdef MOZILLA_INTERNAL_API
   mConnectionCounter = 0;
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -2792,27 +2792,32 @@ PeerConnectionImpl::EndOfLocalCandidates
 
 #ifdef MOZILLA_INTERNAL_API
 nsresult
 PeerConnectionImpl::BuildStatsQuery_m(
     mozilla::dom::MediaStreamTrack *aSelector,
     RTCStatsQuery *query) {
 
   if (!HasMedia()) {
-    return NS_OK;
+    return NS_ERROR_UNEXPECTED;
   }
 
-  if (!mMedia->ice_ctx() || !mThread) {
-    CSFLogError(logTag, "Could not build stats query, critical components of "
-                        "PeerConnectionImpl not set.");
+  // Note: mMedia->ice_ctx() is deleted on STS thread; so make sure we grab and hold
+  // a ref instead of making multiple calls.  NrIceCtx uses threadsafe refcounting.
+  query->iceCtx = mMedia->ice_ctx();
+  if (!query->iceCtx) {
+    CSFLogError(logTag, "Could not build stats query, no ice_ctx");
+    return NS_ERROR_UNEXPECTED;
+  }
+  if (!mThread) {
+    CSFLogError(logTag, "Could not build stats query, no MainThread");
     return NS_ERROR_UNEXPECTED;
   }
 
   nsresult rv = GetTimeSinceEpoch(&(query->now));
-
   if (NS_FAILED(rv)) {
     CSFLogError(logTag, "Could not build stats query, could not get timestamp");
     return rv;
   }
 
   // We do not use the pcHandle here, since that's risky to expose to content.
   query->report = new RTCStatsReportInternalConstruct(
       NS_ConvertASCIItoUTF16(mName.c_str()),
@@ -2869,18 +2874,16 @@ PeerConnectionImpl::BuildStatsQuery_m(
       }
     } else {
       for (auto it = pipelines.begin(); it != pipelines.end(); ++it) {
         query->pipelines.AppendElement(it->second);
       }
     }
   }
 
-  query->iceCtx = mMedia->ice_ctx();
-
   // From the list of MediaPipelines, determine the set of NrIceMediaStreams
   // we are interested in.
   std::set<size_t> levelsToGrab;
   if (aSelector) {
     for (size_t p = 0; p < query->pipelines.Length(); ++p) {
       size_t level = query->pipelines[p]->level();
       levelsToGrab.insert(level);
     }