958221 - For global RTC stats, we should use a global object in c++ land and not individual RTCPeerConnections

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

Currently, our handling of GetLogging() and GetAllStats() is a little sad. For GetLogging(), we just grab any RTCPeerConnection we can find and ask for a log dump; this means we cannot access logging if no RTCPeerConnections currently exist. This is not quite right. For GetAllStats(), we iterate over all RTCPeerConnections in JS, and ask each to fire stats callbacks at the same callback. This is a little bit inefficient, and also doesn't fit our usual callback paradigm.

What we need to be doing instead is basing the WebrtcGlobal API on a class in c++ that has access to all of the PeerConnections, and the logging.

Comment 1

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

(In reply to Byron Campen [:bwc] from comment #0)
> What we need to be doing instead is basing the WebrtcGlobal API on a class
> in c++ that has access to all of the PeerConnections, and the logging.

And then use IPeerConnectionManager to access the GlobalPCList? That could work. Or we could give the existing WebrtcGlobalInformation in PeerConnection.js an internal c++ global it can query instead.

Comment 2

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #1)
> (In reply to Byron Campen [:bwc] from comment #0)
> > What we need to be doing instead is basing the WebrtcGlobal API on a class
> > in c++ that has access to all of the PeerConnections, and the logging.
> 
> And then use IPeerConnectionManager to access the GlobalPCList? That could
> work. Or we could give the existing WebrtcGlobalInformation in
> PeerConnection.js an internal c++ global it can query instead.

I was leaning toward the latter, since we know for sure that we need something like that to handle the logging.

Comment 3

[Byron Campen [:bwc]]

Attachment: Part 1. Move (but not alter) some code as step one in refactoring the GetStats functionality so it can be used by other classes.

A prelude to some refactoring; just moving code around, and altering as little as possible, to make the actual alterations easier to review.

Comment 4

[Byron Campen [:bwc]]

Attachment: Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

Package the laundry list of parameters involved in getting stats in a query object, rename some stuff to make it more obvious what it does, and stop exporting a function.

Comment 5

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=f2b8da3f73c6

Comment 6

[Byron Campen [:bwc]]

Attachment: Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

Function renaming ended up in a later patch file; putting it here.

Comment 7

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

Comment on attachment 8365361 [details] [diff] [review]
Part 1. Move (but not alter) some code as step one in refactoring the GetStats functionality so it can be used by other classes.

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

lgtm.

Comment 8

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

Comment on attachment 8366271 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

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

r+ with a few nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1310,2 @@
>                                 mHandle,
> +                               query),

The WrapRunnable macro creates a copy of query here, which means RTCStatsQuery is duplicated, which works as long as everything in the struct is copyable, but this might surprise someone modifying the struct in the future. This was marginally clearer before. It is also inefficient, which I realize is not a regression.

An nsAutoPtr would solve this and is used elsewhere with WrapRunnable in this file. As a bonus, the need to pass by non-const reference would go away. ;-)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +155,5 @@
>  };
>  
> +#ifdef MOZILLA_INTERNAL_API
> +// Not an inner class so we can forward declare.
> +class RTCStatsQuery {

I don't understand. Either forward-declare here and move this definition to PeerConnectionImpl.cpp, or make it an inner class.

@@ +163,5 @@
> +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> +    mozilla::RefPtr<NrIceCtx> iceCtx;
> +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> +    DOMHighResTimeStamp now;
> +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;

Now that the entire query struct is the handoff, RTCStatsReportInternal can be embedded and we don't need nsAutoPtr here I think.

@@ +164,5 @@
> +    mozilla::RefPtr<NrIceCtx> iceCtx;
> +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> +    DOMHighResTimeStamp now;
> +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;
> +    nsCOMPtr<nsIThread> mainThread;

Why pass around mainThread?

@@ +518,2 @@
>  
> +  static nsresult ExecuteStatsQuery_s(RTCStatsQuery &query);

I dislike non-const reference passing, because it looks deceptively like pass-by-value at the call-site. Can we use a pointer?

@@ +567,5 @@
>        std::string& fingerprint, size_t& spaceIdx) const;
>  
>  
>  #ifdef MOZILLA_INTERNAL_API
> +  static void GetStatsForPCObserver_s(

I like the new names.

@@ +572,2 @@
>        const std::string& pcHandle,
> +      RTCStatsQuery &query);

Pointer?

@@ +577,3 @@
>        const std::string& pcHandle,
>        nsresult result,
> +      RTCStatsQuery &query);

Same here.

Comment 9

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> Comment on attachment 8366271 [details] [diff] [review]
> Part 2. Simplify the statistics accessors, and rename things to better
> describe what they do.
> 
> Review of attachment 8366271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a few nits.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1310,2 @@
> >                                 mHandle,
> > +                               query),
> 
> The WrapRunnable macro creates a copy of query here, which means
> RTCStatsQuery is duplicated, which works as long as everything in the struct
> is copyable, but this might surprise someone modifying the struct in the
> future. This was marginally clearer before. It is also inefficient, which I
> realize is not a regression.
> 
> An nsAutoPtr would solve this and is used elsewhere with WrapRunnable in
> this file. As a bonus, the need to pass by non-const reference would go
> away. ;-)
> 

   For the most part, that struct contains RefPtrs and some other simple stuff. But, it could be made slightly more efficient with an auto ptr, and perhaps easier to analyze.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +155,5 @@
> >  };
> >  
> > +#ifdef MOZILLA_INTERNAL_API
> > +// Not an inner class so we can forward declare.
> > +class RTCStatsQuery {
> 
> I don't understand. Either forward-declare here and move this definition to
> PeerConnectionImpl.cpp, or make it an inner class.
>

   I was planning on forward declaring it later in another class (the class that will be backing WebrtcGlobalInformation).

> @@ +163,5 @@
> > +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> > +    mozilla::RefPtr<NrIceCtx> iceCtx;
> > +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> > +    DOMHighResTimeStamp now;
> > +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;
> 
> Now that the entire query struct is the handoff, RTCStatsReportInternal can
> be embedded and we don't need nsAutoPtr here I think.
> 

   If we pass the query in an auto ptr, I agree. Probably pretty sensible.

> @@ +164,5 @@
> > +    mozilla::RefPtr<NrIceCtx> iceCtx;
> > +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> > +    DOMHighResTimeStamp now;
> > +    nsAutoPtr<mozilla::dom::RTCStatsReportInternal> report;
> > +    nsCOMPtr<nsIThread> mainThread;
> 
> Why pass around mainThread?
>

   Ultimately, everything in the query must be freed on the main thread, and it is up to the caller to enforce this (yuck!). Throwing a pointer to the main thread at least makes this burden slightly less annoying.
 
> @@ +518,2 @@
> >  
> > +  static nsresult ExecuteStatsQuery_s(RTCStatsQuery &query);
> 
> I dislike non-const reference passing, because it looks deceptively like
> pass-by-value at the call-site. Can we use a pointer?
>

   Yeah, our convention is to use pointers for out vars. I can do this.
 
> @@ +572,2 @@
> >        const std::string& pcHandle,
> > +      RTCStatsQuery &query);
> 
> Pointer?
>

   Sure.

> @@ +577,3 @@
> >        const std::string& pcHandle,
> >        nsresult result,
> > +      RTCStatsQuery &query);
> 
> Same here.

   Sure.

Comment 10

[Byron Campen [:bwc]]

Attachment: Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

Incorporate feedback.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8366747 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

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

Carry forward r+ from jib.

Comment 12

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=d8e3d3695d3f

Comment 13

[Byron Campen [:bwc]]

Attachment: Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

Add some error checking, and fix a crash bug on some platforms.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8367512 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

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

Carry forward r+ from jib.

Comment 15

[Byron Campen [:bwc]]

Needinfo myself to push to try once it reopens.

Comment 16

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=7acdb018ae10

Comment 17

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

(In reply to Byron Campen [:bwc] from comment #9)
> > > +// Not an inner class so we can forward declare.
> > > +class RTCStatsQuery {
> > 
> > I don't understand. Either forward-declare here and move this definition to
> > PeerConnectionImpl.cpp, or make it an inner class.
> >
> 
>    I was planning on forward declaring it later in another class (the class
> that will be backing WebrtcGlobalInformation).

Does that other class need to access the details of ths struct? If not, I think we should move the definition of RTCStatsQuery to PeerConnectionImpl.cpp, effectively containing the details of this struct to the implementation (I've verified locally that this compiles).

> > Why pass around mainThread?
> 
>    Ultimately, everything in the query must be freed on the main thread, and
> it is up to the caller to enforce this (yuck!). Throwing a pointer to the
> main thread at least makes this burden slightly less annoying.

Yeah but mainThread is ubiquitous (e.g.  NS_DispatchToMainThread()), so there's no need to pass pointers to it around, so I don't think we should here either.

Comment 18

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #17)
> (In reply to Byron Campen [:bwc] from comment #9)
> > > > +// Not an inner class so we can forward declare.
> > > > +class RTCStatsQuery {
> > > 
> > > I don't understand. Either forward-declare here and move this definition to
> > > PeerConnectionImpl.cpp, or make it an inner class.
> > >
> > 
> >    I was planning on forward declaring it later in another class (the class
> > that will be backing WebrtcGlobalInformation).
> 
> Does that other class need to access the details of ths struct? If not, I
> think we should move the definition of RTCStatsQuery to
> PeerConnectionImpl.cpp, effectively containing the details of this struct to
> the implementation (I've verified locally that this compiles).
> 

   It needs to access the stats report, for sure. I have some work-in-progress that hides the rest using pimpl idiom, although strangely enough none of the mozilla auto_ptr-like classes can be used for this without generating warnings (why does ScopedDeletePointer<Internal> work fine in PeerConnectionImpl, but not RTCStatsQuery?). A private std::auto_ptr works fine as long as a d'tor is declared (but not defined) in the header. I need to figure out what is different about PeerConnectionImpl::mInteral here.

> > > Why pass around mainThread?
> > 
> >    Ultimately, everything in the query must be freed on the main thread, and
> > it is up to the caller to enforce this (yuck!). Throwing a pointer to the
> > main thread at least makes this burden slightly less annoying.
> 
> Yeah but mainThread is ubiquitous (e.g.  NS_DispatchToMainThread()), so
> there's no need to pass pointers to it around, so I don't think we should
> here either.

   Ok, as long as that's easy.

Comment 19

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Moved the impl of WebrtcGlobalInformation to c++.

Comment 20

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Fixing RTP stats reporting, while preventing duplicate ICE stats.

Comment 21

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Remove obsolete param.

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8372369 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

This is going to need a DOM review; the basic idea is that I am moving a JS-implemented chrome-only statistics API to a C++ implementation.

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8372369 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

I'm not a DOM peer. :)

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8372369 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

>+[ChromeOnly,
>+ Constructor ()]
>+interface WebrtcGlobalInformation {
>+
>+  [Throws]
>+  void getAllStats(WebrtcGlobalStatisticsCallback callback);
>+
>+  [Throws]
>+  void getLogging(DOMString pattern,
>+                  WebrtcGlobalLoggingCallback callback);
>+};
Couldn't you just use static methods here? That would simplify the implementation quite a bit, 
if I understand the patch correctly.

Comment 25

[Byron Campen [:bwc]]

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8372369 [details] [diff] [review]
> Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> >+[ChromeOnly,
> >+ Constructor ()]
> >+interface WebrtcGlobalInformation {
> >+
> >+  [Throws]
> >+  void getAllStats(WebrtcGlobalStatisticsCallback callback);
> >+
> >+  [Throws]
> >+  void getLogging(DOMString pattern,
> >+                  WebrtcGlobalLoggingCallback callback);
> >+};
> Couldn't you just use static methods here? That would simplify the
> implementation quite a bit, 
> if I understand the patch correctly.

Did not realize that was an option, but yes that would be great! Let me try that out.

Comment 26

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Switch to static functions on a constructor-less interface. Simpler, but still lots of cruft that the binding code won't let me remove.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 8372661 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

So, I switched over to static functions, and removed the Constructor from the interface, but there is still a lot of boilerplate that I'm required to leave in place that is just dead code. In particular, I've deleted every c++ constructor for the class in question, so there cannot be any instance of it initialized, but the binding code doesn't know this and still insists that certain non-static functions be defined. Is there any way around this?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8372661 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.


>+// TODO([e-mail]): Hello DOM reviewer! Since this class has no
>+// constructor, but there is still code generated that expects it to have
>+// a mountain of macro declared/implemented functions, all of this stuff is
>+// just wasted code. Is there a way around this that I'm missing?
No need to cycle collect anything or inherit wrappercache etc.
You may need to add wrapperCache: False to Bindings.conf
I think either concrete: False would drop the requirement for AddRef/Release,
or nativeOwnership: 'owned'.
Would need to look at the generated code in <objdir>/dom/bindings/
'owned' is in theory right, but requires inheriting NonRefcountedDOMObject which is
in this case annoying.



>+
>+nsPIDOMWindow* WebrtcGlobalInformation::GetParentObject() {
nsPIDOMWindow* should be in the previous line, and { should be on its own line.
same also elsewhere.

>+void WebrtcGlobalInformation::GetAllStats(
>+    const GlobalObject &aGlobal,
>+    WebrtcGlobalStatisticsCallback &statsCallback,
>+    ErrorResult &err) {

void
WebrtcGlobalInformation::GetAllStats(const GlobalObject& aGlobal,
                                     WebrtcGlobalStatisticsCallback& aCallback,
                                     ErrorResult& aErr)
{
Same also elsewhere


>+
>+  err = WebrtcGlobalInformationImpl::GetAllStats(statsCallback);
>+}
So why you need WebrtcGlobalInformationImpl and
WebrtcGlobalInformation?

Comment 29

[Byron Campen [:bwc]]

(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8372661 [details] [diff] [review]
> Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> 
> >+// TODO([e-mail]): Hello DOM reviewer! Since this class has no
> >+// constructor, but there is still code generated that expects it to have
> >+// a mountain of macro declared/implemented functions, all of this stuff is
> >+// just wasted code. Is there a way around this that I'm missing?
> No need to cycle collect anything or inherit wrappercache etc.
> You may need to add wrapperCache: False to Bindings.conf
> I think either concrete: False would drop the requirement for AddRef/Release,
> or nativeOwnership: 'owned'.
> Would need to look at the generated code in <objdir>/dom/bindings/
> 'owned' is in theory right, but requires inheriting NonRefcountedDOMObject
> which is
> in this case annoying.
> 

   I'll give these a shot.

> >+
> >+  err = WebrtcGlobalInformationImpl::GetAllStats(statsCallback);
> >+}
> So why you need WebrtcGlobalInformationImpl and
> WebrtcGlobalInformation?

Because the implementation requires a _lot_ of extra include paths in media/webrtc, while putting things in media/webrtc requires picking up a bunch of new libraries.

Comment 30

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Apply a couple of tweaks courtesy of smaug, which allowed me to delete a bunch of code. Hooray!

Comment 31

[Byron Campen [:bwc]]

Comment on attachment 8373694 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

This is a _lot_ closer to what I wanted to write in the first place, at least DOM wise.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8373694 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.


>+#ifdef MOZILLA_INTERNAL_API
>+class RTCStatsQuery::Internal {
Nit, { goes to its new line.


>+// Work around the fact that std::vector does not support move semantics in
>+// our build configuration (ugh!)
Use 

>+namespace {
>+class JSErrorResult : public ErrorResult
Call it JSExceptionEater or some such :)

>+{
>+public:
>+  ~JSErrorResult()
>+  {
>+    WouldReportJSException();
>+    if (IsJSException()) {
>+      MOZ_ASSERT(NS_IsMainThread());
>+      AutoJSContext cx;
>+      dom::Optional<JS::Handle<JS::Value> > value(cx);
>+      StealJSException(cx, &value.Value());
>+    }
>+  }
>+};
>+}
This is so sad. In which case do we need it? (And if this is really needed, it hints that this kind of helper should be in the bindings code)

>+static void OnGetLogging_m(
>+    dom::WebrtcGlobalLoggingCallback *loggingCallback,
4 space indentation and _m? Argument names should be in form aFoo. And * goes with the type.
(Would be nice to get WebRTC to use the common coding style. Perhaps that can happen gradually.)


I think this setup starts to look pretty good. A lot simpler than the previous one. Needs a re-review from a webrtc peer too, IMO.

Comment 33

[Byron Campen [:bwc]]

(In reply to Olli Pettay [:smaug] from comment #32)
> Comment on attachment 8373694 [details] [diff] [review]
> Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> >+// Work around the fact that std::vector does not support move semantics in
> >+// our build configuration (ugh!)
> Use 
> 

   Use what? The suspense is killing me...

> >+namespace {
> >+class JSErrorResult : public ErrorResult
> Call it JSExceptionEater or some such :)
> 
> >+{
> >+public:
> >+  ~JSErrorResult()
> >+  {
> >+    WouldReportJSException();
> >+    if (IsJSException()) {
> >+      MOZ_ASSERT(NS_IsMainThread());
> >+      AutoJSContext cx;
> >+      dom::Optional<JS::Handle<JS::Value> > value(cx);
> >+      StealJSException(cx, &value.Value());
> >+    }
> >+  }
> >+};
> >+}
> This is so sad. In which case do we need it? (And if this is really needed,
> it hints that this kind of helper should be in the bindings code)
> 

   I agree. This is borrowed from PeerConnectionImpl. I can check to see what happens if I remove it and throw exceptions from the JS callbacks, but be aware that this is not the only place this sort of thing exists, and I was hoping to learn of a better way.

> >+static void OnGetLogging_m(
> >+    dom::WebrtcGlobalLoggingCallback *loggingCallback,
> 4 space indentation and _m? Argument names should be in form aFoo. And *
> goes with the type.
> (Would be nice to get WebRTC to use the common coding style. Perhaps that
> can happen gradually.)

   Unfortunately, any code standard we use seems to be on a file-by-file basis at best.

> 
> 
> I think this setup starts to look pretty good. A lot simpler than the
> previous one. Needs a re-review from a webrtc peer too, IMO.

   Of course.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Oops, I was going to say that use some Gecko thing, like LinkedList or nsTArray or such, but
they may not have the feature set you want.

Comment 35

[Byron Campen [:bwc]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Oops, I was going to say that use some Gecko thing, like LinkedList or
> nsTArray or such, but
> they may not have the feature set you want.

nsTArray is definitely no good, since RTCStatsQuery is not POD. Let me go look at LinkedList (I tried looking on MDN for documentation of the container classes that are available, but could only find the various xArray stuff that doesn't do what I need).

Comment 36

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

bryon: What are the requirements you have here?

Comment 37

[Byron Campen [:bwc]]

1. We should be able to get r_log regardless of whether any PeerConnections exist at the moment (the present code requires at least one PC to exist as an API entry point).
2. The JS callback for global webrtc statistics should be called exactly once, containing either a statistics report or an error for each PC (the current code requires each PC to call the callback with its own statistics report).

Comment 38

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Fix up some conventions, and remove some more unnecessary code.

Comment 39

[Byron Campen [:bwc]]

So, LinkedList isn't much help either; I'm looking for a container that takes ownership of the stuff you give to it. LinkedList asserts that it be empty prior to destruction.

Comment 40

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Substitute a mozilla::Vector<nsAutoPtr<RTCStatsQuery>> for our std::vector hack, since it supports move semantics.

Comment 41

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

A little more cleanup of TODOs. Still one left.

Comment 42

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Ok, enough polish. Let's get this reviewed.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8377813 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

>+#ifdef MOZILLA_INTERNAL_API
>+class RTCStatsQuery::Internal {
>+  public:
>+    Internal() : internalStats(false) {}
>+
>+    std::string pcName;
>+    bool internalStats;
>+    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
>+    mozilla::RefPtr<NrIceCtx> iceCtx;
>+    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
>+    DOMHighResTimeStamp now;
>+};
Any chance for normal coding style, or is this what webrtc uses?
(mFoo, and { goes to its own line when defining a class)



>+
>+RTCStatsQuery::~RTCStatsQuery() {
>+  MOZ_ASSERT(NS_IsMainThread());
>+  delete mInternal;
>+}
Hmm, this screams nsAutoPtr for mInternal

>+WebrtcGlobalInformation::GetLogging(
>+    const GlobalObject& aGlobal,
>+    const nsAString& aPattern,
>+    WebrtcGlobalLoggingCallback& aLoggingCallback,
>+    ErrorResult& aRv)
>+{
>+  if (!NS_IsMainThread()) {
>+    aRv.Throw(NS_ERROR_NOT_SAME_THREAD);
>+    return;
>+  }
>+
>+  nsresult res;
rv

>+  nsCOMPtr<nsIEventTarget> stsThread =
>+      do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &res);
2 space indentation

>+
>+  if (!stsThread) {
>+    aRv.Throw(res);
>+    return;
>+  }
>+
>+  std::string pattern(NS_ConvertUTF16toUTF8(aPattern).get());
Since I don't know std::string's ownership model, someone else should review this kind of stuff.
(Using nsCString here would be nicer.)


>+class WebrtcGlobalInformation {
{ to next line

>+  WebrtcGlobalInformation() MOZ_DELETE;
>+  WebrtcGlobalInformation(const WebrtcGlobalInformation& orig) MOZ_DELETE;
aOrig

>+  WebrtcGlobalInformation& operator=(
>+      const WebrtcGlobalInformation& rhs) MOZ_DELETE;
2 space indentation please, and aRhs


This needs r+ from a webrtc peer (if it hasn't got that already).

Comment 44

[Byron Campen [:bwc]]

(In reply to Olli Pettay [:smaug] from comment #43)
> Comment on attachment 8377813 [details] [diff] [review]
> Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> >+#ifdef MOZILLA_INTERNAL_API
> >+class RTCStatsQuery::Internal {
> >+  public:
> >+    Internal() : internalStats(false) {}
> >+
> >+    std::string pcName;
> >+    bool internalStats;
> >+    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> >+    mozilla::RefPtr<NrIceCtx> iceCtx;
> >+    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> >+    DOMHighResTimeStamp now;
> >+};
> Any chance for normal coding style, or is this what webrtc uses?
> (mFoo, and { goes to its own line when defining a class)

   This is the style used in this file already, and inter-file consistency is less important than intra-file consistency.

> >+
> >+RTCStatsQuery::~RTCStatsQuery() {
> >+  MOZ_ASSERT(NS_IsMainThread());
> >+  delete mInternal;
> >+}
> Hmm, this screams nsAutoPtr for mInternal
> 

   Agreed; however, using nsAutoPtr on a forward declared type spews warnings, even when the member is private and there is a destructor declared. Strangely enough, std::auto_ptr does _not_ spew warnings for exactly the same thing, but I would need to add a comment explaining myself for that one. This is less trouble.

> >+WebrtcGlobalInformation::GetLogging(
> >+    const GlobalObject& aGlobal,
> >+    const nsAString& aPattern,
> >+    WebrtcGlobalLoggingCallback& aLoggingCallback,
> >+    ErrorResult& aRv)
> >+{
> >+  if (!NS_IsMainThread()) {
> >+    aRv.Throw(NS_ERROR_NOT_SAME_THREAD);
> >+    return;
> >+  }
> >+
> >+  nsresult res;
> rv
> 

   Ok.

> >+  nsCOMPtr<nsIEventTarget> stsThread =
> >+      do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &res);
> 2 space indentation
> 
> >+
> >+  if (!stsThread) {
> >+    aRv.Throw(res);
> >+    return;
> >+  }
> >+
> >+  std::string pattern(NS_ConvertUTF16toUTF8(aPattern).get());
> Since I don't know std::string's ownership model, someone else should review
> this kind of stuff.
> (Using nsCString here would be nicer.)
>

   There is basically no way to make std::string take ownership of a buffer; it always does deep copies.
 
> 
> >+class WebrtcGlobalInformation {
> { to next line
> 

   Ok.

> >+  WebrtcGlobalInformation() MOZ_DELETE;
> >+  WebrtcGlobalInformation(const WebrtcGlobalInformation& orig) MOZ_DELETE;
> aOrig
> 
> >+  WebrtcGlobalInformation& operator=(
> >+      const WebrtcGlobalInformation& rhs) MOZ_DELETE;
> 2 space indentation please, and aRhs

   Ok.

> This needs r+ from a webrtc peer (if it hasn't got that already).

Of course, just getting reviews out of the way one at a time.

Comment 45

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Incorporate feedback.

Comment 46

[Byron Campen [:bwc]]

Comment on attachment 8378624 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Carry forward r=smaug

Comment 47

[Byron Campen [:bwc]]

Comment on attachment 8378624 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Carry forward r=smaug

Comment 48

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=2a98655b47e1

Comment 49

[Byron Campen [:bwc]]

Comment on attachment 8365361 [details] [diff] [review]
Part 1. Move (but not alter) some code as step one in refactoring the GetStats functionality so it can be used by other classes.

Need to go ahead and check this in, since it blocks other work.

Comment 50

[Byron Campen [:bwc]]

Comment on attachment 8367512 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do.

Need to go ahead and check this in, since it blocks other work.

Comment 51

[Byron Campen [:bwc]]

Comment on attachment 8378624 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Butterfingers...

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Part 2 has conflicts on inbound. Also, we're back to using individual checkin? requests again? :(

Comment 53

[Byron Campen [:bwc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #52)
> Part 2 has conflicts on inbound. Also, we're back to using individual
> checkin? requests again? :(

   Something must have rotted it yesterday. Let me take a look. Please don't land part 3 yet.

Comment 54

[Byron Campen [:bwc]]

Wow. These got rotted so bad that I'm going to have to start over.

Comment 55

[Byron Campen [:bwc]]

Attachment: Part 2. Simplify the statistics accessors, and rename things to better describe what they do. r=jib

Unrot.

Comment 56

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot.

Comment 57

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=62735a000c32

Comment 58

[Byron Campen [:bwc]]

Comment on attachment 8379096 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do. r=jib

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

Carry forward r=jib (will request checkin once it is clear that the unrot did not lead to bustage)

Comment 59

[Byron Campen [:bwc]]

Comment on attachment 8379097 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Adam is unavailable for review; do you have cycles?

Comment 60

[Byron Campen [:bwc]]

Comment on attachment 8379097 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Adam is unavailable for review; do you have cycles?

(Also, carry forward r=smaug)

Comment 61

[Byron Campen [:bwc]]

Needinfo myself to keep an eye on the try push.

Comment 62

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

Sure, looking now.

Comment 63

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

Comment on attachment 8379097 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

I've just reviewed the xhtml but wanted to give a heads up on r- for incorrect display of RTCP information.

Review comments on plumbing next.

::: toolkit/content/aboutWebrtc.xhtml
@@ +21,2 @@
>    } catch (e) {
> +    console.log("Exception calling getLogging()" + e.toString());

Don't you want a space after getLogging()? Seems like you never observed a error. What specifically are you catching here?

@@ +29,2 @@
>    } catch(e) {
> +    console.log("Exception calling getLogging()" + e.toString());

Same questions.

@@ +238,5 @@
> +  rtpHeading.appendChild(document.createTextNode("RTP statistics"));
> +  newPcDiv.appendChild(rtpHeading);
> +
> +  // Build map from id -> inbound RTP stats; outbound RTP stats record their
> +  // associated inbound RTP stats, but not vice versa.

This code comment sounds wrong, so I worry the code here is wrong too.

Inbound and outbound stats don't all point to each other, but when they do, the link is reciprocal.

I think the confusion here is that RTCP information is stored separately from RTP information in opposite structures:

Peer A         | RTP info                     RTCP info
---------------+-------------------------------------------------------
               |
Transmit pipe: | outboundRTPStreamStats <-----------------------------.
               |   .isRemote = false                                  |
               |   .remoteId *--------------> inboundRTPStreamStats   |
               |                                .isRemote = true      |
               |                                .remoteId *-----------'
               |                           
Receive pipe:  | inboundRTPStreamStats <------------------------------.
               |   .isRemote = false                                  |
               |   .remoteId *--------------> outboundRTPStreamStats  |
               |                                .isRemote = true      |
               |                                .remoteId *-----------'

isRemote=true means this is RTCP info.

I recommend looking at https://bug947665.bugzilla.mozilla.org/attachment.cgi?id=8362332 for an example of how to pair this information for display (wait ~5 seconds for RTCP info to appear).

@@ +255,5 @@
> +              .appendChild(document.createTextNode(outboundRtpStat.id));
> +      newPcDiv.appendChild(dumpStat(outboundRtpStat, "Local: "));
> +
> +      if (outboundRtpStat.remoteId) {
> +        // For sendrecv, group the inbound stats with the outbound one

Yeah this is wrong. Having a remoteId means there is RTCP information available for this direction, not that we are sendrecv vs sendonly.

@@ +318,2 @@
>    } catch(e) {
> +    console.log("Exception calling getAllStats():" + e.toString());

Same question.

Comment 64

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #63)

   Ah, I see. Will fix just as soon as I'm done unrotting some other stuff.

Comment 65

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Incorportate some feedback from jib.

Comment 66

[Byron Campen [:bwc]]

Comment on attachment 8379328 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

I think this should address the feedback on about:webrtc.

Comment 67

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

Comment on attachment 8379328 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

r- for removing queueOrRun, mInternal ugliness not seeming necessary, and maybe use nsTArray.

::: dom/media/PeerConnection.js
@@ -821,5 @@
>    },
>  
>    getStats: function(selector, onSuccess, onError) {
> -    this._queueOrRun({
> -      func: this._getStats,

You're removing the queueing of getStats here when I suspect you just meant to remove getStatsInternal...

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +153,5 @@
> +    Internal() : internalStats(false) {}
> +
> +    std::string pcName;
> +    bool internalStats;
> +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;

We actually use nsTArray<nsRefPtr<Foo>> elsewhere in this same file, so we should probably stick with that precedent where possible.

@@ +155,5 @@
> +    std::string pcName;
> +    bool internalStats;
> +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> +    mozilla::RefPtr<NrIceCtx> iceCtx;
> +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;

I don't see anything in here that should require the mInternal indirection, just a bunch of RefPtrs. PeerConnectionImpl.h is already full of RefPtrs to forward-declared types. e.g. mozilla::RefPtr<DtlsIdentity> , wont that work for mozilla::MediaPipeline and mozilla::NrIceMediaStream? Then we can avoid this ugly indirection throughout.

@@ +1931,5 @@
>      bool internalStats,
>      RTCStatsQuery *query) {
>  
> +  query->mInternal = new RTCStatsQuery::Internal;
> +  query->mInternal->internalStats = internalStats;

Avoiding constructor?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +169,5 @@
> +  private:
> +    friend class PeerConnectionImpl;
> +    // This contains a bunch of internals; don't force the user to include
> +    // stuff like MediaPipeline.h etc.
> +    Internal* mInternal;

Did you figure out why you couldn't use
mozilla::ScopedDeletePtr<Internal> mInternal; ?

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +26,5 @@
> +using sipcc::PeerConnectionCtx;
> +using sipcc::RTCStatsQuery;
> +
> +static const char* logTag = "WebrtcGlobalInformation";
> +typedef NS_ConvertUTF8toUTF16 ObString;

ObString, short for PCObserverString, was a trick in PeerConnectionImpl.cpp to make unittests work there. Not really accomplishing much here.

@@ +34,5 @@
> +
> +typedef Vector<nsAutoPtr<RTCStatsQuery>> RTCStatsQueries;
> +
> +static void OnStatsReport_m(
> +  dom::WebrtcGlobalStatisticsCallback* aStatsCallback,

- dom:: You're already inside dom here. x13.

@@ +90,5 @@
> +                          NS_DISPATCH_NORMAL);
> +}
> +
> +static void OnGetLogging_m(
> +  dom::WebrtcGlobalLoggingCallback* aLoggingCallback,

How about already_AddRefed<WebrtcGlobalLoggingCallback> aLoggingCallback here? This semantic adds some extra safety with less casting and potentially smoother handoff since nsRefPtr.forget() function returns this type (except in this case handling the dispatch failure case safely makes using .forget() not as smooth as I'd hoped).

@@ +146,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIEventTarget> stsThread =
> +    do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +
> +  if (!stsThread) {

if (NS_FAILED(rv)) {

@@ +164,5 @@
> +         ++p) {
> +      MOZ_ASSERT(p->second);
> +
> +      // If push_back throws, we leak. Is this worth writing some boilerplate
> +      // for?

This is mozilla::Vector, so this comment seems out of place.

@@ +176,5 @@
> +  // We cannot pass nsRefPtr, since that trips over threadsafety assertions.
> +  // (CallbackObject does not use a threadsafe reference count)
> +  // We also cannot use weak pointer semantics, since those aren't
> +  // supported on CallbackObject. We could instead use something like
> +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.

I actually like the nsAutoPtr<nsRefPtr<>> idea a lot. We would need to be solidly sure about STS shutdown never freeing unprocessed events or it might free the reference on the wrong thread rather than leak it should things go south before the roundtrip completes (I'm pretty sure it's fine). I suppose we would have run across shutdown leaks by now if that wasn't tight. But this is fine as well.

@@ +178,5 @@
> +  // We also cannot use weak pointer semantics, since those aren't
> +  // supported on CallbackObject. We could instead use something like
> +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.
> +  nsRefPtr<dom::WebrtcGlobalStatisticsCallback>
> +    exceptionSafety(&aStatsCallback);

exceptionSafety seems like a misnomer since there are no exceptions involved. How about callback?

@@ +210,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIEventTarget> stsThread =
> +    do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +
> +  if (!stsThread) {

if (NS_FAILED(rv)) {

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.h
@@ +17,5 @@
> +}
> +}
> +
> +namespace mozilla {
> +namespace dom {

You're closing and reopening the same namespaces here. Boilerplaty.

::: toolkit/content/aboutWebrtc.xhtml
@@ +14,5 @@
>    </head>
>    <script>
>  
>  
>  function getCandPairLogs(id) {

Maybe inline this single-line stub? Called from one place.

@@ +20,3 @@
>  }
>  
>  function getAllLogs() {

Same here?

@@ +158,5 @@
>    newPcDiv.appendChild(heading);
>  
> +  if (stats.error) {
> +    var errorHeading = document.createElement('h4');
> +    errorHeading.appendChild(document.createTextNode("Error: " + stats.error));

When I refresh my running test page, causing it to stop, and refresh about:webrtc, I see this error:

  "Error: Error building stats query, PeerConnectionImpl not open"

Unfortunate double wording aside (why I put this here), this sounds alarming when when nothing is wrong. The errors (two in my case since I did a local loop test) stay around until I go to about:memory and manually garbage collect.

I vote we not show closed peerConnections at all, especially since it's hard to discern whether they're still in use by a page or not.

@@ +232,5 @@
> +
> +  // Build map from id -> remote RTP stats (ie; stats obtained from RTCP
> +  // from the other end). This allows us to pair up local/remote stats for
> +  // the same stream more easily.
> +  var remoteRtpStatsMap = {};

maybe rtcpStatsMap? The word "remote" was chosen because it works both ways, but here you're actually filtering our rtcp specifically.

Come to think of it, maybe isRtcp would be better than isRemote for the spec...

@@ +256,5 @@
> +      newPcDiv.appendChild(dumpStat(rtpStat, "Local: "));
> +
> +      // Might not be receiving RTCP, so we have no idea what the
> +      // statistics look like from the perspective of the other end.
> +      // TODO([e-mail]): Maybe put something like 

trailing whitespace.

@@ +259,5 @@
> +      // statistics look like from the perspective of the other end.
> +      // TODO([e-mail]): Maybe put something like 
> +      // "Remote: no statistics reported by other end" when we have no
> +      // remoteId? Might also be nice to display how stale this data is,
> +      // when present.

Commenting on your comment: I think what you have now looks good FWIW (only showing local data when there's no RTCP). The RTCP timestamps are different and already show how stale that data is.

@@ +283,3 @@
>    console.log("Got stats callback.");
> +  globalReport.reports.forEach(function (report) {
> +    var pcid = report.pcid;

pcid used once. Inline?

@@ +296,5 @@
> +    }
> +  });
> +
> +  globalReport.errors.forEach(function (error) {
> +    var pcid = error.pcid;

Used once. Inline?

@@ +303,5 @@
> +    var pcDiv = document.getElementById(pcDivHeading);
> +    var newPcDiv = buildPcDiv(error, pcDivHeading);
> +    newPcDiv.id = pcDivHeading;
> +
> +    if (!pcDiv) {

flip?

Comment 68

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #67)
> Comment on attachment 8379328 [details] [diff] [review]
> Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> Review of attachment 8379328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for removing queueOrRun, mInternal ugliness not seeming necessary, and
> maybe use nsTArray.
> 
> ::: dom/media/PeerConnection.js
> @@ -821,5 @@
> >    },
> >  
> >    getStats: function(selector, onSuccess, onError) {
> > -    this._queueOrRun({
> > -      func: this._getStats,
> 
> You're removing the queueing of getStats here when I suspect you just meant
> to remove getStatsInternal...
>

   Could be right, I'll look.
 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +153,5 @@
> > +    Internal() : internalStats(false) {}
> > +
> > +    std::string pcName;
> > +    bool internalStats;
> > +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> 
> We actually use nsTArray<nsRefPtr<Foo>> elsewhere in this same file, so we
> should probably stick with that precedent where possible.
> 

   This is how it was before, only moved. But, I'll attempt to change it.

> @@ +155,5 @@
> > +    std::string pcName;
> > +    bool internalStats;
> > +    std::vector<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
> > +    mozilla::RefPtr<NrIceCtx> iceCtx;
> > +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> 
> I don't see anything in here that should require the mInternal indirection,
> just a bunch of RefPtrs. PeerConnectionImpl.h is already full of RefPtrs to
> forward-declared types. e.g. mozilla::RefPtr<DtlsIdentity> , wont that work
> for mozilla::MediaPipeline and mozilla::NrIceMediaStream? Then we can avoid
> this ugly indirection throughout.
> 

   I tried that, and it doesn't work. PeerConnectionImpl.cpp is picking up the necessary includes that make it possible to create an RTCStatsQuery without the compiler complaining. WebrtcGlobalInformation.cpp does not.

> @@ +1931,5 @@
> >      bool internalStats,
> >      RTCStatsQuery *query) {
> >  
> > +  query->mInternal = new RTCStatsQuery::Internal;
> > +  query->mInternal->internalStats = internalStats;
> 
> Avoiding constructor?
> 

   I'm not sure what you mean. Are you proposing adding a c'tor that takes this bool?

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> @@ +169,5 @@
> > +  private:
> > +    friend class PeerConnectionImpl;
> > +    // This contains a bunch of internals; don't force the user to include
> > +    // stuff like MediaPipeline.h etc.
> > +    Internal* mInternal;
> 
> Did you figure out why you couldn't use
> mozilla::ScopedDeletePtr<Internal> mInternal; ?
> 

   It all hinges on whether the code that creates the type knows what the type is. Which WebrtcGlobalInformation.cpp does not.

> ::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
> @@ +26,5 @@
> > +using sipcc::PeerConnectionCtx;
> > +using sipcc::RTCStatsQuery;
> > +
> > +static const char* logTag = "WebrtcGlobalInformation";
> > +typedef NS_ConvertUTF8toUTF16 ObString;
> 
> ObString, short for PCObserverString, was a trick in PeerConnectionImpl.cpp
> to make unittests work there. Not really accomplishing much here.
> 

   It makes the code a little less noisy, but I could go either way.

> @@ +34,5 @@
> > +
> > +typedef Vector<nsAutoPtr<RTCStatsQuery>> RTCStatsQueries;
> > +
> > +static void OnStatsReport_m(
> > +  dom::WebrtcGlobalStatisticsCallback* aStatsCallback,
> 
> - dom:: You're already inside dom here. x13.
> 

   Yeah, I kept going back and forth on the namespacing here, and didn't fixup once it settled.

> @@ +90,5 @@
> > +                          NS_DISPATCH_NORMAL);
> > +}
> > +
> > +static void OnGetLogging_m(
> > +  dom::WebrtcGlobalLoggingCallback* aLoggingCallback,
> 
> How about already_AddRefed<WebrtcGlobalLoggingCallback> aLoggingCallback
> here? This semantic adds some extra safety with less casting and potentially
> smoother handoff since nsRefPtr.forget() function returns this type (except
> in this case handling the dispatch failure case safely makes using .forget()
> not as smooth as I'd hoped).
>

   I'll look into it.
 
> @@ +146,5 @@
> > +  nsresult rv;
> > +  nsCOMPtr<nsIEventTarget> stsThread =
> > +    do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> > +
> > +  if (!stsThread) {
> 
> if (NS_FAILED(rv)) {
>

   What circumstances will yield a failure result, but still return non null? I've already been bitten by stuff returning nullptr without setting an error, so to feel comfortable with doing this I'd need to spend some time reading the code involved here. Or I could just check both...
 
> @@ +164,5 @@
> > +         ++p) {
> > +      MOZ_ASSERT(p->second);
> > +
> > +      // If push_back throws, we leak. Is this worth writing some boilerplate
> > +      // for?
> 
> This is mozilla::Vector, so this comment seems out of place.
> 

   Right, forgot to remove that.

> @@ +176,5 @@
> > +  // We cannot pass nsRefPtr, since that trips over threadsafety assertions.
> > +  // (CallbackObject does not use a threadsafe reference count)
> > +  // We also cannot use weak pointer semantics, since those aren't
> > +  // supported on CallbackObject. We could instead use something like
> > +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.
> 
> I actually like the nsAutoPtr<nsRefPtr<>> idea a lot. We would need to be
> solidly sure about STS shutdown never freeing unprocessed events or it might
> free the reference on the wrong thread rather than leak it should things go
> south before the roundtrip completes (I'm pretty sure it's fine). I suppose
> we would have run across shutdown leaks by now if that wasn't tight. But
> this is fine as well.
>

   Yeah, this was exactly what I was worried about; if this goes out of scope on STS, we'll hit a threadsafety assertion on debug builds, tweak a non-threadsafe refcount on non-debug builds, and if we don't end up horribly in the weeds due to that, destroy the callback on the wrong thread. It seems to me that leaking is preferable.

> @@ +178,5 @@
> > +  // We also cannot use weak pointer semantics, since those aren't
> > +  // supported on CallbackObject. We could instead use something like
> > +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.
> > +  nsRefPtr<dom::WebrtcGlobalStatisticsCallback>
> > +    exceptionSafety(&aStatsCallback);
> 
> exceptionSafety seems like a misnomer since there are no exceptions
> involved. How about callback?
> 

   If you're confident there is no possibility of exceptions, this can be removed entirely, and we can just call AddRef() directly.

> ::: toolkit/content/aboutWebrtc.xhtml
> @@ +14,5 @@
> >    </head>
> >    <script>
> >  
> >  
> >  function getCandPairLogs(id) {
> 
> Maybe inline this single-line stub? Called from one place.
>

   Yeah, it is small enough at this point.
 
> @@ +20,3 @@
> >  }
> >  
> >  function getAllLogs() {
> 
> Same here?
>

   Yep.
 
> @@ +158,5 @@
> >    newPcDiv.appendChild(heading);
> >  
> > +  if (stats.error) {
> > +    var errorHeading = document.createElement('h4');
> > +    errorHeading.appendChild(document.createTextNode("Error: " + stats.error));
> 
> When I refresh my running test page, causing it to stop, and refresh
> about:webrtc, I see this error:
> 
>   "Error: Error building stats query, PeerConnectionImpl not open"
> 
> Unfortunate double wording aside (why I put this here), this sounds alarming
> when when nothing is wrong. The errors (two in my case since I did a local
> loop test) stay around until I go to about:memory and manually garbage
> collect.
> 
> I vote we not show closed peerConnections at all, especially since it's hard
> to discern whether they're still in use by a page or not.
>

   Hmm. I don't know. I could see it being useful for detecting that garbage collection didn't successfully collect a specific PC, or if some page is holding onto closed PCs for no good reason.

> @@ +232,5 @@
> > +
> > +  // Build map from id -> remote RTP stats (ie; stats obtained from RTCP
> > +  // from the other end). This allows us to pair up local/remote stats for
> > +  // the same stream more easily.
> > +  var remoteRtpStatsMap = {};
> 
> maybe rtcpStatsMap? The word "remote" was chosen because it works both ways,
> but here you're actually filtering our rtcp specifically.
> 
> Come to think of it, maybe isRtcp would be better than isRemote for the
> spec...
> 

   I'd choose wasReportedByRemote; that's maximally clear to everyone whether they know what RTCP is or not.

> @@ +259,5 @@
> > +      // statistics look like from the perspective of the other end.
> > +      // TODO([e-mail]): Maybe put something like 
> > +      // "Remote: no statistics reported by other end" when we have no
> > +      // remoteId? Might also be nice to display how stale this data is,
> > +      // when present.
> 
> Commenting on your comment: I think what you have now looks good FWIW (only
> showing local data when there's no RTCP). The RTCP timestamps are different
> and already show how stale that data is.
> 

   Ok, will remove comment.

> @@ +283,3 @@
> >    console.log("Got stats callback.");
> > +  globalReport.reports.forEach(function (report) {
> > +    var pcid = report.pcid;
> 
> pcid used once. Inline?
> 

   Sure.

> @@ +296,5 @@
> > +    }
> > +  });
> > +
> > +  globalReport.errors.forEach(function (error) {
> > +    var pcid = error.pcid;
> 
> Used once. Inline?
> 

   Sure.

> @@ +303,5 @@
> > +    var pcDiv = document.getElementById(pcDivHeading);
> > +    var newPcDiv = buildPcDiv(error, pcDivHeading);
> > +    newPcDiv.id = pcDivHeading;
> > +
> > +    if (!pcDiv) {
> 
> flip?

   Why not.

Comment 69

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Incorporate feedback.

Comment 70

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=9b9ca99e854c

Comment 71

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

(In reply to Byron Campen [:bwc] from comment #68)
>    This is how it was before, only moved. But, I'll attempt to change it.

You're right, not a blocker then. Thanks for considering.

> > I don't see anything in here that should require the mInternal indirection,
> > just a bunch of RefPtrs. PeerConnectionImpl.h is already full of RefPtrs to
> > forward-declared types. e.g. mozilla::RefPtr<DtlsIdentity> , wont that work
> > for mozilla::MediaPipeline and mozilla::NrIceMediaStream? Then we can avoid
> > this ugly indirection throughout.
> 
>    I tried that, and it doesn't work. PeerConnectionImpl.cpp is picking up
> the necessary includes that make it possible to create an RTCStatsQuery
> without the compiler complaining. WebrtcGlobalInformation.cpp does not.

Thanks for explaining. RefPtr<> needs to know MediaPipeline and NrIceMediaStream are refcounted.

Why not pull in the includes for MediaPipeline and NrIceMediaStream then? PeerConnectionImpl.h has already been pair down and is pulled in by the binding code already i.e. works. Seems the lesser evil than ->mInternal-> everything.

> > > +  query->mInternal = new RTCStatsQuery::Internal;
> > > +  query->mInternal->internalStats = internalStats;
> > 
> > Avoiding constructor?
> 
> I'm not sure what you mean. Are you proposing adding a c'tor that takes
> this bool?

Yes, it's an invariant. Just an observation if we keep this pattern, which I hope we wont have to.

> > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> > @@ +169,5 @@
> > > +  private:
> > > +    friend class PeerConnectionImpl;
> > > +    // This contains a bunch of internals; don't force the user to include
> > > +    // stuff like MediaPipeline.h etc.
> > > +    Internal* mInternal;
> > 
> > Did you figure out why you couldn't use
> > mozilla::ScopedDeletePtr<Internal> mInternal; ?
> 
>    It all hinges on whether the code that creates the type knows what the
> type is. Which WebrtcGlobalInformation.cpp does not.

Right, I worry we're trading one ill for another here. This is a direction issue, and I think we should move toward a code landscape where RAII is trivial, not hard.

> > if (NS_FAILED(rv)) {
> 
>    What circumstances will yield a failure result, but still return non
> null? I've already been bitten by stuff returning nullptr without setting an
> error, so to feel comfortable with doing this I'd need to spend some time
> reading the code involved here. Or I could just check both...

Your code is bitten right now by a nullptr returned without setting an error, because you return an uninitialized rv in that case. A scratch, not flesh wound though.

But yeah, manual error-handling blows. MOZ_ASSERT?

> > I actually like the nsAutoPtr<nsRefPtr<>> idea a lot. We would need to be
> > solidly sure about STS shutdown never freeing unprocessed events or it might
> > free the reference on the wrong thread rather than leak it should things go
> > south before the roundtrip completes (I'm pretty sure it's fine). I suppose
> > we would have run across shutdown leaks by now if that wasn't tight. But
> > this is fine as well.
> 
>    Yeah, this was exactly what I was worried about; if this goes out of
> scope on STS, we'll hit a threadsafety assertion on debug builds, tweak a
> non-threadsafe refcount on non-debug builds, and if we don't end up horribly
> in the weeds due to that, destroy the callback on the wrong thread. It seems
> to me that leaking is preferable.

Where's your sense of adventure? ;-) Yeah maybe not.

> > exceptionSafety seems like a misnomer since there are no exceptions
> > involved. How about callback?
> 
>    If you're confident there is no possibility of exceptions, this can be
> removed entirely, and we can just call AddRef() directly.

No, the code is sound, if NS_FAILED then you mustn't forget. Fail it may, exception no. Just a naming thing.

> > I vote we not show closed peerConnections at all, especially since it's hard
> > to discern whether they're still in use by a page or not.
> 
>    Hmm. I don't know. I could see it being useful for detecting that garbage
> collection didn't successfully collect a specific PC,

I closed the tab, so any failure to collect would be a browser bug. We have leak tools for that.

> or if some page is holding onto closed PCs for no good reason.

How do you know they have no good reasons? Besides, isn't a closed PC mostly harmless?

In any case, I claim what I did was unexceptional, so an error seems wrong. If there's utility here for debugging I would open a new bug to address that in a way that isn't tripped up by closing a tab.

>    I'd choose wasReportedByRemote; that's maximally clear to everyone
> whether they know what RTCP is or not.

If they know RTP I think they know RTCP, but I'll leave it up to you. :-)

Comment 72

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #71)
> 
> Thanks for explaining. RefPtr<> needs to know MediaPipeline and
> NrIceMediaStream are refcounted.
> 
> Why not pull in the includes for MediaPipeline and NrIceMediaStream then?
> PeerConnectionImpl.h has already been pair down and is pulled in by the
> binding code already i.e. works. Seems the lesser evil than ->mInternal->
> everything.
> 

   I feel the opposite way. I have a feeling that we're going to need to modify this stuff in Internal with some frequency, and this encapsulates that so other parts of the system don't need to deal with the churn. If RefPtr and friends could be induced to not throw compiler warnings when the variables were in a private section of a class with a declared destructor, it would be another matter.

> > I'm not sure what you mean. Are you proposing adding a c'tor that takes
> > this bool?
> 
> Yes, it's an invariant. Just an observation if we keep this pattern, which I
> hope we wont have to.
>

   I guess we could do that.

> > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
> > > Did you figure out why you couldn't use
> > > mozilla::ScopedDeletePtr<Internal> mInternal; ?
> > 
> >    It all hinges on whether the code that creates the type knows what the
> > type is. Which WebrtcGlobalInformation.cpp does not.
> 
> Right, I worry we're trading one ill for another here. This is a direction
> issue, and I think we should move toward a code landscape where RAII is
> trivial, not hard.
> 

   Actually... maybe declaring a c'tor too is what we need to make the compiler warnings go away... need to try that. It may be that we need a c'tor symbol defined somewhere that knows about the type, since exceptions in a c'tor cause teardown without calling the d'tor. I'll give this a go.

> Your code is bitten right now by a nullptr returned without setting an
> error, because you return an uninitialized rv in that case. A scratch, not
> flesh wound though.
> 
> But yeah, manual error-handling blows. MOZ_ASSERT?
>

   I've just gone ahead and checked both.

> > > exceptionSafety seems like a misnomer since there are no exceptions
> > > involved. How about callback?
> > 
> >    If you're confident there is no possibility of exceptions, this can be
> > removed entirely, and we can just call AddRef() directly.
> 
> No, the code is sound, if NS_FAILED then you mustn't forget. Fail it may,
> exception no. Just a naming thing.
> 

   I've removed a couple of instances of this, and renamed a couple more to "ensureFree", since that is what they are for.

> > > I vote we not show closed peerConnections at all, especially since it's hard
> > > to discern whether they're still in use by a page or not.
> > 
> >    Hmm. I don't know. I could see it being useful for detecting that garbage
> > collection didn't successfully collect a specific PC,
> 
> I closed the tab, so any failure to collect would be a browser bug. We have
> leak tools for that.
> 
> > or if some page is holding onto closed PCs for no good reason.
> 
> How do you know they have no good reasons? Besides, isn't a closed PC mostly
> harmless?

   If I were a webapp developer, I would like to see this information. (It is also presently the only way we have to test that errors are actually reported, because the only other error condition we have right now is a failure to get a timestamp, which is a hard-to-trigger condition. Kinda sad.)

 
> >    I'd choose wasReportedByRemote; that's maximally clear to everyone
> > whether they know what RTCP is or not.
> 
> If they know RTP I think they know RTCP, but I'll leave it up to you. :-)

   Well, this is something that needs to be brought to w3c before we change it, so I'm going to leave it be for now.

Comment 73

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Found the right way to avoid some compiler warnings, and could delete some code.

Comment 74

[Byron Campen [:bwc]]

Comment on attachment 8380031 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Here we go, best of both worlds.

Comment 75

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=ce1c8028f7dc

Comment 76

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

Comment on attachment 8380031 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

(In reply to Byron Campen [:bwc] from comment #72)
> > > > I vote we not show closed peerConnections at all, especially since it's hard
> > > > to discern whether they're still in use by a page or not.
> > > 
> > >    Hmm. I don't know. I could see it being useful for detecting that garbage
> > > collection didn't successfully collect a specific PC,
> > 
> > I closed the tab, so any failure to collect would be a browser bug. We have
> > leak tools for that.
> > 
> > > or if some page is holding onto closed PCs for no good reason.
> > 
> > How do you know they have no good reasons? Besides, isn't a closed PC mostly
> > harmless?
> 
>    If I were a webapp developer, I would like to see this information. (It
> is also presently the only way we have to test that errors are actually
> reported, because the only other error condition we have right now is a
> failure to get a timestamp, which is a hard-to-trigger condition. Kinda sad.)

At best, not being able to deliver stats is an internal "out-of-bound" error (as opposed to an error saying "connection failed"), and belongs in browser console. But I don't think this is an error at all. To me, expecting closed PCs to have stats is the error.

As for testing, we should test the error mechanism with mochitests if we aren't already, and not reason such things into the UI.

I oppose showing "Error" to the end-user for just closing a tab, as this might flood support ("my about: page says Error"). Except for dev console, I don't see Firefox saying "Error" a lot to its users. For instance, about:memory says "Warning" when memory goes bonkers! (I realize we have log-spew in about:webrtc, but logs are opaque to most people).

I vote we throw errors gathering stats into browser console, and show closed PCs as "closed" or not show them at all.

r+ with nit below, but escalating to Gavin to decide on remaining issue.

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +222,5 @@
> +  // (CallbackObject does not use a threadsafe reference count)
> +  // We also cannot use weak pointer semantics, since those aren't
> +  // supported on CallbackObject. We could instead use something like
> +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.
> +  aLoggingCallback.AddRef();

Hmm, I didn't mean to make you descend to AddRef/Release, but I suppose there's something to be said for being honest about what's going on here.

@@ +227,5 @@
> +
> +  rv = RUN_ON_THREAD(
> +    stsThread,
> +    WrapRunnableNM(&GetLogging_s,
> +      already_AddRefed<WebrtcGlobalLoggingCallback>(&aLoggingCallback),

Casting to already_AddRefed sorta defeats the purpose of it I think, but I agree there's no safe handoff that'll look pretty here.

I was thinking of something like:

  nsRefPtr<WebrtcGlobalLoggingCallback> callback(&aLoggingCallback);

  already_AddRefed<WebrtcGlobalLoggingCallback> handoff = callback.forget();

  rv = RUN_ON_THREAD(stsThread,
                     WrapRunnableNM(&GetLogging_s, handoff, pattern),
                     NS_DISPATCH_NORMAL);

  if (NS_FAILED(rv)) {
      callback = handoff;
  }

Verbose but no casting.

Also, splinter inter-diff didn't pick up this file as having changed (maybe because it's new?) If you changed files other than PeerConnectionImpl.cpp|h and this one, please point them out to me. It doesn't look like you did.

Comment 77

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

My already_AddRefed advice was bad. Use MainThreadPtrHandle from Bug 968836 comment 19.

Comment 78

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #76)
> Comment on attachment 8380031 [details] [diff] [review]
> Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> Review of attachment 8380031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As for testing, we should test the error mechanism with mochitests if we
> aren't already, and not reason such things into the UI.

   This is not possible to test with a mochitest; we do not have the ability to artificially inject failures into the c++ code that handles the stats dispatch. It is also impossible to unit-test in principle, because we can't even compile any of the stats stuff without MOZILLA_INTERNAL_API.

> 
> I oppose showing "Error" to the end-user for just closing a tab, as this
> might flood support ("my about: page says Error"). Except for dev console, I
> don't see Firefox saying "Error" a lot to its users. For instance,
> about:memory says "Warning" when memory goes bonkers! (I realize we have
> log-spew in about:webrtc, but logs are opaque to most people).
> 
> I vote we throw errors gathering stats into browser console, and show closed
> PCs as "closed" or not show them at all.

   So, if an error is encountered on a PeerConnection, are you voting that nothing regarding that PeerConnection should show up in about:webrtc? Or that it gets just a heading but no data? I could maybe get behind the latter, but my criterion is that it should be immediately apparent that something is wrong without needing to open up the dev console. I feel like a one-liner error message is very nice to have in this scenario.

   As for treating "closed" like an error, I guess I can see the downside. Let me see if I can convince myself that "closed properly" is distinguishable from "bizarre state that the PC should never be in".

> 
> r+ with nit below, but escalating to Gavin to decide on remaining issue.
> 
> ::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
> @@ +222,5 @@
> > +  // (CallbackObject does not use a threadsafe reference count)
> > +  // We also cannot use weak pointer semantics, since those aren't
> > +  // supported on CallbackObject. We could instead use something like
> > +  // nsAutoPtr<nsRefPtr<>>, but this way seems a little less ugly. YMMV.
> > +  aLoggingCallback.AddRef();
> 
> Hmm, I didn't mean to make you descend to AddRef/Release, but I suppose
> there's something to be said for being honest about what's going on here.
> 

   Yeah, this is a pig. No lipstick should be applied, I think.

> Also, splinter inter-diff didn't pick up this file as having changed (maybe
> because it's new?) If you changed files other than PeerConnectionImpl.cpp|h
> and this one, please point them out to me. It doesn't look like you did.

Here's the interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8379328&action=interdiff&newid=8380031&headers=1

Comment 79

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #77)
> My already_AddRefed advice was bad. Use MainThreadPtrHandle from Bug 968836
> comment 19.

I'll look into this.

Comment 80

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Don't throw errors for PCs that were closed normally, but still do sanity-checking on the stuff that is supposed to be set if not closed, just in case. Also, use nsMainThreadPtrHandle for keeping callback objects alive.

Comment 81

[Byron Campen [:bwc]]

Comment on attachment 8381800 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

How's this?

Comment 82

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=da3d17d7dc18

Comment 83

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

(In reply to Byron Campen [:bwc] from comment #78)
>    This is not possible to test with a mochitest; we do not have the ability
> to artificially inject failures into the c++ code that handles the stats
> dispatch.

These look like internal failings of the stats-gathering attempt itself you're collecting, stuff we can't readily reproduce because it shouldn't fail: GetTimeFromEpoch() wont fail, and IceCtx and the thread will be there, unless something is really wrong. If these fail you have nothing to display IMHO. We have fuzzing for this.

>    So, if an error is encountered on a PeerConnection, are you voting that
> nothing regarding that PeerConnection should show up in about:webrtc? Or
> that it gets just a heading but no data?

The latter. If this was an image drawing routine that failed, I wouldn't expect the error to show in place of the image, I would just expect the image to be absent. Failure to display data should fail to display, unless you're in a console, which this is not (i.e. don't put stderr to stdout). This is a UI first, and I would use CSFLogError here.

Comment 84

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

Comment on attachment 8381800 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Looks good, but I'll take an interdiff again if you have it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1745,5 @@
>    MOZ_ASSERT(!rv.Failed());
>  }
>  
> +bool
> +PeerConnectionImpl::Closed() const

isClosed?

Comment 85

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #84)
> Comment on attachment 8381800 [details] [diff] [review]
> Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> Review of attachment 8381800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'll take an interdiff again if you have it.
> 

https://bugzilla.mozilla.org/attachment.cgi?oldid=8380031&action=interdiff&newid=8381800&headers=1

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1745,5 @@
> >    MOZ_ASSERT(!rv.Failed());
> >  }
> >  
> > +bool
> > +PeerConnectionImpl::Closed() const
> 
> isClosed?

That could work too. I'll make the change once my unrot finishes compiling.

Comment 86

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot, and remove error reports from WebrtcGlobalInformation, since none of the errors we check for are really worth exposing.

Comment 87

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Rename a function

Comment 88

[Byron Campen [:bwc]]

Attachment: Part 3. (unrot of old patch for interdiff, do not commit) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot old patch so we can interdiff the new one.

Comment 89

[Byron Campen [:bwc]]

Comment on attachment 8382434 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Interdiff here: https://bugzilla.mozilla.org/attachment.cgi?oldid=8382435&action=interdiff&newid=8382434&headers=1

Comment 90

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=3fe6de17bbad

Comment 91

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

Comment on attachment 8382434 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

lgtm.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1935,4 @@
>      RTCStatsQuery *query) {
>  
> +  if (!mMedia) {
> +    // Closed.

if (IsClosed()) { would be self-documenting

@@ +1945,5 @@
>    query->report = RTCStatsReportInternalConstruct(
>        NS_ConvertASCIItoUTF16(mName.c_str()),
>        query->now);
>  
> +  // Error check after setting the PC id in the report.

This consideration seems unnecessary now.

Comment 92

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot, and fix a lifecycle bug.

Comment 93

[Byron Campen [:bwc]]

Attachment: Part 3. (another unrot of an old patch for interdiff, do not commit) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot old patch for interdiff.

Comment 94

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Fix a couple of nits too.

Comment 95

[Byron Campen [:bwc]]

Comment on attachment 8383206 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Sorry to do this, but I found a lifecycle bug that needs review (your nits are in here too).

Here's the interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8383189&action=interdiff&newid=8383206&headers=1

And try: https://tbpl.mozilla.org/?tree=Try&rev=18a5a8b386d1

Comment 96

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

Comment on attachment 8383206 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

(In reply to Byron Campen [:bwc] from comment #95)
> Sorry to do this, but I found a lifecycle bug that needs review (your nits
> are in here too).

Sorry for not catching it. NrIceCtx has a whacky life, created on main, destroyed on STS, but I didn't catch that it had to be. I may have vaguely known that at some point but not why.

I don't see a thread assert in NrIceCtx::~NrIceCtx(). Can we add one, assuming the requirement is real?

r+ with nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2089,5 @@
>    ASSERT_ON_THREAD(query->iceCtx->thread());
>  
> +  // NrIceCtx must be destroyed on STS, so it is not safe to dispatch it back
> +  // to main.
> +  RefPtr<NrIceCtx> iceCtxTmp(query->iceCtx);

You can remove this line as it thankfully does nothing.

@@ +2090,5 @@
>  
> +  // NrIceCtx must be destroyed on STS, so it is not safe to dispatch it back
> +  // to main.
> +  RefPtr<NrIceCtx> iceCtxTmp(query->iceCtx);
> +  query->iceCtx = nullptr;

Sorry I didn't notice this earlier in my review of Part 2, but coding style for member variables in PeerConnectionImpl.h is:

  query->mIceCtx = nullptr; 

PS: Don't forget to add r= to commit msg and bug description whenever carrying forward r+ so it is apparent who the reviewer is (after some digging in Part 2 I found it was me :-)

Comment 97

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

Comment on attachment 8379096 [details] [diff] [review]
Part 2. Simplify the statistics accessors, and rename things to better describe what they do. r=jib

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +166,5 @@
> +    mozilla::RefPtr<NrIceCtx> iceCtx;
> +    std::vector<mozilla::RefPtr<NrIceMediaStream>> streams;
> +    DOMHighResTimeStamp now;
> +    mozilla::dom::RTCStatsReportInternal report;
> +    nsCOMPtr<nsIThread> mainThread;

Style should really be mPcName, mInternalStats, mPilelines, mIceCtx etc.

Comment 98

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #96)
> Comment on attachment 8383206 [details] [diff] [review]
> Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing
> logging-related stuff from PeerConnectionImpl.
> 
> Review of attachment 8383206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Byron Campen [:bwc] from comment #95)
> > Sorry to do this, but I found a lifecycle bug that needs review (your nits
> > are in here too).
> 
> Sorry for not catching it. NrIceCtx has a whacky life, created on main,
> destroyed on STS, but I didn't catch that it had to be. I may have vaguely
> known that at some point but not why.
> 
> I don't see a thread assert in NrIceCtx::~NrIceCtx(). Can we add one,
> assuming the requirement is real?
> 
> r+ with nits.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +2089,5 @@
> >    ASSERT_ON_THREAD(query->iceCtx->thread());
> >  
> > +  // NrIceCtx must be destroyed on STS, so it is not safe to dispatch it back
> > +  // to main.
> > +  RefPtr<NrIceCtx> iceCtxTmp(query->iceCtx);
> 
> You can remove this line as it thankfully does nothing.

   Actually, we need to keep it alive because the NrIceMediaStreams rely on it, and don't hold their own references (yikes!).

> 
> @@ +2090,5 @@
> >  
> > +  // NrIceCtx must be destroyed on STS, so it is not safe to dispatch it back
> > +  // to main.
> > +  RefPtr<NrIceCtx> iceCtxTmp(query->iceCtx);
> > +  query->iceCtx = nullptr;
> 
> Sorry I didn't notice this earlier in my review of Part 2, but coding style
> for member variables in PeerConnectionImpl.h is:
> 
>   query->mIceCtx = nullptr; 
> 
> PS: Don't forget to add r= to commit msg and bug description whenever
> carrying forward r+ so it is apparent who the reviewer is (after some
> digging in Part 2 I found it was me :-)

   Yeah, that's something I've been doing as I update patches.

Comment 99

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

(In reply to Byron Campen [:bwc] from comment #98)
> > > +  RefPtr<NrIceCtx> iceCtxTmp(query->iceCtx);
> > 
> > You can remove this line as it thankfully does nothing.
> 
>    Actually, we need to keep it alive because the NrIceMediaStreams rely on
> it, and don't hold their own references (yikes!).

But it is held open by mMedia here http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h#377 and you verify mMedia is open above. Since you're not using iceCtxTmp here there should be no need.

Comment 100

[Byron Campen [:bwc]]

> But it is held open by mMedia here
> http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> peerconnection/PeerConnectionMedia.h#377 and you verify mMedia is open
> above. Since you're not using iceCtxTmp here there should be no need.

   We verify that mMedia is non-null while building the query, but we do not retain any reference to the PeerConnectionImpl; both the PeerConnectionImpl and PeerConnectionMedia could be gone by the time we make it to STS.

Comment 101

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

Oh right, this is on sts. Fair enough then, except it's a little disconcerting to think this code may run after mMedia is shut down. That means all of these things are gone: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#354 - Do we know it works?

Comment 102

[Byron Campen [:bwc]]

Yeah; the telemetry patch I'm working on fires off a stats dispatch inside CloseInt(), to get the final state of things to record in telemetry. We're retaining references to everything we need, as far as I can tell.

Comment 103

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

OK.

Comment 104

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Incorporate more feedback.

Comment 105

[Byron Campen [:bwc]]

Comment on attachment 8384739 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

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

Carry forward r=jib

Comment 106

[Byron Campen [:bwc]]

Attachment: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl. r=jib

Unrot.

Comment 107

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=964831b4c1da

Comment 108

[Byron Campen [:bwc]]

Comment on attachment 8384743 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl. r=jib

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

Carry forward r=jib

Comment 109

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23bebfef70a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e8301cb71e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdd9dc14f1e

Comment 110

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23bebfef70a2
https://hg.mozilla.org/mozilla-central/rev/95e8301cb71e
https://hg.mozilla.org/mozilla-central/rev/5cdd9dc14f1e