1449306 - Stop using println! for debug logging

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Aria Beingessner [:Gankra]]

User machines seem to sometimes get in a state where stdout isn't writeable, and println! will panic if it can't actually print, crashing the browser. Since we only use println! for debug logging, it should be fine to just continue the program if println fails.

It should be fairly easy to make a debugln! macro which is a copy of println! that simply suppresses its result.

Comment 1

[Aria Beingessner [:Gankra]]

Ah actually webrender already has log/env_logger all setup. We should just use it everywhere: https://github.com/servo/webrender/blob/master/webrender/src/device.rs#L769

Comment 2

[Aria Beingessner [:Gankra]]

Some upstream work for this: https://github.com/servo/webrender/pull/2648

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: Bug 1434630 - use proper logging framework instead of println.

Not 100% confident in the log levels I chose, nor if this actually just works without me doing anything else. Non-trivial review appreciated :D

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8967454 [details] [diff] [review]
Bug 1434630 - use proper logging framework instead of println.

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

Looks ok. The default level is controlled from https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/gfx/webrender_bindings/WebRenderAPI.cpp#253 - I think we might want to change that to info or warn by default. I'm not too sure how much spew it will generate but until we ship webrender getting more output by default is probably useful. We could even increase the level on debug builds by #ifdef DEBUG

::: gfx/webrender_bindings/src/bindings.rs
@@ +753,2 @@
>      let version = gl.get_string(gl::VERSION);
> +    debug!("WebRender - OpenGL version new {}", version);

I think this one should be info instead of debug

Comment 5

[Aria Beingessner [:Gankra]]

Comment on attachment 8967454 [details] [diff] [review]
Bug 1434630 - use proper logging framework instead of println.

>diff --git a/gfx/webrender_bindings/src/bindings.rs b/gfx/webrender_bindings/src/bindings.rs
>index a8cdd32..ac8441c 100644
>--- a/gfx/webrender_bindings/src/bindings.rs
>+++ b/gfx/webrender_bindings/src/bindings.rs
>@@ -422,10 +422,9 @@ fn get_proc_address(glcontext_ptr: *mut c_void,
>     let symbol_name = CString::new(name).unwrap();
>     let symbol = unsafe { get_proc_address_from_glcontext(glcontext_ptr, symbol_name.as_ptr()) };
>
>-    // For now panic, not sure we should be though or if we can recover
>     if symbol.is_null() {
>         // XXX Bug 1322949 Make whitelist for extensions
>-        println!("Could not find symbol {:?} by glcontext", symbol_name);
>+        warn!("Could not find symbol {:?} by glcontext", symbol_name);
>     }
>
>     symbol as *const _
>@@ -530,7 +529,7 @@ pub extern "C" fn wr_renderer_render(renderer: &mut Renderer,
>         Ok(_) => true,
>         Err(errors) => {
>             for e in errors {
>-                println!(" Failed to render: {:?}", e);
>+                warn!(" Failed to render: {:?}", e);
>                 let msg = CString::new(format!("wr_renderer_render: {:?}", e)).unwrap();
>                 unsafe {
>                     gfx_critical_note(msg.as_ptr());
>@@ -754,7 +753,7 @@ pub extern "C" fn wr_window_new(window_id: WrWindowId,
>
>     let version = gl.get_string(gl::VERSION);
>
>-    println!("WebRender - OpenGL version new {}", version);
>+    info!("WebRender - OpenGL version new {}", version);
>
>     let workers = unsafe {
>         Arc::clone(&(*thread_pool).0)
>@@ -796,7 +795,7 @@ pub extern "C" fn wr_window_new(window_id: WrWindowId,
>     let (renderer, sender) = match Renderer::new(gl, notifier, opts) {
>         Ok((renderer, sender)) => (renderer, sender),
>         Err(e) => {
>-            println!(" Failed to create a Renderer: {:?}", e);
>+            warn!(" Failed to create a Renderer: {:?}", e);
>             let msg = CString::new(format!("wr_window_new: {:?}", e)).unwrap();
>             unsafe {
>                 gfx_critical_note(msg.as_ptr());
>@@ -1204,7 +1203,7 @@ pub extern "C" fn wr_api_capture(
>             file.write(revision).unwrap();
>         }
>         Err(e) => {
>-            println!("Unable to create path '{:?}' for capture: {:?}", path, e);
>+            warn!("Unable to create path '{:?}' for capture: {:?}", path, e);
>             return
>         }
>     }
>diff --git a/gfx/webrender_bindings/src/lib.rs b/gfx/webrender_bindings/src/lib.rs
>index 5ffa18e..426d28b 100644
>--- a/gfx/webrender_bindings/src/lib.rs
>+++ b/gfx/webrender_bindings/src/lib.rs
>@@ -10,6 +10,8 @@ extern crate app_units;
> extern crate gleam;
> extern crate rayon;
> extern crate thread_profiler;
>+
>+#[macro_use]
> extern crate log;
>
> #[cfg(target_os = "windows")]

Comment 6

[Aria Beingessner [:Gankra]]

woah what did i just do

ignore that >.>

Comment 7

[Aria Beingessner [:Gankra]]

Attachment: Bug 1434630 - use proper logging framework instead of println.

changed debug! to info!

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8967576 [details] [diff] [review]
Bug 1434630 - use proper logging framework instead of println.

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

A commit message would be good too

Comment 9

[Aria Beingessner [:Gankra]]

Attachment: patch.diff

commit message added

Comment 10

[Kartikaya Gupta (email:[e-mail])]

The patch had the bug number as 1434630. Which is not this bug, but basically the same thing. Since mc-merge will close that bug I'm going to close this one now.