1306920 - Make sure log file is closed when clearing it at runtime

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Valentin Gosu [:valentin] (he/him)]

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 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Make sure log file is closed when clearing it at runtime

Comment 2

[Nathan Froyd [:froydnj]]

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".

Comment 3

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09bf1dc1f903

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cedca0e7248