1269930 - Crash on windows when logging AEC data from about:webrtc

Status: RESOLVED FIXED

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

Description

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

We're crashing the plugin-container because we fail to open AEC log files in windows under e10s.  The code considers a failure-to-open to be crash-worthy.

Another cause for crashing is if you set the pref to a directory that doesn't exist or you can't write to (from the Content process in e10s, or in non-e10s mode, if you can't write for any reason.)

Also, we're using XP_WIN to know if we're on windows or not, we should be using WEBRTC_WIN; XP_WIN is a mozilla-ism.

The base problem for e10s seems to be the use of NS_OS_TEMP_DIR -- I think we should be using NS_WIN_LOW_INTEGRITY_TEMP_BASE.  Separately we should not crash on failure to open (override the upstream code's preference to crash).

Comment 1

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

bob/jimm: Is NS_WIN_LOW_INTEGRITY_TEMP_BASE the right thing to use?

Comment 2

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

Attachment: use NS_WIN_LOW_INTEGRITY_TEMP_BASE for AEC logs

MozReview-Commit-ID: ByZZs9MKMDK

Comment 3

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

Attachment: Fix some errant (though working) ifdefs in mods to upstream webrtc code

MozReview-Commit-ID: 3bCBD3I4fHO

Comment 4

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

Attachment: don't crash if an AEC logfile fails to open

MozReview-Commit-ID: 4MgOZe5jO3p

Comment 5

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

Attachment: use NS_WIN_LOW_INTEGRITY_TEMP_BASE for AEC logs

as pkerr points out, that's clearly windows-only

Comment 6

[Bob Owen (:bobowen)]

(In reply to Randell Jesup [:jesup] from comment #1)
> bob/jimm: Is NS_WIN_LOW_INTEGRITY_TEMP_BASE the right thing to use?

It depends on what you want.

You might be better using NS_APP_CONTENT_PROCESS_TEMP_DIR, which will work for Mac as well.
I suspect you might have similar problems on Mac Nightly with this logging.

NS_APP_CONTENT_PROCESS_TEMP_DIR gets deleted on shutdown, so if that's a problem you could use NS_WIN_LOW_INTEGRITY_TEMP_BASE or perhaps a new directory beneath it.
Although you might need to think about how that gets cleaned up, because it's in a place that users wouldn't normally go to clear temp files.

Also if you're using this directory in the parent, this might have security implications.
Could you use separate directories in the parent and child?
When necessary, NS_OS_TEMP_DIR in the content process gets changed to the same as NS_APP_CONTENT_PROCESS_TEMP_DIR during ContentProcess::Init.

Happened to notice that WebRtcLog.cpp looks like it sets up another log file with NS_OS_TEMP_DIR, would that need to be changed as well?

I'll clear the review request for the moment.

Comment 7

[Bob Owen (:bobowen)]

I'm not sure what is best here.

You could use NS_WIN_LOW_INTEGRITY_TEMP_BASE and construct a sandbox writeable directory and file, but this won't help for Mac assuming you have the same problem.

Even these writeable directories are a bit of a stop gap that we'd like to get rid of eventually, particularly for release builds.

Assuming that this logging is turned on in the UI, could you pass file handles to the child process to be used?
This is probably the best long term solution.

Comment 8

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

Opening in the parent may make sense, though that will be a pain since the low-level code is what's deciding the files to log.  Can the master process create a directory that's writable by content, and pass that down somehow?

In the shorter term, we want to get something working so we can ask people for logs and not tell them to use non-e10s windows or turn it off.  So we may want to land this or something very like it and then follow up.

Passing the data through IPC would be another option, but would require a *bunch* more work to batch this up and asynchronously feed it across from another thread (this is collected in realtime code; it's already playing with fire by using fwrite here today and is mostly saved by stdio/OS buffering and faster machines).

Some of this work may already have been done for webrtc.org logging, so that might be leveraged here.  Chrome instead dumps logs and AEC data and other stuff all into one composite metafile where stuff can pulled out.

Comment 9

[Jim Mathies [:jimm]]

sandboxing related.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Randell Jesup [:jesup] from comment #8)
> Opening in the parent may make sense, though that will be a pain since the
> low-level code is what's deciding the files to log.  Can the master process
> create a directory that's writable by content, and pass that down somehow?
> 
> In the shorter term, we want to get something working so we can ask people
> for logs and not tell them to use non-e10s windows or turn it off.  So we
> may want to land this or something very like it and then follow up.

Can you ask them to disable the security sandbox instead? AFAICT e10s isn't really key here.

Maybe just harden your logging code such that it doesn't crash if the sandbox is on. That should work up to the point where we start enforcing a minimal sandbox for content processes.

Comment 11

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

(In reply to Jim Mathies [:jimm] from comment #10)
> (In reply to Randell Jesup [:jesup] from comment #8)
> > Opening in the parent may make sense, though that will be a pain since the
> > low-level code is what's deciding the files to log.  Can the master process
> > create a directory that's writable by content, and pass that down somehow?
> > 
> > In the shorter term, we want to get something working so we can ask people
> > for logs and not tell them to use non-e10s windows or turn it off.  So we
> > may want to land this or something very like it and then follow up.
> 
> Can you ask them to disable the security sandbox instead? AFAICT e10s isn't
> really key here.

We need to be able to ask users (and third-party-developers) for logs, and in particular to turn on logs in the middle of existing calls. 

> Maybe just harden your logging code such that it doesn't crash if the
> sandbox is on. That should work up to the point where we start enforcing a
> minimal sandbox for content processes.

The other two patches here will block the crashing; but that doesn't help us deal with reports of "I hear echo sometimes", and only barely helps with "I hear echo on machine X".  It's confusing enough getting people to tell the person *not* hearing the echo to turn on AEC logging.

For other logs (i.e. mtransport/nicer/R_LOG* logs) it's perhaps more problematic, though those (currently) are turned on before starting the browser.

I prefer not to go back to the equivalent of the old "please download a debug build and try to repro with logging" if I can.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00dd4f7e1b8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6251d574735f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00dd4f7e1b8f
https://hg.mozilla.org/mozilla-central/rev/6251d574735f