Closed
Bug 1306920
Opened 8 years ago
Closed 8 years ago
Make sure log file is closed when clearing it at runtime
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: valentin, Assigned: valentin)
References
Details
Attachments
(1 file)
1.80 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
I managed to hit an assertion in LogModuleManager::SetLogFile
MOZ_ASSERT(!prevFile, "Should be null because rotation is not allowed");
The reason is that I set the log file, cleared it, then set it again.
The first log file wasn't actually released, because I hadn't set any log modules.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8796888 -
Flags: review?(nfroyd)
![]() |
||
Comment 2•8 years ago
|
||
Comment on attachment 8796888 [details] [diff] [review]
Make sure log file is closed when clearing it at runtime
Review of attachment 8796888 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the changes below, I guess. Surely there has to be a better way to do this.
::: xpcom/base/Logging.cpp
@@ +286,5 @@
> MOZ_ASSERT(!prevFile, "Should be null because rotation is not allowed");
> +
> + // If we just cleared the log file, we must force print, in order to trigger
> + // the closing and release of mToReleaseFile.
> + if (!newFile) {
Ugh. Forcing a print shouldn't really be necessary, but I don't know that spinning to ensure everybody is out of Print() is really any better.
Can we check |!newFile && oldFile| here, so we're not cluttering up the log if we don't need to?
@@ +289,5 @@
> + // the closing and release of mToReleaseFile.
> + if (!newFile) {
> + va_list va;
> + empty_va(&va);
> + Print("Logger", LogLevel::Info, "\n", va);
Nit: why doesn't we make this more informative: "Flushing old log files".
Attachment #8796888 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #2)
> > + if (!newFile) {
> Ugh. Forcing a print shouldn't really be necessary, but I don't know that
> spinning to ensure everybody is out of Print() is really any better.
>
> Can we check |!newFile && oldFile| here, so we're not cluttering up the log
> if we don't need to?
I don't think cluttering would be a problem. This method shouldn't be called too often.
I have however changed the condition to if (oldFile) - as it should be called to clear it, regardless if there is a new file or not.
Assignee | ||
Comment 5•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cedca0e724800f47dc7d0b3ed31683f18829537
Bug 1306920 - Make sure log file is closed when clearing it at runtime r=nfroyd
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•