1454878 - Update WrExternalLogHandler as to use env_logger

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

In gecko, wr::WrLogLevelFilter::Error is always used for WrLogLevelFilter in WebRenderAPI::InitExternalLogHandler(). It seems better to set GfxLogLevel to WrLogLevelFilter.

https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/WebRenderAPI.cpp#257

Comment 1

[Sotaro Ikeda [:sotaro]]

Hmm, WrExternalLogHandler seems to work only when gpu process is enabled.

Comment 2

[Sotaro Ikeda [:sotaro]]

WrExternalLogHandler was set by calling log::set_logger(). But it works only when a logger is not set to rust log yet.
  https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#2367

When WebRender was crated on parent process, a logger was already set to rust log. It was done under InitializeServo(). Its stack was like the following.

xul.dll!log::set_logger_inner<closure>(closure make_logger) 行 1054
xul.dll!env_logger::Builder::try_init() 行 610
xul.dll!env_logger::Builder::init() 行 622
xul.dll!geckoservo::glue::Servo_Initialize(a) 行 18
xul.dll!InitializeServo() 行 2444	C++
xul.dll!nsLayoutStatics::Initialize() 行 286	C++
xul.dll!Initialize() 行 274	C++
xul.dll!nsComponentManagerImpl::KnownModule::Load() 行 727	C++
xul.dll!nsFactoryEntry::GetFactory() 行 1748	C++
xul.dll!nsComponentManagerImpl::CreateInstanceByContractID()

Comment 3

[Sotaro Ikeda [:sotaro]]

Like the following link, log::set_logger_inner() store a logger only when it is not initialized yet.
 https://dxr.mozilla.org/mozilla-central/source/third_party/rust/log/src/lib.rs#1049

Comment 4

[Sotaro Ikeda [:sotaro]]

The rust log is a global system. It seems not make sense to set graphics only logger WrExternalLogHandler to the rust log.

Comment 5

[Sotaro Ikeda [:sotaro]]

MDN already has a document for logging in rust.
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging#Rust

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> The rust log is a global system. It seems not make sense to set graphics
> only logger WrExternalLogHandler to the rust log.

Then it seems better to remove WrExternalLogHandler and use env_logger instead to make the logging consistent to gecko.

Comment 7

[Sotaro Ikeda [:sotaro]]

I changed my mind, it seems better to keep WrExternalLogHandler for now. Even if WrExternalLogHandler works only with GPU process. It helps to analyze WebRender's bug on Windows. And WrExternalLogHandler could coexist with env_logger.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update WrExternalLogHandler as to use env_logger

The patch also limit WrExternalLogHandler usage only in gpu process. On non-gpu process, InitializeServo() handles rust logger initialization and WrExternalLogHandler does not work already.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update WrExternalLogHandler as to use env_logger

Rebased.

Comment 10

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29da378e8457b311343dfbb7081ec9bfc1ecc4f9

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be541e099174
Update WrExternalLogHandler as to use env_logger r=jrmuizel

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/be541e099174