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

RESOLVED FIXED in mozilla30

Status

()

Core
WebRTC: Networking
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 26 obsolete attachments)

9.59 KB, patch
jib
: review+
Details | Diff | Splinter Review
24.72 KB, patch
bwc
: review+
Details | Diff | Splinter Review
54.01 KB, patch
bwc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(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.
(Assignee)

Comment 2

4 years ago
(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.
(Assignee)

Updated

4 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created 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.

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

Comment 4

4 years ago
Created attachment 8365410 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8365361 - Flags: review?(jib)
(Assignee)

Updated

4 years ago
Attachment #8365410 - Flags: review?(jib)
(Assignee)

Updated

4 years ago
Blocks: 964161
(Assignee)

Comment 6

4 years ago
Created attachment 8366271 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8365410 - Attachment is obsolete: true
Attachment #8365410 - Flags: review?(jib)
(Assignee)

Updated

4 years ago
Attachment #8366271 - Flags: review?(jib)
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.
Attachment #8365361 - Flags: review?(jib) → review+
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.
Attachment #8366271 - Flags: review?(jib) → review+
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

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

Incorporate feedback.
(Assignee)

Updated

4 years ago
Attachment #8366271 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
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.
Attachment #8366747 - Flags: review+
(Assignee)

Comment 13

4 years ago
Created attachment 8367512 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8366747 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
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.
Attachment #8367512 - Flags: review+
(Assignee)

Comment 15

4 years ago
Needinfo myself to push to try once it reopens.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 16

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7acdb018ae10
Flags: needinfo?(docfaraday)
(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.
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
Created attachment 8371712 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Moved the impl of WebrtcGlobalInformation to c++.
(Assignee)

Comment 20

4 years ago
Created attachment 8372367 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8371712 - Attachment is obsolete: true
(Assignee)

Comment 21

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

Remove obsolete param.
(Assignee)

Updated

4 years ago
Attachment #8372367 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
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.
Attachment #8372369 - Flags: review?(continuation)
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. :)
Attachment #8372369 - Flags: review?(continuation) → review?(bugs)
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.
Attachment #8372369 - Flags: review?(bugs) → review-
(Assignee)

Comment 25

4 years ago
(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.
(Assignee)

Comment 26

4 years ago
Created attachment 8372661 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8372369 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
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?
Attachment #8372661 - Flags: feedback?(bugs)
Comment on attachment 8372661 [details] [diff] [review]
Part 3. (WIP) New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.


>+// TODO(bcampen@mozilla.com): 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?
Attachment #8372661 - Flags: feedback?(bugs)
(Assignee)

Comment 29

4 years ago
(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(bcampen@mozilla.com): 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.
(Assignee)

Comment 30

4 years ago
Created attachment 8373694 [details] [diff] [review]
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!
(Assignee)

Updated

4 years ago
Attachment #8372661 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
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.
Attachment #8373694 - Flags: feedback?(bugs)
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.
Attachment #8373694 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 33

4 years ago
(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.
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.
(Assignee)

Comment 35

4 years ago
(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).
bryon: What are the requirements you have here?
(Assignee)

Comment 37

4 years ago
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).
(Assignee)

Comment 38

4 years ago
Created attachment 8374568 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8373694 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
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.
(Assignee)

Comment 40

4 years ago
Created attachment 8374989 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8374568 - Attachment is obsolete: true
(Assignee)

Comment 41

4 years ago
Created attachment 8375078 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8374989 - Attachment is obsolete: true
(Assignee)

Comment 42

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

Ok, enough polish. Let's get this reviewed.
(Assignee)

Updated

4 years ago
Attachment #8375078 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8377813 - Flags: review?(bugs)
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).
Attachment #8377813 - Flags: review?(bugs) → review+
(Assignee)

Comment 44

4 years ago
(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.
(Assignee)

Comment 45

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

Incorporate feedback.
(Assignee)

Updated

4 years ago
Attachment #8377813 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
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
Attachment #8378624 - Flags: review+
(Assignee)

Comment 47

4 years ago
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
Attachment #8378624 - Flags: review?(adam)
(Assignee)

Comment 49

4 years ago
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.
Attachment #8365361 - Flags: checkin?(adam)
(Assignee)

Comment 50

4 years ago
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.
Attachment #8367512 - Flags: checkin?(adam)
(Assignee)

Updated

4 years ago
Attachment #8378624 - Flags: review?(adam) → checkin?(adam)
(Assignee)

Comment 51

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

Butterfingers...
Attachment #8378624 - Flags: checkin?(adam) → review?(adam)
Part 2 has conflicts on inbound. Also, we're back to using individual checkin? requests again? :(
Attachment #8365361 - Flags: checkin?(adam)
Attachment #8367512 - Flags: checkin?(adam)
(Assignee)

Comment 53

4 years ago
(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.
(Assignee)

Comment 54

4 years ago
Wow. These got rotted so bad that I'm going to have to start over.
(Assignee)

Comment 55

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

Unrot.
(Assignee)

Updated

4 years ago
Attachment #8367512 - Attachment is obsolete: true
(Assignee)

Comment 56

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

Unrot.
(Assignee)

Updated

4 years ago
Attachment #8378624 - Attachment is obsolete: true
Attachment #8378624 - Flags: review?(adam)
(Assignee)

Comment 58

4 years ago
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)
Attachment #8379096 - Flags: review+
(Assignee)

Comment 59

4 years ago
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?
Attachment #8379097 - Flags: review?(jib)
(Assignee)

Comment 60

4 years ago
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)
Attachment #8379097 - Flags: review+
(Assignee)

Comment 61

4 years ago
Needinfo myself to keep an eye on the try push.
(Assignee)

Updated

4 years ago
Flags: needinfo?(docfaraday)
Sure, looking now.
(Assignee)

Updated

4 years ago
Blocks: 970690
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.
Attachment #8379097 - Flags: review?(jib) → review-
(Assignee)

Comment 64

4 years ago
(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.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 65

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

Incorportate some feedback from jib.
(Assignee)

Updated

4 years ago
Attachment #8379097 - Attachment is obsolete: true
(Assignee)

Comment 66

4 years ago
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.
Attachment #8379328 - Flags: review?(jib)
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(bcampen@mozilla.com): Maybe put something like 

trailing whitespace.

@@ +259,5 @@
> +      // statistics look like from the perspective of the other end.
> +      // TODO(bcampen@mozilla.com): 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?
Attachment #8379328 - Flags: review?(jib) → review-
(Assignee)

Comment 68

4 years ago
(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(bcampen@mozilla.com): 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.
(Assignee)

Comment 69

4 years ago
Created attachment 8379855 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Incorporate feedback.
(Assignee)

Updated

4 years ago
Attachment #8379328 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8379855 - Flags: review?(jib)
(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. :-)
(Assignee)

Comment 72

4 years ago
(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.
(Assignee)

Comment 73

4 years ago
Created attachment 8380031 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8379855 - Attachment is obsolete: true
Attachment #8379855 - Flags: review?(jib)
(Assignee)

Comment 74

4 years ago
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.
Attachment #8380031 - Flags: review?(jib)
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.
Attachment #8380031 - Flags: review?(jib)
Attachment #8380031 - Flags: review?(gavin.sharp)
Attachment #8380031 - Flags: review+
My already_AddRefed advice was bad. Use MainThreadPtrHandle from Bug 968836 comment 19.
(Assignee)

Comment 78

4 years ago
(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
(Assignee)

Comment 79

4 years ago
(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.
(Assignee)

Comment 80

4 years ago
Created attachment 8381800 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8380031 - Attachment is obsolete: true
Attachment #8380031 - Flags: review?(gavin.sharp)
(Assignee)

Comment 81

4 years ago
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?
Attachment #8381800 - Flags: review?(jib)
(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 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?
(Assignee)

Comment 85

4 years ago
(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.
(Assignee)

Comment 86

4 years ago
Created attachment 8382416 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8381800 - Attachment is obsolete: true
Attachment #8381800 - Flags: review?(jib)
(Assignee)

Comment 87

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

Rename a function
(Assignee)

Updated

4 years ago
Attachment #8382416 - Attachment is obsolete: true
(Assignee)

Comment 88

4 years ago
Created attachment 8382435 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8382435 - Attachment is obsolete: true
(Assignee)

Comment 89

4 years ago
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
Attachment #8382434 - Flags: review?(jib)
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.
Attachment #8382434 - Flags: review?(jib) → review+
(Assignee)

Comment 92

4 years ago
Created attachment 8383186 [details] [diff] [review]
Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl.

Unrot, and fix a lifecycle bug.
(Assignee)

Updated

4 years ago
Attachment #8382434 - Attachment is obsolete: true
(Assignee)

Comment 93

4 years ago
Created attachment 8383189 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8383189 - Attachment is obsolete: true
(Assignee)

Comment 94

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

Fix a couple of nits too.
(Assignee)

Updated

4 years ago
Attachment #8383186 - Attachment is obsolete: true
(Assignee)

Comment 95

4 years ago
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
Attachment #8383206 - Flags: review?(jib)
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 :-)
Attachment #8383206 - Flags: review?(jib) → review+
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.
(Assignee)

Comment 98

4 years ago
(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.
Attachment #8379096 - Attachment description: Part 2. Simplify the statistics accessors, and rename things to better describe what they do. → Part 2. Simplify the statistics accessors, and rename things to better describe what they do. r=jib
(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.
(Assignee)

Comment 100

4 years ago
> 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.
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?
(Assignee)

Comment 102

4 years ago
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.
(Assignee)

Comment 104

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

Incorporate more feedback.
(Assignee)

Updated

4 years ago
Attachment #8383206 - Attachment is obsolete: true
(Assignee)

Comment 105

4 years ago
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
Attachment #8384739 - Flags: review+
(Assignee)

Comment 106

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

Unrot.
(Assignee)

Updated

4 years ago
Attachment #8384739 - Attachment is obsolete: true
(Assignee)

Comment 108

4 years ago
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
Attachment #8384743 - Flags: review+
Attachment #8384743 - Attachment description: Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl. → Part 3. New webidl for WebrtcGlobalInformation, c++ impl, and removing logging-related stuff from PeerConnectionImpl. r=jib
(Assignee)

Updated

4 years ago
Blocks: 979471
(Assignee)

Updated

4 years ago
Keywords: checkin-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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.