Closed Bug 1454878 Opened 2 years ago Closed 2 years ago

Update WrExternalLogHandler as to use env_logger

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → sotaro.ikeda.g
Hmm, WrExternalLogHandler seems to work only when gpu process is enabled.
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()
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
The rust log is a global system. It seems not make sense to set graphics only logger WrExternalLogHandler to the rust log.
(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.
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.
Summary: Connect GfxLogLevel to WrLogLevel → 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.
Attachment #8969180 - Flags: review?(jmuizelaar)
Blocks: 1456350
Attachment #8969180 - Flags: review?(jmuizelaar) → review+
Rebased.
Attachment #8969180 - Attachment is obsolete: true
Attachment #8970771 - Flags: review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be541e099174
Update WrExternalLogHandler as to use env_logger r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/be541e099174
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.