1153056 - about:webrtc blanks whenever allocated PeerConnections goes to zero (REGRESSION)

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: fix about:webrtc to not blank on zero allocated PeerConnections

A recent crash-fix (Bug 1147857) causes about:webrtc to blank unless there is at least one created PeerConnection in the system. Since the crash-fix was uplifted to release, so is this regression. :-(

This may not sound like much of a bug, but about:webrtc importantly shows previously closed PeerConnections for post-mortem of calls, which is helpful in Hello where the conversation window is typically gone (and PeerConnections relinquished) after a call.

STR (non-e10s):
1. In tab 1, start a webRTC test-call e.g. http://jsfiddle.net/zy3e8rqo
2. In tab 2, open about:webrtc (shows OK)
3. Close tab 1 and refresh tab 2.

Expected result:
- about:webrtc should show the closed PeerConnections and the connection log.

Actual result:
- about:webrtc shows no PeerConnections or connection log.

Workaround:
- Open any new page with WebRTC on it, and refresh about:webrtc to see the PeerConnections again (including the closed ones, they're still there, they were just hidden) and the connection log.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8590583 [details] [diff] [review]
fix about:webrtc to not blank on zero allocated PeerConnections

Review of attachment 8590583 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I want a clean upliftable patch (unless I'm wrong in my assessment that the middle part of this patch is just cleanup/style).  Drop that or convince me there's a need for it and r+ (or write an aurora/beta uplift patch that's stripped down, and r+ for this one for inbound).

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +141,5 @@
>    nsresult rv;
>    nsCOMPtr<nsIEventTarget> stsThread =
>      do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>  
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

Good

@@ +157,5 @@
>    // If there is no PeerConnectionCtx, go through the same motions, since
>    // the API consumer doesn't care why there are no PeerConnectionImpl.
>    PeerConnectionCtx *ctx = GetPeerConnectionCtx();
>    if (ctx) {
> +    for (auto& p : ctx->mPeerConnections) {

Can we uplift this syntax to 38?  (Probably, but I forget the cutoff for c++11 support).  Also, it makes uplifts look scarier - for this sort of fix, let's KISS for safety and quality of review

@@ +172,3 @@
>            }
> +          MOZ_ASSERT(query->report);
> +          queries->append(query);

I understand why you want to do this, but isn't this functionally equivalent?  Since this needs uplift, let's not do general rewrite/cleanup unless it helps the goal.

@@ -177,5 @@
>        }
>      }
>    }
>  
> -  if (!queries->empty()) {

And here's the bug I presume

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: fix about:webrtc to not blank on zero allocated PeerConnections (minimized uplift version)

Sure thing. Sorry about that, just found it easier to reason about with some cleanup. Do we want to still land the cleanup on trunk?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: minor cleanup around stats queries

Broke out cleanup as follow-on patch.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8590824 [details] [diff] [review]
Part 1: fix about:webrtc to not blank on zero allocated PeerConnections (minimized uplift version)

Review of attachment 8590824 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +168,5 @@
>          if (p->second->HasMedia()) {
>            queries->append(nsAutoPtr<RTCStatsQuery>(new RTCStatsQuery(true)));
> +          rv = p->second->BuildStatsQuery_m(nullptr, queries->back()); // all tracks
> +          if (NS_WARN_IF(NS_FAILED(rv))) {
> +            aRv.Throw(rv);

ok, though this aborts the entire request because one failed, but that's ok here.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8590828 [details] [diff] [review]
Part 2: minor cleanup around stats queries

Review of attachment 8590828 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +186,5 @@
>                       WrapRunnableNM(&GetAllStats_s, callbackHandle, queries),
>                       NS_DISPATCH_NORMAL);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    aRv.Throw(rv);
> +  }

How does aRv get set in the success case?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #4)
> > +          rv = p->second->BuildStatsQuery_m(nullptr, queries->back()); // all tracks
> > +          if (NS_WARN_IF(NS_FAILED(rv))) {
> > +            aRv.Throw(rv);
> 
> ok, though this aborts the entire request because one failed, but that's ok
> here.

I agree, given that errors from BuildStatsQuery_m seem to be of the systemic kind. For instance, during shutdown, the "every second telemetry request" might lose the ice context half-way through building the request-list, and in the old code the partial-list request would then still be dispatched to the STS thread where it might not do much harm but would do no good either, running more code on shutdown than necessary.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Randell Jesup [:jesup] from comment #5)
> How does aRv get set in the success case?

In http://mxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorResult.h?rev=55fefc16edba&mark=47-47#44

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa6d7fab6e

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2aaa6d7fab6e

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

FYI I've verified that Bug 1147857 got uplifted to 37 even on desktop:

> ★ ~/moz/mozilla-release $ hg log -rFIREFOX_37_0_RELEASE:FIREFOX_37_0_1_RELEASE media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
> changeset:   252135:927bd4e66023
> user:        Randell Jesup <[e-mail]>
> date:        Sat Mar 28 21:45:42 2015 -0400
> files:       media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
> description:
> Bug 1147857 - Followup patch to continue BuildStats cleanup. r=jib, a=lmandel

and I've verified that the problem exists on 37.0.1 locally on OSX, so I'll be requesting uplift to release here.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8590824 [details] [diff] [review]
Part 1: fix about:webrtc to not blank on zero allocated PeerConnections (minimized uplift version)

Approval Request Comment
[Feature/regressing bug #]: Bug 1147857

[User impact if declined]:
about:webrtc will appear blank after a webrtc call, with no access to connection logs or stats on just-closed peerConnections, unless at least one tab is open that has allocated mozRTCPeerConnection instances.

This problem is common with Hello where ending a call closes the chat window, which hampers post-mortem call diagnostics and support in the field.

Workaround: open a tab that allocates at least one mozRTCPeerConnection, and refresh about:webrtc to unhide the information.

[Describe test coverage new/current, TreeHerder]:
Landed on m-c. Verified locally on OSX.

[Risks and why]:
Very low risk since this patch mostly restores the behavior prior to patch 2 in bug 1147857 (patch 2 is the cleanup-for-symmetry one, not the one fixing the actual crash), which is to dispatch to STS thread even with an empty query list, such that the eventual success callback (e.g. about:webrtc's) gets called.

The part that remains of the new behavior is that any failure to build a single query still bails on the entire request, on the reasoning that such failures are always systemic, e.g. we're in shutdown and necessary resources are gone.

[String/UUID change made/needed]: none

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Approval request is for Part 1 here only.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590824 [details] [diff] [review]
Part 1: fix about:webrtc to not blank on zero allocated PeerConnections (minimized uplift version)

Taking it as we rely a lot on webrtc for Hello.
Should be in 38 beta 5.

I will let Lawrence make the call for 37.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/258c70a0728a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e487ace8d7f9

Comment 16

[Maire Reavy [:mreavy]]

37.0.2 goes to build today.  Although this patch is very low risk (as jib says), this is a debugging tool; not part of the core product, and I blanch at taking a patch that just landed a couple of days ago directly to release unless we're solving a critical problem for the product that can't be solved any other way.  And the workaround (of effectively starting a new call), while annoying, will work.  

I would like this to be considered for 37.0.3 if we need to do one.  I don't think this patch needs a lot of bake time, but I'd like it to have a few days to a week on Aurora/Beta before taking it into release.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

As Maire advises, we will not take this change in 37.0.2 but I will keep it on the list in case there is a need for 37.0.3.

I am marking 37 as affected as it will be after 37.0.2 ships.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590824 [details] [diff] [review]
Part 1: fix about:webrtc to not blank on zero allocated PeerConnections (minimized uplift version)

m-r is now 38. Clearing the flag.
If we take it, we will use a relbranch for this.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

Marking Firefox 37 status as wontfix as this release will be EOL when Firefox 38 is released on Tuesday.