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)

47 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

Attachments

(1 file)

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.
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+
Priority: -- → P3
(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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: