Closed Bug 1163859 Opened 8 years ago Closed 8 years ago

WebRTC logging module triggering content thread warning on pref write operation

Categories

(Core :: WebRTC, defect, P4)

41 Branch
defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
40.3 - 11 May
Tracking Status
firefox41 --- fixed

People

(Reporter: pkerr, Assigned: pkerr)

Details

Attachments

(1 file, 1 obsolete file)

[Child 20525] WARNING: ENSURE_MAIN_PROCESS failed. Cannot SetCString from content process: media.webrtc.debug.aec_log_dir: file ../../../modules/libpref/Preferences.cpp, line 1516.

The above is caused by about:webrtc code starting logging or AEC sample capture in both chrome and content thread under e10s operation. The WebRtcLog module needs to check to see if it is running in the gecko_default thread before updating webrtc.debug prefs.
Attachment #8604381 - Attachment is obsolete: true
Since a call to SetWebRtcDebug is made in both the chrome and content process by WebrtcGlobalInformation.cpp, these prefs will be set without needed to make the call in both processes. The file names would have been identical.
Attachment #8604382 - Flags: review?(rjesup)
Comment on attachment 8604382 [details] [diff] [review]
Only update webrtc.debug prefs from gecko thread

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

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
@@ +150,5 @@
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    // Capture the final choices for the trace settings.
> +    mozilla::Preferences::SetCString("media.webrtc.debug.log_file", aLogFile);
> +    mozilla::Preferences::SetCString("media.webrtc.debug.aec_log_dir", aAECLogDir);
> +  }

Note there's an existing problem in that this modifies the pref settings, which would then apply to future runs. Since these are debugging tools, that may be ok, but it is surprising and problematic if a user wants logs to appear in a specific place.
Attachment #8604382 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #4)
> Comment on attachment 8604382 [details] [diff] [review]
> Only update webrtc.debug prefs from gecko thread
> 
> Review of attachment 8604382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
> @@ +150,5 @@
> > +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> > +    // Capture the final choices for the trace settings.
> > +    mozilla::Preferences::SetCString("media.webrtc.debug.log_file", aLogFile);
> > +    mozilla::Preferences::SetCString("media.webrtc.debug.aec_log_dir", aAECLogDir);
> > +  }
> 
> Note there's an existing problem in that this modifies the pref settings,
> which would then apply to future runs. Since these are debugging tools, that
> may be ok, but it is surprising and problematic if a user wants logs to
> appear in a specific place.

It is in the specific case where the user wants to temporarily override the prefs that they had previously set by using the environment vars and expecting the pref values that this is problematic. The log will always appear where they want it if they use the prefs. The WebrtcGlobalInformation API could be extended to allow for the file locations to be queried, but this would need to get dom approval.
https://hg.mozilla.org/mozilla-central/rev/ad07670653a2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.