1156313 - Enable auto-uploading of logs

Status: RESOLVED WONTFIX

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Eric Rescorla (:ekr)]

We are still having intermittent but persistent problems with echo. The procedure for reporting the data makes them hard to debug. I propose two changes:

1. Have a button to automatically upload the AEC logs somewhere so one can
just refer to them in bugs.

2. Have a 30ish second lookback buffer so I can just push the "there was echo" button after I hear one and have it engage the mechanism in #1.

Comment 1

[Paul Kerr [:pkerr]]

Attachment: Part 1: WIP about:webrtc upload logs button

Comment 2

[Paul Kerr [:pkerr]]

Attachment: Part 1: WIP about:webrtc upload logs button

Comment 3

[Paul Kerr [:pkerr]]

Attachment: Part 1: WIP about:webrtc upload logs button

record last upload time to prevent re-sending old log files

Comment 4

[Paul Kerr [:pkerr]]

Attachment: Part 1: WIP about:webrtc upload logs button

add streaming gzip for large files

Comment 5

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

Update localization file. Add optional description field prompt.

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

whitespace cleanup

Comment 7

[Paul Kerr [:pkerr]]

Comment on attachment 8607697 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

jib: can you give me some feedback on my usage of promises and Task.jsm in the additions I have made to aboutWebrtc.js to handle file compression.

The final form of the multi-part POST will not be determined until I integrate with a IT supported collection server.

Comment 8

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

Comment on attachment 8607697 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

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

Found no errors so these are just suggestions.

I see no need for generators here over simpler promise-chains, though perhaps some people prefer them (I don't).

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +48,5 @@
> +}
> +
> +OutputListener.prototype = {
> +  _outStream: null,
> +  _callback: null,

These two aren't needed since they're set in the constructor.

@@ +51,5 @@
> +  _outStream: null,
> +  _callback: null,
> +
> +  onStartRequest: function(aRequest, aContext) {
> +    return;

There are spurious return statements at end of functions throughout here.

@@ +94,5 @@
> +      // Completion callback: send back output nsIFile reference or failure code.
> +      if (Components.isSuccessCode(aStatusCode)) {
> +        resolve(outFile);
> +      } else {
> +        reject(aStatusCode);

maybe reject(new Error("" + aStatusCode)); ?

DOM folks keep saying rejections should be error objects, even if nothing enforces this.

@@ +133,5 @@
> +      ControlSet.add(new UploadLogs(uploadUri,
> +                                    savePageCtrl,
> +                                    debugModeCtrl,
> +                                    aecLoggingCtrl));
> +    } catch (e) {}

Why suppress errors? If something goes wrong don't we want to see errors in browser console or web console (wherever it goes for about pages)?

@@ +236,5 @@
> +
> +  this._uploadUri = aUploadUri;
> +  this._savePage = aSavePageCtrl || {getFileNames: function() {return []}};
> +  this._debugMode = aDebugModeCtrl || {getFileNames: function() {return []}};
> +  this._aecLogging = aAecLoggingCtrl || {getFileNames: function() {return []}};

defaults here seem redundant and is dead code.

@@ +262,5 @@
> +UploadLogs.prototype.gzipFiles = Task.async(function* (aFiles) {
> +  let gzips = [];
> +  for (let file of aFiles) {
> +    try {
> +      let gzip = yield gzipCompress(file);

Any reason to use generators over promise-chains here? e.g.

    return Promise.all(aFiles.map(file => gzipFiles(file)));

or

    return aFiles.reduce((p, file) => p.then(() => gzipFiles(file)), Promise.resolve());

if you don't want to do them in parallel.

@@ +265,5 @@
> +    try {
> +      let gzip = yield gzipCompress(file);
> +      gzips.push(gzip);
> +    } catch (e) {
> +      continue;

Since gzipCompress() returns a promise, this try/catch doesn't do much.

@@ +279,5 @@
> +  }
> +
> +  let saveFiles = [];
> +  let debugFiles = []
> +  let aecFiles = [];

This triplet seems to be passed around a bit. Have you considered keeping it in an array? Might collapse some code.

@@ +286,5 @@
> +  this.update();
> +
> +  Task.spawn(function*() {
> +    saveFiles = yield this.gzipFiles(this.newFileFilter(
> +                        this._savePage.getFileNames(), gLastUploadTime));

Not to dismiss generator functions, but is there an advantage here over plain promise-chains?

I ask because it strikes me that these might be done in parallel:

Promise.all([
  this.gzipFiles(this._savePage), 
  this.gzipFiles(this._debugMode),
  this.gzipFiles(this._aecLogging)
])
.then(files => {
  [ saveFiles, debugFiles, aecFiles ] = files;
})

@@ +330,5 @@
> +    this._message = getString("upload_logs_sending");
> +    this.update();
> +
> +    let req = new XMLHttpRequest();
> +    req.open("POST", this._uploadUri);

You should be able to use fetch here.

@@ +331,5 @@
> +    this.update();
> +
> +    let req = new XMLHttpRequest();
> +    req.open("POST", this._uploadUri);
> +    req.onload = (event) => {

Redundant parenthesis

@@ +332,5 @@
> +
> +    let req = new XMLHttpRequest();
> +    req.open("POST", this._uploadUri);
> +    req.onload = (event) => {
> +      if (req.status === 200) {

Style guide says ==

@@ +342,5 @@
> +    };
> +    req.send(formData);
> +
> +    return;
> +  }).catch((error) => {

No parens

@@ +343,5 @@
> +    req.send(formData);
> +
> +    return;
> +  }).catch((error) => {
> +    this._message = formatString("upload_logs_error", [error], 1);

what type is error here?

@@ +345,5 @@
> +    return;
> +  }).catch((error) => {
> +    this._message = formatString("upload_logs_error", [error], 1);
> +    this.update();
> +    return;

(All these returns are confusing)

@@ +375,5 @@
> +  let fp = new FilePicker();
> +  fp.init(window, getString("save_page_dialog_title"), fp.modeSave);
> +  fp.defaultString = LOGFILE_NAME_DEFAULT;
> +  fp.open({done: aResult => {
> +    if (aResult == fp.returnOK || aResult == fp.returnReplace) {

I would invert this if to bail and save an indent.

@@ +485,5 @@
> +    }
> +
> +    return fileNames;
> +  } catch (e) {
> +    return [];

This suppresses even coding errors. Have you considered:

    } catch (e) {
      setTimeout(() => { throw e; }); // to browser console
      return [];
    }

?

Comment 9

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

incorporate feedback comments

Comment 10

[Paul Kerr [:pkerr]]

Test collection server is hosted on the Mozilla Stackato service (paas). Source can be found at https://github.com/PauKerr/stackato-webrtc-dev.

Comment 11

[Paul Kerr [:pkerr]]

Path configured for test build logs: http://webrtc-dev.paas.allizom.org/logs/
Only POSTs are currently handled.

Comment 12

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

Comment 13

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> Comment on attachment 8607697 [details] [diff] [review]
> Part 1: about:webrtc upload all logs button
> 
> Review of attachment 8607697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> I see no need for generators here over simpler promise-chains, though
> perhaps some people prefer them (I don't).
> 
> ::: toolkit/content/aboutwebrtc/aboutWebrtc.js
> @@ +48,5 @@
> 
> Any reason to use generators over promise-chains here? e.g.
> 
>     return Promise.all(aFiles.map(file => gzipFiles(file)));
> 
> or
> 
>     return aFiles.reduce((p, file) => p.then(() => gzipFiles(file)),
> Promise.resolve());
> 
> if you don't want to do them in parallel.
> 

When there is a chain of asynchronous Promise-based operations, I find the code easier to follow and maintain by using the Task.jsm wrappers along with the yield operation. This form can help in unit testing as a generator functions can stand in for Promise returning functions and the yield will operate in the same manner.

Comment 14

[Paul Kerr [:pkerr]]

> 
> Since gzipCompress() returns a promise, this try/catch doesn't do much.
> 

gzipCompress returns a promise which is handled by the yield statement. The yield will transform a rejected promise into a throw. The try block is to handle that case.

Comment 15

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

update log collection description prompt

Comment 16

[Paul Kerr [:pkerr]]

Comment on attachment 8614723 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

JIB: This is little changed from the last version you looked over. Cleaned up some sections based on your feedback and made some cosmetic changes to the prompt code.

Comment 17

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

(In reply to Paul Kerr [:pkerr] from comment #13)
> When there is a chain of asynchronous Promise-based operations, I find the
> code easier to follow and maintain by using the Task.jsm wrappers along with
> the yield operation.

Odd, asynchronous operations is pretty much what promises do and were designed for, and is supposed to be their strength, e.g.:

  doA()
  .then(() => doB())
  .then(resultB => doC(resultB))
  .catch(failed);

I realize this may come down to style and preference, but I find using both Task.jsm and promises harder to follow than just promises. There's stuff only Task.jsm can do, but this isn't one of them.

Always choosing Task.jsm for promise-chains seems like a mistake to me, as it would make it harder to spot things that could have run in parallel - those kinds of structural observations - and harder to act on it, since we've committed to a pattern we didn't need. That, to me, makes operations harder to reason about, not easier.

Comment 18

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

(In reply to Paul Kerr [:pkerr] from comment #14)
> gzipCompress returns a promise which is handled by the yield statement. The
> yield will transform a rejected promise into a throw. The try block is to
> handle that case.

Ah, ok. TIL.

In that case, don't we want to report the error before we continue; ?

Comment 19

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

Comment on attachment 8614723 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

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

Code looks good, except I think we should use fetch, and a few nits still to be addressed, so I think I would like to see it again.

On the feature itself...

Do we feel the UX is sufficiently clear that this uploads data to Mozilla? It didn't feel that way when I tried it. Also, on wording, "upload" feels technical and more "how" than "why". We could follow about:crashes which uses "submit" which I think sounds more appropriate for what the user intent is here.

How about: [Submit logs to Mozilla] on the button, and "Successfully submitted WebRTC diagnostics to Mozilla" or something like that on the success message? The right wording here seems important, and right now it just says "Success: upload complete", which seems a little skimpish, given what just happened.

I'd like someone else to look at this, in case it needs a privacy and/or UX review. Asking dveditz. Not sure who to ping for UX review.

Also, when I first tried it, I got "No logs found" (or "No new logs found" unsure) alerts every time I hit the upload button, even though I had used WebRTC in another tab and even hit "Start AEC logging" I think. That doesn't seem ideal. I think we should ghost the button except when there are actual logs that can be uploaded.

After letting it sit for a while and hitting the button another time, then it suddenly worked. Not sure what happened there.

Also the prompt after hitting the button needs some UX love I feel:

    Log Collection Label

    Enter a description that will uniquely identify this log
    set: username or handle, problem encountered, etc. (A
    UTC timestamp is added by the server.)

It seems a bit techie, even for someone like me.

::: modules/libpref/init/all.js
@@ +332,5 @@
>  pref("media.webrtc.debug.aec_log_dir", "");
>  pref("media.webrtc.debug.log_file", "");
>  pref("media.webrtc.debug.aec_dump_max_size", 4194304); // 4MB
> +#if defined(MOZ_DEV_EDITION) || defined(NIGHTLY_BUILD)
> +pref("media.webrtc.debug.log_upload_uri", "http://webrtc-dev.paas.allizom.org/log/");

Someone other than me should probably review this before it lands, since we're uploading data to a server.

@@ +335,5 @@
> +#if defined(MOZ_DEV_EDITION) || defined(NIGHTLY_BUILD)
> +pref("media.webrtc.debug.log_upload_uri", "http://webrtc-dev.paas.allizom.org/log/");
> +#else
> +pref("media.webrtc.debug.log_upload_uri", "");
> +#endif

This leaves an "Upload logs" no-op button dangling in about:webrtc if we forget to remove it before Beta.

I assume the plan is to undo this with an uplift before it reaches Beta, but I'd feel better if we made the appearance of the upload button in about:webrtc somehow dependent on the media.webrtc.debug.log_upload_uri, or something like that, just in case we forget this is hanging out there. Is something like that possible?

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +131,5 @@
> +      ControlSet.add(new UploadLogs(uploadUri,
> +                                    savePageCtrl,
> +                                    debugModeCtrl,
> +                                    aecLoggingCtrl));
> +    } catch (e) {}

Still hiding errors here.

@@ +234,5 @@
> +
> +  this._uploadUri = aUploadUri;
> +  this._savePage = aSavePageCtrl;
> +  this._debugMode = aDebugModeCtrl;
> +  this._aecLogging = aAecLoggingCtrl;

Sorry I wasn't clear earlier. By "dead code" I meant that the || defaults you removed here were truly redundant, because UpdateLogs() is never called without all four arguments.

I meant "remove them", not "remove them and add checks everywhere they're used".

I think in JS, defensive coding is expensive, and the pattern I see used is to prefer things throwing on undefined over silently no-op'ing when unintended things happen.

@@ +248,5 @@
> +
> +  for (let name of aFileNames) {
> +    let file = new FileUtils.File(name);
> +    if (file.lastModifiedTime > aTimeMin)
> +    {

bracket at end of previous line.

@@ +283,5 @@
> +  this._message = getString("upload_logs_collecting");
> +  this.update();
> +
> +  Task.spawn(function*() {
> +    if (this._savePage) {

redundant

@@ +288,5 @@
> +      saveFiles = yield this.gzipFiles(this.newFileFilter(
> +        this._savePage.getFileNames(), gLastUploadTime));
> +    }
> +
> +    if (this._debugMode) {

redundant

@@ +293,5 @@
> +      debugFiles = yield this.gzipFiles(this.newFileFilter(
> +        this._debugMode.getFileNames(), gLastUploadTime));
> +    }
> +
> +    if (this._aecLogging) {

redundant

@@ +298,5 @@
> +      // Small binary data files and wave audio files do not need compression.
> +      aecFiles = this.newFileFilter(this._aecLogging.getFileNames(), gLastUploadTime);
> +    }
> +
> +    return; // and resolve Promise

redundant return (and comment)

@@ +304,5 @@
> +    gLastUploadTime = Date.now();
> +
> +    if (!saveFiles.length && !debugFiles.length && !aecFiles.length)
> +    {
> +      let msg = getString("upload_logs_not_found");

No alert and/or message if only one or two of these fail?

@@ +308,5 @@
> +      let msg = getString("upload_logs_not_found");
> +      alert(msg);
> +      this._message = msg;
> +      this.update();
> +      return; // and resolve Promise

redundant comment

@@ +339,5 @@
> +    this._message = getString("upload_logs_sending");
> +    this.update();
> +
> +    let req = new XMLHttpRequest();
> +    req.open("POST", this._uploadUri, true);

I really think we should use fetch here. See http://jsfiddle.net/yp2qwkaf for an example.

Chrome js represents a unique opportunity to test and showcase these new features in a way few others can afford.

@@ +342,5 @@
> +    let req = new XMLHttpRequest();
> +    req.open("POST", this._uploadUri, true);
> +    req.onload = event => {
> +      if (req.readyState === 4) {
> +        if (req.status === 200) {

==

@@ +353,5 @@
> +      }
> +    };
> +    req.send(formData);
> +
> +    return; // and resolve Promise

redundant return (and comment)

@@ +357,5 @@
> +    return; // and resolve Promise
> +  }).catch(error => {
> +    this._message = formatString("upload_logs_error", [error], 1);
> +    this.update();
> +    return; // and resolve Promise

redundant return (and comment)

@@ +414,1 @@
>      this.update();

This seems to be a repeating pattern. Have you considered:

  this.updateMsg(formatString("save_page_msg", [fp.file.path], 1));

?

Comment 20

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

UX nit: In about:webrtc I now see:

  Save Page: page saved to: /Users/Jan/aboutWebrtc.html
  Debug Mode: trace log can be found at: /tmp/webrtctrace.log
  AEC Logging: captured log files can be found in: /tmp/

Almost seems like three different ways to say the same thing ('in' vs. 'at').

Comment 21

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

Comment on attachment 8614723 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

Sevaan, any input on this?

Comment 22

[Eric Rescorla (:ekr)]

For privacy review, this needs to go to Marshall Erwin.

One question I have is: what is the content of the AEC logs? Can you
reconstruct the conversation?

Comment 23

[Sevaan Franks [:sevaan]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #21)
> Comment on attachment 8614723 [details] [diff] [review]
> Part 1: about:webrtc upload all logs button
> 
> Sevaan, any input on this?

Hi Jib, is there a screenshot I can look at? Thanks!

Comment 24

[Paul Kerr [:pkerr]]

Sevaan,
I will send you one after I apply some of the basic feedback from jib.
Paul

Comment 25

[Paul Kerr [:pkerr]]

FYI: a bug already exists to give the about:webrtc page a more complete UX overhaul.
Bug 1146994.

Comment 26

[Paul Kerr [:pkerr]]

(In reply to Eric Rescorla (:ekr) from comment #22)
> For privacy review, this needs to go to Marshall Erwin.
> 
> One question I have is: what is the content of the AEC logs? Can you
> reconstruct the conversation?

When AEC logging is turned on, it will collect a set of wave files from near, far and out paths. Limited to ~3 minutes. So, yes.

Comment 27

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)
> Comment on attachment 8614723 [details] [diff] [review]
> > +    if (!saveFiles.length && !debugFiles.length && !aecFiles.length)
> > +    {
> > +      let msg = getString("upload_logs_not_found");
> 
> No alert and/or message if only one or two of these fail?
> 

Not every type of log file will have been created. It is not an error to have not files.

Comment 28

[Paul Kerr [:pkerr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)
> Comment on attachment 8614723 [details] [diff] [review]
> > +
> > +    return; // and resolve Promise
> 
> redundant return (and comment)
> 

I have found that Promise chains can get messed up if you do not have an explicit return on exit.

Comment 29

[Paul Kerr [:pkerr]]

Attachment: Part 1: about:webrtc upload all logs button

updated with technical review feedback and some UX updates.

Comment 30

[Paul Kerr [:pkerr]]

Comment on attachment 8616254 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

jib: can you do a technical review on this latest version? I will defer landing this patch until a legal review and UX feedback are given.

dveditz: looking for review on legal implications of the log collection upload. Please needinfo [e-mail] while I am on PTO from June 8 though 15.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 8614723 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

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

I didn't dig in enough to call this a "review" but I don't think that's what jib was asking from me. This is in a special chrome-privileged page triggered by user button clicking so there aren't a lot of remote security worries. Can the page be clickjacked? No, a web page should not be able to frame a chrome-privileged about page, nor inject malicious code into it. (Of course both have happened, but that's would be a bug outside the scope of this feature.)

Server storage raises at least policy issues (that someone else will have to approve) and of course there's the security of the data itself which can't be answered from this bug because server access is not defined. Those are the biggest risks raised by this feature. We also may have to worry about people DOS'ing the server with lots of irrelevant data, but we must have some ways to deal with that because crash-stats has that issue in spades.

We should never communicate user data over insecure HTTP. Ever. Even in a prototype implementation it's a bad idea because sometimes the temporary choices stick around a lot longer than they should.

::: modules/libpref/init/all.js
@@ +332,5 @@
>  pref("media.webrtc.debug.aec_log_dir", "");
>  pref("media.webrtc.debug.log_file", "");
>  pref("media.webrtc.debug.aec_dump_max_size", 4194304); // 4MB
> +#if defined(MOZ_DEV_EDITION) || defined(NIGHTLY_BUILD)
> +pref("media.webrtc.debug.log_upload_uri", "http://webrtc-dev.paas.allizom.org/log/");

We absolutely must not upload over insecure HTTP, not even for testing. We should have wildcard certs for *.allizom.org and *.paas.allizom.org that IT or WebOps can set up for you.

Bug comments say this server is POST-only so where does the data go and how is it protected against unauthorized access? Is this data covered by our current privacy policy or do we need to add a new clause for this new kind of data? (that last one needs to be answered by the legal team)

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +127,5 @@
>      controls.appendChild(set);
> +
> +    try {
> +      let uploadUri = Services.prefs.getCharPref("media.webrtc.debug.log_upload_uri");
> +      ControlSet.add(new UploadLogs(uploadUri,

We should have some code to enforce that the URL uses https://, either here or down in UploadLogs.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 8616254 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

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

I commented on the wrong version of the patch and it looks like you addressed my main concern. Would still be nice to enforce https: urls.

Comment 33

[Paul Kerr [:pkerr]]

(In reply to Daniel Veditz [:dveditz] from comment #32)
> Comment on attachment 8616254 [details] [diff] [review]
> Part 1: about:webrtc upload all logs button
> 
> Review of attachment 8616254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I commented on the wrong version of the patch and it looks like you
> addressed my main concern. Would still be nice to enforce https: urls.

I will add https enforcement within the aboutWebrtc.js module.

Comment 34

[Eric Rescorla (:ekr)]

(In reply to Paul Kerr [:pkerr] from comment #26)
> (In reply to Eric Rescorla (:ekr) from comment #22)
> > For privacy review, this needs to go to Marshall Erwin.
> > 
> > One question I have is: what is the content of the AEC logs? Can you
> > reconstruct the conversation?
> 
> When AEC logging is turned on, it will collect a set of wave files from
> near, far and out paths. Limited to ~3 minutes. So, yes.

This seems like a pretty serious issue that is going to deter people uploading
logs. Can we do anything for audio artifacts without this?

Comment 35

[Paul Kerr [:pkerr]]

(In reply to Eric Rescorla (:ekr) from comment #34)
> (In reply to Paul Kerr [:pkerr] from comment #26)
> > (In reply to Eric Rescorla (:ekr) from comment #22)
> > > For privacy review, this needs to go to Marshall Erwin.
> > > 
> > > One question I have is: what is the content of the AEC logs? Can you
> > > reconstruct the conversation?
> > 
> > When AEC logging is turned on, it will collect a set of wave files from
> > near, far and out paths. Limited to ~3 minutes. So, yes.
> 
> This seems like a pretty serious issue that is going to deter people
> uploading
> logs. Can we do anything for audio artifacts without this?

At the core, part 1 of this bug is to make it easier to get the logs to us after someone has used the existing features of about:webrtc. Previously, some sets AEC logs have been captured by users after being walked through the process via irc. And then they find the logs, zip them up, and send them or attach them to bugs in bugzilla(, or fail). Bugzilla is essentially open to the public.

That is not to say there is not an issue here. Logs of information that do not include actual speech capture would generate less resistance to upload. There is a button on about:webrtc that can generate audio samples for AEC debug. There is another bug asking for capture of audio data to help track down the 'robot' voice distortion. But, these features are less useful without an easier way to get those logs.

Is this upload feature viable for this type of information? I think it depends on the user group. Testers and others using Nightly and Dev Edition to heavily use WebRTC and Hello would be, I feel, likely to use the feature. The general population, no; this is not telemetry.

Comment 36

[Maire Reavy [:mreavy]]

(In reply to Eric Rescorla (:ekr) from comment #34)
> > When AEC logging is turned on, it will collect a set of wave files from
> > near, far and out paths. Limited to ~3 minutes. So, yes.
> 
> This seems like a pretty serious issue that is going to deter people
> uploading
> logs. Can we do anything for audio artifacts without this?

Not much for audio/echo issues.  Let's put a checkbox in for including the the AEC logs. Note: it should not say "AEC logs"; it should be phrased so people will understand it -- "If you experienced audio quality or echo during the call, checking this box will send the last few minutes of audio from your conversation so we can better troubleshoot the problem."

Comment 37

[Paul Kerr [:pkerr]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #36)
> (In reply to Eric Rescorla (:ekr) from comment #34)
> > > When AEC logging is turned on, it will collect a set of wave files from
> > > near, far and out paths. Limited to ~3 minutes. So, yes.
> > 
> > This seems like a pretty serious issue that is going to deter people
> > uploading
> > logs. Can we do anything for audio artifacts without this?
> 
> Not much for audio/echo issues.  Let's put a checkbox in for including the
> the AEC logs. Note: it should not say "AEC logs"; it should be phrased so
> people will understand it -- "If you experienced audio quality or echo
> during the call, checking this box will send the last few minutes of audio
> from your conversation so we can better troubleshoot the problem."

The latest patch has an additional confirmation when AEC is enabled. I added it as a placeholder to begin to give the non-technical user more information as to what was going to happen.

Comment 38

[Paul Kerr [:pkerr]]

Builds of Nightly with the current patch:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2216cb3a9aa4

Comment 39

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

Comment on attachment 8616254 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

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

lgtm with nits.

I'm not a UX, html or css reviewer so I'll leave that to someone else.

Looks a lot better! Might suffice for landing until the UX overhaul?

I would try to rephrase some of the language to avoid "you" as I found that a bit jarring when reading. I could be wrong but it seems to me we tend to avoid that? with passive voice etc.

The whole "pick a unique id" bit seems a bit odd, the crash-reporter just makes up uuids, which might be a better approach down the road, but this works I suppose.

::: toolkit/content/aboutwebrtc/aboutWebrtc.js
@@ +341,5 @@
> +      saveFiles = yield this.gzipFiles(this.newFileFilter(this._savePage.getFileNames(),
> +                                                          gLastUploadTime));
> +      debugFiles = yield this.gzipFiles(this.newFileFilter(this._debugMode.getFileNames(),
> +                                                           gLastUploadTime));
> +      // Small binary data files and wave audio files are not worth compressing.

Aren't wave audio files uncompressed audio?

@@ +344,5 @@
> +                                                           gLastUploadTime));
> +      // Small binary data files and wave audio files are not worth compressing.
> +      aecFiles = this.newFileFilter(this._aecLogging.getFileNames(), gLastUploadTime);
> +
> +      return true;

seems redundant

@@ +347,5 @@
> +
> +      return true;
> +    }.bind(this));
> +  }).then(submit => {
> +    if (!submit) {

Will submit ever be false?

@@ +356,5 @@
> +    {
> +      // Don't bother with an empty POST
> +      let msg = getString("upload_logs_not_found");
> +      alert(msg);
> +      return null;

The previous version also called updateMessage here. Was removal intentional?

@@ +392,5 @@
> +      this.updateMessage(formatString("upload_logs_failure",
> +                                      [response.status, response.statusText], 2));
> +    }
> +
> +    return;

redundant return

@@ +394,5 @@
> +    }
> +
> +    return;
> +  }).catch(error => {
> +    this.updateMessage(formatString("upload_logs_error", [error], 1));

I think you need [error.toString()] here.

This displays a non-localized error message in the UI. Unusual, but then about:webrtc is chock full of non-localized logs and errors anyway so maybe that's ok?

@@ +395,5 @@
> +
> +    return;
> +  }).catch(error => {
> +    this.updateMessage(formatString("upload_logs_error", [error], 1));
> +    return;

redundant return

@@ +1093,5 @@
> +
> +LogSetDialog.prototype = {
> +  init: function() {
> +    if (!this.dialog) {
> +      return false;

I'm a big fan of invariants. As far as I can tell, this.dialog is guaranteed to be truthy, so this line will never happen, which is good because nothing checks the return value from LogSetDialog.init().

@@ +1101,5 @@
> +    let cancel = this.dialog.querySelector(".button-cancel");
> +    this._descElement = this.dialog.querySelector("#logset-desc");
> +
> +    if (!ok || !cancel) {
> +      return false;

Same here. These are invariants. No need to return anything from init() at all it seems, which is good, because it's not clear that what would happen then is any better than not returning here.

The quicker we get to a JS TypeError the better IMHO, as defensive coding just delays and moves errors farther away from the problem spot, making bugs harder to locate.

@@ +1105,5 @@
> +      return false;
> +    }
> +
> +    ok.onclick = () => {this.close(true)};
> +    cancel.onclick = () => {this.close(false)};

lose the { }s

@@ +1112,5 @@
> +
> +    let setString = (element_id, string_id) => {
> +      let node = this.dialog.querySelector(element_id);
> +      let string = getString(string_id);
> +      if (node && string) {

When will node or string be falsy? And if they are, don't we want to know about it? Seems like they're both invariants in all the calls below. Not a fan of no-op on error.

setString can then be simplified.

let setString = (a, b) => this.dialog.querySelector(a) = getString(b);

@@ +1115,5 @@
> +      let string = getString(string_id);
> +      if (node && string) {
> +        node.textContent = string;
> +      }
> +      return node;

return node; doesn't do anything.

@@ +1125,5 @@
> +    setString("#logset-notice", "logset_legal_notice");
> +    setString("#logset-desc-label", "logset_desc_label");
> +
> +    let string = getString("logset_desc_hint");
> +    if (string && this._descElement) {

invariant

@@ +1132,5 @@
> +  },
> +
> +  open: function() {
> +    if (!this.dialog) {
> +      return Promise.resolve(false);

can't happen

@@ +1135,5 @@
> +    if (!this.dialog) {
> +      return Promise.resolve(false);
> +    }
> +
> +    if (this._descElement) {

invariant

@@ +1148,5 @@
> +  close: function(flag) {
> +    let rval = flag;
> +
> +    if (!this.dialog) {
> +      rval = false;

invariant? Or is there something I'm not getting?

@@ +1155,5 @@
> +    }
> +
> +    if (this._resolve) {
> +      this._resolve(rval);
> +      this._resolve = undefined;

delete this._resolve; I think is preferred, but I doubt this is needed at all.

@@ +1162,5 @@
> +    return rval;
> +  },
> +
> +  getDescriptionText: function() {
> +    return this._descElement ? this._descElement.value : null;

invariant?

::: toolkit/locales/en-US/chrome/global/aboutWebrtc.properties
@@ +137,5 @@
> +logset_cancel = Cancel Upload
> +logset_desc_label = Call Description
> +logset_desc_hint = Enter a description of the WebRTC session associated with these logs. Please include some unique details so that this log set can be identified later on the collection server.
> +logset_legal_notice = Notice: the logs files collected will be sent and stored on a Mozilla server. These logs can contain internal and external internet addresses from both sides of a session, along with hardware information. If AEC debugging was enabled up to three minutes of audio samples may have been captured. These logs can be from any WebRTC session since you opened about:webrtc: your current session or previous sessions. One or more of these logs may be attached to publicly viewable bug reports or be generally accessible by the public.
> +aec_notice = As part of the Acoustic Echo Cancellation (AEC) debug information, up to three minutes of you and your callers converstation will be captured. This is used to analyse the echo. Do you wish to proceed?

typos: conversation and analyze.

Do you wish to proceed is a yes/no question, but buttons say OK, Cancel. Not familiar enough with this to know how this is commonly handled, just stuck out when I tested it.

Comment 40

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

(In reply to Paul Kerr [:pkerr - on PTO to June 16] from comment #28)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #19)
> > Comment on attachment 8614723 [details] [diff] [review]
> > > +    return; // and resolve Promise
> > 
> > redundant return (and comment)
> 
> I have found that Promise chains can get messed up if you do not have an
> explicit return on exit.

(I missed this comment earlier) I don't believe this to be true. Return statements are not required in JS. Running off the end of the function is equivalent to return undefined; or just return; The code should function without them or some other assumption must be wrong.

My r+ assumes these will be removed.

Comment 41

[Sevaan Franks [:sevaan]]

Comment on attachment 8616254 [details] [diff] [review]
Part 1: about:webrtc upload all logs button

I'm not sure how to build or test these patches. Could you please attach a screenshot for me to see to give feedback? Thanks!

Comment 42

[Maire Reavy [:mreavy]]

Paul - see comment 41

Comment 43

[Paul Kerr [:pkerr]]

Working on that. Thanks.

Comment 44

[Paul Kerr [:pkerr]]

Attachment: about:webrtc initial view

Initial view when about:webrtc is loaded. In this example there is an active session and information from a previous webrtc session, now closed.

Comment 45

[Paul Kerr [:pkerr]]

Attachment: save page dialog

When the "Save This Page" button is pushed, a file chooser dialog is launched to allow the user to name the file and choose the folder for storage. This file will contain all the data that is displayed on the about:webrtc page currently.

Comment 46

[Paul Kerr [:pkerr]]

Attachment: page save complete

After the about:webrtc page is saved to a file, a message is show with a reminder as to the file name and location.

Comment 47

[Paul Kerr [:pkerr]]

Attachment: details after expanding section

Comment 48

[Paul Kerr [:pkerr]]

Attachment: debug mode turned on

When debug mode is on, the button changes to "Stop Debug Mode" and an information line is shown with the location of the trace file. The actual file location and name can be set via prefs or from environment variables. There is also a default name and location (per operating system) if the user has not specified a location. That is one of the reasons that I display the file name as the user may have no idea where it will be stored.

Comment 49

[Paul Kerr [:pkerr]]

Attachment: echo canceller logging confirmation

When the "Start AEC Logging" button is selected, a confirmation dialog is displayed since this feature may capture live audio from the session.

Comment 50

[Paul Kerr [:pkerr]]

Attachment: echo canceller capture active

In this screen capture, both Debug Mode and AEC Logging are active.

Comment 51

[Paul Kerr [:pkerr]]

Attachment: aec and debug mode stopped

Comment 52

[Paul Kerr [:pkerr]]

Attachment: log upload confirmation dialog

This is the new feature in the attached patch. The dialog needs to re-confirm with the user they type of data that will be uploaded, who will see it, and where it will be stored. The current text is just a placeholder. We would also like to prompt the user to describe the issue encountered in this bad session and maybe label this with "call with pkerr - bad echo".

Comment 53

[Paul Kerr [:pkerr]]

Attachment: log upload in progress

It can take up to 30 seconds to compress and upload a complete set of log files.

Comment 54

[Paul Kerr [:pkerr]]

Attachment: log upload complete

Comment 55

[Paul Kerr [:pkerr]]

Sevaan,
I have attached a set of screen caps that walk through the various operations and responses of the about:webrtc page. Only the "Upload Logs" feature is new and requires the attached patch to test. The other features captured in the screen caps can be seen in Firefox beta. You can get a feel for the about:webrtc page by doing a Hello call and opening about:webrtc.

We should set up a time to walk through all of this after you have had an initial view.

Comment 56

[Sevaan Franks [:sevaan]]

> When debug mode is on, the button changes to "Stop Debug Mode" and an
> information line is shown with the location of the trace file.

Can we make the location to the trace file a link that opens in a new tab? That way a user can quickly access it and see the contents without having to use Finder/Explorer.
 
> When the "Start AEC Logging" button is selected, a confirmation dialog is
> displayed since this feature may capture live audio from the session.

The importance of this message needs to be amplified. Let's add a title (string to be reviewed by someone more qualified) "Warning: Your call will be recorded" and add a warning icon. There should also be a "Learn more" link taking the user to a page that explains why it's being recorded, who will listen, how long that data persists, etc.

One thing that concerns me here is that this user is making a decision for both users regarding the call being recorded. I think each user needs to be informed and allowed to make that choice. We have implemented text chat, so maybe this is a good mechanism to send a message to both users saying something like "Your partner in the conversation wishes to upload a log of this conversation. As a result, 3 minutes of your audio/video will be recorded for review by yadda yadda. Learn more." Can we give them an option to cancel this recording from this dialogue? A button perhaps? If not, maybe the text can mention that the user should leave the conversation is they do not want their conversation recorded.

Also, Paul had a good idea during a brief conversation we had about this to put something on the video stream showing that the call is being recorded...maybe a red blinking dot with a short message beside it.

> Created attachment 8627791 [details]
> echo canceller capture active

Maybe we could have the same blinking red dot indicating recording is active on this screen in the far-right. Or change the text saying the logging is active to a red colour so it catches the eye.

> Created attachment 8627791 [details]
> echo canceller capture active

Idea: Make the aec and debug button text green when it is not recording, and red when it is recording, to indicate start/stop.

> Created attachment 8627797 [details]
> log upload in progress

A spinner or loading bar here would be helpful.

> Created attachment 8627799 [details]
> log upload complete

A check mark icon next to the upload complete text would be helpful.

Comment 57

[Paul Kerr [:pkerr]]

rjesup: I am contemplating changing how the AEC logging capture is triggered. Instead of turning it on and then having the capture run until the user turns it off again, the new behavior would be that a dialog pops up when the user hits the AEC button telling her that audio will be captured with an ok/cancel choice. If ok is selected, the dialog changes to prompt the callers to talk while a timeout counts down. After the timeout, the AEC capture will be stopped. This accomplished two main things: getting the near and far end to say something long enough for a good capture and to turn off the capture in a way the user cannot forget to do -- helping with the privacy concerns. By using a pop-up dialog, it is very obvious when capture is happening and when it stops.

Questions for you:
1) Will this work to generate the files that we want to analyze?
2) What is the ideal sample size? We are currently limiting capture to three minutes worth of samples. Would one minute work as well if the users are prompted to speak?

Comment 58

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

(In reply to Paul Kerr [:pkerr] from comment #57)

> Questions for you:
> 1) Will this work to generate the files that we want to analyze?

Sure.  That works; my initial implementation was done the way it was because it was expedient.  This is better.

> 2) What is the ideal sample size? We are currently limiting capture to three
> minutes worth of samples. Would one minute work as well if the users are
> prompted to speak?

Typically 1 minute is enough if both sides talk.  If it's a popup, a count-down to a maximum time works, with a Stop button (and it stops on anything that dismisses the popup, though if it's modal that's probably implies.

Comment 59

[Paul Kerr [:pkerr]]

Attachment: Example POST of log data

Sections of the large attached files have been clipped to save space, otherwise the upload is multiple megabytes of data.