1209252 - about:webrtc should have a clear screen button

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[jared]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

navigate to about:webrtc 


Actual results:

all webrtc sessions are listed


Expected results:

It would be nice to clear previous webrtc sessions that are closed.

Comment 1

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

Also the Forget button should forget the appropriate about:webrtc entries.  Also, data should be purged from the Connection log, not just the PeerConnections

Comment 2

[Maire Reavy [:mreavy]]

Hey jib - can you put this towards the top of your queue?  This is a privacy issue.  Thanks.  I'd love to get this into 45 if we can.

Comment 3

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

Paul can probably do this a lot faster. Is he busy?

As I recall (since I urged the reporter to file this if I remember correctly) this request wasn't about privacy, since none of the about:webrtc data is persisted to disk AFAIK, but rather about regaining overview in the about:webrtc page after numerous calls. I therefore equate it more to e.g. the clear button in web console and browser console.

So this is probably a different bug from a full privacy review of about:webrtc.

A privacy review of about:webrtc would likely generate more work than this bug. The fact that about:webrtc includes calls from private browsing mode even after the last private browsing session is closed, comes to mind.

Comment 4

[Paul Kerr [:pkerr]]

Attachment: add buttons to clear session and signaling logs

Comment 5

[Paul Kerr [:pkerr]]

Attachment: add buttons to clear session and signaling logs

clean up whitespace

Comment 6

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

Comment on attachment 8699011 [details] [diff] [review]
add buttons to clear session and signaling logs

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

r- for wrong args to function and compile error (I would love to try it to see how it looks).

Otherwise looks good, just lots of nits.

::: dom/webidl/WebrtcGlobalInformation.webidl
@@ +23,5 @@
>    [Throws]
>    static void getLogging(DOMString pattern,
>                           WebrtcGlobalLoggingCallback callback);
>  
> +  static void clearLogs();

Curious, any reason for Logs vs. Logging?

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +405,5 @@
>    const nsAString& aPcIdFilter,
>    WebrtcGlobalChild* aThisChild,
>    const int aRequestId)
>  {
> +  nsAutoPtr<RTCStatsQueries> queries(new decltype(queries)::element_type);

Looks needlessly complicated. Any reasoning or precedent for this?

My understanding from the style guide is we should use decltype only when necessary.

@@ +444,5 @@
> +WebrtcGlobalInformation::ClearAllStats(
> +  const GlobalObject& aGlobal)
> +{
> +  if (!NS_IsMainThread()) {
> +    return;

Is this ever not a programming error?

@@ +460,5 @@
> +  // Flush the history for the chrome process
> +  ClearClosedStats();
> +
> +  return;
> +}

Superfluous return.

@@ +558,5 @@
> +{
> +  // Make call off main thread.
> +  RLogRingBuffer* logs = RLogRingBuffer::GetInstance();
> +  if (logs) {
> +    logs->Clear();

Compile error: no member named 'Clear' in 'mozilla::RLogRingBuffer'

@@ +561,5 @@
> +  if (logs) {
> +    logs->Clear();
> +  }
> +  return;
> +}

Superfluous return.

@@ +572,5 @@
> +    do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  } else if (!stsThread) {

nit: else is redundant after return

@@ +579,5 @@
> +
> +  rv = RUN_ON_THREAD(stsThread,
> +                     WrapRunnableNM(&ClearLogs_s),
> +                     NS_DISPATCH_NORMAL);
> +  return rv;

Or just: return RUN_ON_THREAD(...)?

@@ +591,5 @@
> +    return;
> +  }
> +
> +  // Chrome-only API
> +  MOZ_ASSERT(XRE_IsParentProcess());

nit: Maybe add same comment to same assert in ClearAllStats(), or remove this one, for consistency?

@@ +603,5 @@
> +
> +  // Clear chrome process signaling logs
> +  Unused << RunLogClear();
> +  return;
> +}

Superfluous return.

@@ +872,5 @@
> +{
> +  if (!mShutdown) {
> +    RunLogClear();
> +  }
> +  return true;

nit: maybe pick one pattern? (compare to RecvClearStatsRequest)

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.h
@@ +46,5 @@
>  private:
>    WebrtcGlobalInformation() = delete;
>    WebrtcGlobalInformation(const WebrtcGlobalInformation& aOrig) = delete;
>    WebrtcGlobalInformation& operator=(
> +  const WebrtcGlobalInformation& aRhs) = delete;

isn't indent here better?

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +25,5 @@
>  const WEBRTC_TRACE_ALL = 65535;
>  
> +function getStats() {
> +  return new Promise(resolve =>
> +    WebrtcGlobalInformation.getAllStats(stats => resolve(stats)));

Maybe not this patch, but we really should fix getAllStats to just return a promise, then we wont need these wrappers.

@@ +31,4 @@
>  
> +function getLog() {
> +  return new Promise(resolve =>
> +    WebrtcGlobalInformation.getLogging("", log => resolve(log)));

Same with getLogging.

@@ +55,5 @@
>      return;
>    }
>  
> +  let contentInit = function(data) {
> +    AboutWebRTC.init(onClearStats, onClearLog);

Why pass in onClearStats and onClearLog when `init` is only ever called from here?

@@ +56,5 @@
>    }
>  
> +  let contentInit = function(data) {
> +    AboutWebRTC.init(onClearStats, onClearLog);
> +    AboutWebRTC.render(content, data);

Design-nit: I find the 'data' conceit confusing. Wouldn't three arguments 'reports', 'logs' and 'error' be simpler?

@@ +61,5 @@
> +  };
> +
> +  Promise.all([reportsRetrieved, logRetrieved]).then(
> +    ([stats, log]) => contentInit({reports: stats.reports, log: log}),
> +    (error) => contentInit({error: error})

Style-nit: no parens around single-arg `error` here. Applies throughout.

(Note: Parens are still needed around destructured arg [stats, log] because of es6, but Brendan told me they'd fix this in es7.)

@@ +62,5 @@
> +
> +  Promise.all([reportsRetrieved, logRetrieved]).then(
> +    ([stats, log]) => contentInit({reports: stats.reports, log: log}),
> +    (error) => contentInit({error: error})
> +  ).catch(() => contentInit([], []));

Here you're passing two arrays to a function that expects one object. Not good.

Also, this catch only ever runs if a previous call to contentInit fails, is that the intention?

@@ +70,5 @@
> +  WebrtcGlobalInformation.clearLogs();
> +  getLog().then(
> +    (log) => AboutWebRTC.refresh({log: log}),
> +    (error) => AboutWebRTC.refresh({logError: error})
> +  );

Promise-chains should always either be returned or terminated with a catch, or errors may get swallowed. This does neither.

Also, inlining these functions at their single call-site might make this more obvious.

My advice: I find I never use the second argument to .then() and always use a final .catch() instead.

@@ +78,5 @@
> +  WebrtcGlobalInformation.clearAllStats();
> +  getStats().then(
> +    (stats) => AboutWebRTC.refresh({reports: stats.reports}),
> +    (error) => AboutWebRTC.refresh({reportError: error})
> +  );

Same here.

Comment 7

[Paul Kerr [:pkerr]]

Attachment: add buttons to clear session and signaling logs

Removed un-intended dependency on patch for bug 1221786

Comment 8

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> Comment on attachment 8699011 [details] [diff] [review]
> ::: dom/webidl/WebrtcGlobalInformation.webidl
> @@ +23,5 @@
> >    [Throws]
> >    static void getLogging(DOMString pattern,
> >                           WebrtcGlobalLoggingCallback callback);
> >  
> > +  static void clearLogs();
> 
> Curious, any reason for Logs vs. Logging?
> 

I felt that clearLogging was a clunky word combination; 'Logging' is a verb in a position that is normally held by a noun. But, they probably should have the same matching root as they target the same entity.

Comment 9

[Paul Kerr [:pkerr]]

> ::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
> @@ +405,5 @@
> >    const nsAString& aPcIdFilter,
> >    WebrtcGlobalChild* aThisChild,
> >    const int aRequestId)
> >  {
> > +  nsAutoPtr<RTCStatsQueries> queries(new decltype(queries)::element_type);
> 
> Looks needlessly complicated. Any reasoning or precedent for this?
> 
> My understanding from the style guide is we should use decltype only when
> necessary.
> 
For the same reason that 'auto' is used: so that the template argument does not need to be repeated in the invocation of 'new'. This allows that any later type changes to be handled in one place.

Comment 10

[Paul Kerr [:pkerr]]

> 
> @@ +444,5 @@
> > +WebrtcGlobalInformation::ClearAllStats(
> > +  const GlobalObject& aGlobal)
> > +{
> > +  if (!NS_IsMainThread()) {
> > +    return;
> 
> Is this ever not a programming error?
> 
I am handling this error condition in a similar manner to the already existing GetAllStats method. I am not sure of the original motivation in that original case.

Comment 11

[Paul Kerr [:pkerr]]

Attachment: add buttons to clear session and signaling logs

Revise based on review comments

Comment 12

[Paul Kerr [:pkerr]]

Comment on attachment 8712365 [details] [diff] [review]
add buttons to clear session and signaling logs

JIB: this should work now. One thing to note when testing: if you clear the upper call stats section of the page using the button and there are any active sessions (those not marked as closed), those sessions will re-appear after clearing because they are still PeerConnectionImpl instances alive and the their status data will be used to generate the stats section again.

The is caused by the fact that the stats are gathered from both PeerConnectinCtx, which returns data on the live sessions, and the previously closed sessions stored by calls to WebrtcGlobalInformation::StoreLongTermICEStatistics. Thus, the clear operation wipes out the stored stats and leaves any live session visible.

Comment 13

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

Comment on attachment 8712365 [details] [diff] [review]
add buttons to clear session and signaling logs

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

r=me with rename fix.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +54,1 @@
>    if (!content) {

Forgot to rename content here.

@@ +61,5 @@
> +  };
> +
> +  Promise.all([reportsRetrieved, logRetrieved]).then(
> +    ([stats, log]) => contentInit({reports: stats.reports, log: log})
> +  ).catch(error => contentInit({error: error}));

It's unusual to see word-wrap on (. Instead we seem to tend to use:

Promise.all([reportsRetrieved, logRetrieved])
  .then(([stats, log]) => contentInit({reports: stats.reports, log: log})
  .catch(error => contentInit({error: error}));

(in 3 places)

::: toolkit/content/jar.mn
@@ +30,5 @@
>     content/global/aboutServiceWorkers.js
>     content/global/aboutServiceWorkers.xhtml
>     content/global/aboutwebrtc/aboutWebrtc.css   (aboutwebrtc/aboutWebrtc.css)
>     content/global/aboutwebrtc/aboutWebrtc.js    (aboutwebrtc/aboutWebrtc.js)
> +   content/global/aboutwebrtc/aboutWebrtc.html (aboutwebrtc/aboutWebrtc.html)

Nit: the ()s at the end used to line up.

Comment 14

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

Comment on attachment 8712365 [details] [diff] [review]
add buttons to clear session and signaling logs

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

::: media/mtransport/rlogringbuffer.h
@@ +98,5 @@
>  
>      void Log(std::string&& log);
>  
> +    void Clear();
> +  

Almost missed an excess whitespace.

Comment 15

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

(In reply to Paul Kerr [:pkerr] from comment #12)
> if you clear the upper call stats section of the page using the button and
> there are any active sessions (those not marked as closed), those sessions
> will re-appear after clearing because they are still PeerConnectionImpl
> instances alive

I see what you mean. It's a little surprising. I saw the timestamps change on the live ones when I hit clear, so maybe people will figure it out.

Comment 16

[Paul Kerr [:pkerr]]

Attachment: add buttons to clear session and signaling logs

Comment 17

[Paul Kerr [:pkerr]]

Comment on attachment 8712939 [details] [diff] [review]
add buttons to clear session and signaling logs

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

Carry forward r+ jib.

Comment 18

[Paul Kerr [:pkerr]]

Comment on attachment 8712939 [details] [diff] [review]
add buttons to clear session and signaling logs

I need a DOM peer review of the changes to the WebrtcGlobalInformation.webidl interface. Two calls have been added to allow the Stats and Logging collections to be cleared. The current consumers of the WebrtcGlobalInformation interface are the about:webrtc page and Hello.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2838a45f3f17

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d81da0ef528d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2838a45f3f17
https://hg.mozilla.org/mozilla-central/rev/d81da0ef528d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2838a45f3f17
https://hg.mozilla.org/mozilla-central/rev/d81da0ef528d