1399389 - Integrate gfxCriticalNote/gfxCriticalError in WebRender

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

Being able to attach some info to crash reports is useful in gecko and it would be useful to have access to that in WebRender code as well.

This should be fairly easy to do.

Relevant webrender issue: https://github.com/servo/webrender/issues/1677

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: redirect the warn/error message to gfx_critical_error/note in WR.

MozReview-Commit-ID: Fxkz3Fq96Tb

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating.

MozReview-Commit-ID: 8FIu1K1jjxM

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8922247 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR.

This patch try to use a custom log_handler which could redirect the error/warn message to gecko through the ffi.

I will get a strange call stack in crash report for a gfx_critical_error message. Maybe it's the problem as bug 1352475.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8922248 [details] [diff] [review]
P2: update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating.

Just update the cargo.lock and ffi.h

Comment 5

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8922247 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR.

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

Good start! A few fixes are needed

::: gfx/thebes/gfxPlatform.cpp
@@ +141,5 @@
>  #include "mozilla/gfx/GPUParent.h"
>  #include "mozilla/layers/MemoryReportingMLGPU.h"
>  #include "prsystem.h"
>  
> +#include "mozilla/webrender/webrender_ffi.h"

nit: any reason for this to be on a separate line?

::: gfx/webrender_bindings/Cargo.toml
@@ +12,5 @@
>  euclid = "0.15"
>  app_units = "0.5.6"
>  gleam = "0.4"
> +log = "0.3"
> +env_logger = "0.4"

we shouldn't use `env_logger` in FF at all

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1: redirect the warn/error message to gfx_critical_error/note in WR. v2.

remove the env_logger.

MozReview-Commit-ID: Fxkz3Fq96Tb

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2: update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating. v2

update for new webrender_bindings change.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee22692d9844
redirect the warn/error message to gfx_critical_error/note in WR. r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/2be892ca2def
update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating. r=nical

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ee22692d9844
https://hg.mozilla.org/mozilla-central/rev/2be892ca2def

Comment 10

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

Comment on attachment 8922726 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR. v2.

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

::: gfx/thebes/gfxPlatform.cpp
@@ +2568,5 @@
>      }
> +
> +    // Redirect the webrender's log to gecko's log system.
> +    // The current log level is "warning".
> +    mozilla::wr::wr_init_external_log_handler(wr::LogLevelFilter::Warn);

I don't think this will do what you want on Windows. On Windows this code runs in the UI process, but the useful part of the webrender code lives in the GPU process.

Also it would be good to wrap this raw wr_* function in a WebRenderAPI static function so we don't have raw FFI calls littered all over the place.

Comment 11

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

^

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Comment on attachment 8922726 [details] [diff] [review]

> I don't think this will do what you want on Windows. On Windows this code
> runs in the UI process, but the useful part of the webrender code lives in
> the GPU process.
> 
> Also it would be good to wrap this raw wr_* function in a WebRenderAPI
> static function so we don't have raw FFI calls littered all over the place.

Thanks, I will create another bug to fix this.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Create bug 1415816 for comment 10.