833939 - Write to console.log file only if XRE_CONSOLE_LOG is set

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P3)

Description

[Marco Bonardo [:mak]]

Dolske found he had a 400MB console.log in his UserAppData folder, and Jesse reported a multi-GB one.
These are created only by debug builds, and all the console.log is appended.
if XRE_CONSOLE_LOG is set, the file is created at the given path, otherwise UserAppData is used, though due to this hidden behavior it's likely people who needs this data has it on a better location, and people who has it in the default location is unaware of it.

Would be much better to write the console log only if XRE_CONSOLE_LOG is set, so that each developer can choose whether he wants to use it and where to log.

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsConsoleWriter.cpp#21

I think would also make sense to remove the file from UserAppData if XRE_CONSOLE_LOG is not set and the file exists, so we cleanup existing profiles, though it may be worth notify people in mozilla.dev.platforms, to avoid dataloss.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: patch v1

This is the first time I'm touching Moz C++ code, please review carefully!

Comment 2

[Mike de Boer [:mikedeboer]]

I assigned this to Dolske, because Marco is away.

Comment 3

[Marco Bonardo [:mak]]

I think it may be worth to have a part 2 patch that removes existing logs, since someone has multiGB logs they are not aware of, though I'm not sure if it's a good idea to do that here, since would run at each shutdown, even if it's just for debug builds.
Any idea where we could hook a removal to run just once?
Otherwise we could just put out a blog post on planet or m.d.platform and whoever is interested will just do manual cleanup.

Comment 4

[Mike de Boer [:mikedeboer]]

IMHO a blog/ mailing list post would be a better idea, because adding code for this will add another layer of obscure, hidden behavior just like the very thing we're trying to solve.
If a dev at some point tries the download the next best movie torrent and he's out of space... a single multi-GB file will prolly show up.

Comment 5

[Justin Dolske [:Dolske]]

Hmmm. I went down the rabbit hole looking at what gLogConsoleErrors is doing. Removing that check makes a bunch of other code dead.

It appears the original intent in bug 291129 was to log early startup errors to a file, since there's no window for showing errors. Do we still want to keep that ability? I checked the IRC logs when we talked about this, and it wasn't really clear if Benjamin was considering the original case or not.

So we've got a couple choices:

1) Back out all of 291129, if "occasionally" means this isn't worth keeping. I would hope errors in early startup are unlikely to hit end-users, and developers can use a debug build to get the other usual logging for diagnosis. XRE_CONSOLE_LOG would be entirely unused.

2) Minimally fix this bug by _only_ changing gLogConsoleErrors so it's always initialized to |false|, even in debug builds. This prevents logging in normal usage, but the NS_ENSURE_SUCCESS_LOG / NS_ENSURE_TRUE_LOG macros can still still enable the log file for startup errors. Setting XRE_CONSOLE_LOG would still force the log file to be used.

[Note to mostly-self: This code is named a bit confusingly, because all it really does is dump a snapshot of the error console's content to a file when XPCOM shuts down. It's doesn't actually generate a running log.]

Either would fix the problem I ran into. Benjamin, do you have a preference here?

Comment 6

[Justin Dolske [:Dolske]]

Oh, my "occasionally" quote in option one doesn't make since without the IRC log. :)

11:51:15 <Jesse> bsmedberg, how often do you actually use those files?
11:51:35 <bsmedberg> uhh...
11:51:39 <bsmedberg> I don't remember, why?
11:51:53 <bsmedberg> I mean, I've occasionally told people to use those logs
11:51:56 <Jesse> because they're tens of GB on some developer machines
11:52:08 <bsmedberg> they aren't on by default...
11:52:32 <Jesse> they seem to be on by default in debug builds
11:52:39 <dolske> right http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#1062
11:53:03 <dolske> I guess it's not a big deal if it's debug-only, but seems kind of not-useful.
11:53:25 <mak> maybe should write only if XRE_CONSOLE_LOG is set to something
11:53:29 <dolske> I bet most people don't even know about it (clearly I didn't ;)
11:53:33 <mak> while now it writes in both cases
11:53:46 <mconley> debug builds are in ur profile, eatin' ur space
11:53:47 <mak> but if XRE_CONSOLE_LOG is not set, it writes to the data folder
11:53:54 <bsmedberg> I kinda thought that this only happened with XRE_CONSOLE_LOG
11:54:08 <bsmedberg> or if one of those special things failed, because we wanted to log if no UI came up

Comment 7

[Benjamin Smedberg]

(In reply to Justin Dolske [:Dolske] from comment #5)
> Hmmm. I went down the rabbit hole looking at what gLogConsoleErrors is
> doing. Removing that check makes a bunch of other code dead.
> 
> It appears the original intent in bug 291129 was to log early startup errors
> to a file, since there's no window for showing errors. Do we still want to
> keep that ability?

Yes. That's what this is for.

> 2) Minimally fix this bug by _only_ changing gLogConsoleErrors so it's
> always initialized to |false|, even in debug builds. This prevents logging

That's what I said on IRC, think!

> [Note to mostly-self: This code is named a bit confusingly, because all it
> really does is dump a snapshot of the error console's content to a file when
> XPCOM shuts down. It's doesn't actually generate a running log.]

Yeah, it was actually supposed to do a running log, but the current system was a compromise.

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 718908 [details] [diff] [review]
patch v1

Ok, wasn't clear about the IRC bit.

So let's go with #2 in comment 5.

Comment 9

[Justin Dolske [:Dolske]]

I also agree with comment 4 -- not worth it to add code to remove any existing console.log. We've lived with this for 8 years. :) A simple newsgroup post is adequate.

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: v2, initializing gLogConsoleErrors to FALSE, also in debug builds

This second patch fixes the bug as proposed in option #2 in comment #5.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3bed297ae8

Comment 12

[Mike de Boer [:mikedeboer]]

should this email be sent to the firefox-dev mailing list?

Comment 13

[Marco Bonardo [:mak]]

this is not just for firefox developers, probably mozilla.dev.platform is a better place.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc3bed297ae8