Closed
Bug 833939
Opened 11 years ago
Closed 11 years ago
Write to console.log file only if XRE_CONSOLE_LOG is set
Categories
(Toolkit :: Startup and Profile System, defect, P3)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: mak, Assigned: mikedeboer)
References
Details
Attachments
(1 file, 1 obsolete file)
1.12 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Component: Error Console → Startup and Profile System
Priority: -- → P3
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mdeboer
Assignee | ||
Comment 1•11 years ago
|
||
This is the first time I'm touching Moz C++ code, please review carefully!
Attachment #718908 -
Flags: review?(dolske)
Assignee | ||
Comment 2•11 years ago
|
||
I assigned this to Dolske, because Marco is away.
Reporter | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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?
Flags: needinfo?(benjamin)
Comment 6•11 years ago
|
||
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•11 years ago
|
||
(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.
Flags: needinfo?(benjamin)
Comment 8•11 years ago
|
||
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.
Attachment #718908 -
Flags: review?(dolske) → review-
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
This second patch fixes the bug as proposed in option #2 in comment #5.
Attachment #718908 -
Attachment is obsolete: true
Attachment #720619 -
Flags: review?(dolske)
Attachment #720619 -
Flags: review?(benjamin)
Updated•11 years ago
|
Attachment #720619 -
Flags: review?(dolske)
Attachment #720619 -
Flags: review?(benjamin)
Attachment #720619 -
Flags: review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3bed297ae8
Keywords: checkin-needed
Assignee | ||
Comment 12•11 years ago
|
||
should this email be sent to the firefox-dev mailing list?
Reporter | ||
Comment 13•11 years ago
|
||
this is not just for firefox developers, probably mozilla.dev.platform is a better place.
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cc3bed297ae8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•