Make sure log file is closed when clearing it at runtime

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: valentin.gosu, Assigned: valentin.gosu)

Tracking

47 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cedca0e724800f47dc7d0b3ed31683f18829537
Bug 1306920 - Make sure log file is closed when clearing it at runtime r=nfroyd
https://hg.mozilla.org/mozilla-central/rev/7cedca0e7248
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.