about:webrtc should have a clear screen button

RESOLVED FIXED in Firefox 47

Status

()

Core
WebRTC
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jared, Assigned: pkerr)

Tracking

({privacy})

unspecified
mozilla47
privacy
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Also the Forget button should forget the appropriate about:webrtc entries.  Also, data should be purged from the Connection log, not just the PeerConnections
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 15
Ever confirmed: true
Priority: -- → P1
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.
Assignee: nobody → jib
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.
Flags: needinfo?(pkerr)
Summary: about:webrtc should have a clear history button → about:webrtc should have a clear screen button
(Assignee)

Updated

3 years ago
Assignee: jib → pkerr
Flags: needinfo?(pkerr)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Depends on: 1221786
(Assignee)

Comment 4

3 years ago
Created attachment 8698656 [details] [diff] [review]
add buttons to clear session and signaling logs
(Assignee)

Updated

3 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 5

3 years ago
Created attachment 8699011 [details] [diff] [review]
add buttons to clear session and signaling logs

clean up whitespace
(Assignee)

Updated

3 years ago
Attachment #8698656 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 7

2 years ago
Created attachment 8712221 [details] [diff] [review]
add buttons to clear session and signaling logs

Removed un-intended dependency on patch for bug 1221786
(Assignee)

Updated

2 years ago
Attachment #8699011 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
No longer depends on: 1221786
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

2 years ago
> 
> @@ +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.
(Assignee)

Comment 11

2 years ago
Created attachment 8712365 [details] [diff] [review]
add buttons to clear session and signaling logs

Revise based on review comments
(Assignee)

Updated

2 years ago
Attachment #8712221 - Attachment is obsolete: true
(Assignee)

Comment 12

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

Comment 16

2 years ago
Created attachment 8712939 [details] [diff] [review]
add buttons to clear session and signaling logs
(Assignee)

Updated

2 years ago
Attachment #8712365 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 18

2 years ago
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.
Attachment #8712939 - Flags: review+ → review?(jonas)

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2838a45f3f17
https://hg.mozilla.org/mozilla-central/rev/d81da0ef528d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.