-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #1077: Deprecate Mozc_tsf_ui.log
#1078
Conversation
This commit affects only mozc_tip{32,64}.dll build with debug mode. There must be no behavior change in release build. Currently 'Mozc_tsf_ui.log' is created only when NDEBUG is defined. With this commit we stop creating it even when NDEBUG is defined. This addresses a crash issue in debug builds discussed in google#1077. See google#856 about why absl::LocalTimeZone cannot be used in Windows right now. Note that this commit may also help us diagnose google#1076, where Windows.Storage.OneCore.dll looks to be intercepting certain Win32 file I/O API calls in AppContainer processes then trigger RoInitialize as needed. Creating 'Mozc_tsf_ui.log' only in debug builds can make our debugging more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi yukawa,
Thank you for your PR!
@@ -511,8 +510,6 @@ class TipTextServiceImpl | |||
StorePointerForCurrentThread(this); | |||
|
|||
HRESULT result = E_UNEXPECTED; | |||
RegisterLogFileSink( | |||
FileUtil::JoinPath(SystemUtil::GetLoggingDirectory(), kLogFileName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about commenting out them with a note why the log is not stored into a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually wondering if we can permanently deprecate the log file, as it's not reliable (we cannot write file from AppContainer process) and I suspect no one is checking it as it is only available in debug build. At least I in general do not rely on log files for debugging.
Are you still using *.log
from other processes? I'm actually leaning towards deprecating other logging files, but it's a separate discussion after fixing this obvious crash bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine to permanently deprecate the log file.
I'd like to prevent anyone from creating the log file again in the future, since it may cause the crash issue again.
So adding a note instead of the comment out is also fine with me.
Description
This is a quick fix for #1077.
While #1077 itself is not the root cause of #1076, merging this commit would also help debugging #1076 with debug builds.
Issue IDs
#1077
Steps to test new behaviors
A clear and concise description about how to verify new behaviors.
Mozc64.msi
Without this commit, the app crashes because of the same reason as #856.