Last Comment Bug 714286 - Cycle collection log should be written to a place where the process can write to
: Cycle collection log should be written to a place where the process can write to
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla12
Assigned To: :Ehsan Akhgari
: Nathan Froyd [:froydnj]
Depends on:
  Show dependency treegraph
Reported: 2011-12-30 08:02 PST by :Ehsan Akhgari
Modified: 2012-01-04 06:25 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (v1) (1.38 KB, patch)
2011-12-30 08:04 PST, :Ehsan Akhgari
bugs: review+
Details | Diff | Splinter Review
Patch (v2) (1.99 KB, patch)
2011-12-30 14:00 PST, :Ehsan Akhgari
continuation: review+
Details | Diff | Splinter Review
Windows fix followup (1.18 KB, patch)
2012-01-03 15:44 PST, :Ehsan Akhgari
continuation: review+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2011-12-30 08:02:50 PST
Also, it's a good idea to print the full path of the log file to the error console so that the user wouldn't need to search their entire file system looking for the log file.

I have a patch for this.
Comment 1 User image :Ehsan Akhgari 2011-12-30 08:04:10 PST
Created attachment 584968 [details] [diff] [review]
Patch (v1)
Comment 2 User image Andrew McCreight [:mccr8] 2011-12-30 08:58:03 PST
Could you add some kind of #ifdef or something that lets you easily revert to the old behavior of the file name generation with a recompile? I think this is an improvement for people doing a one-off CC dump, but when you are dumping every CC, and doing multiple runs, it is helpful to have them in a predictable location.  The console logging is fine either way.
Comment 3 User image :Ehsan Akhgari 2011-12-30 10:46:06 PST
I'm not sure what you mean.  The location is predictable, it's the temp directory.  Arguably it's more predictable than before, since at least on Windows the current directory changes when you invoke things like the file picker dialog.  :-)
Comment 4 User image Andrew McCreight [:mccr8] 2011-12-30 12:23:00 PST
The directory part sounds reasonable.  How does tmpnam affect the file name itself?
Comment 5 User image :Ehsan Akhgari 2011-12-30 13:47:46 PST
tmpnam() returns the full path name to a temp file, something like "/tmp/mygarbage.tmp".  I can walk the string back from the end to find the first directory separator, and then append cc-edges to that, so that we would end up with "/tmp/cc-edges.N.MMMM.log" instead of "/tmp/mygarbage.tmp-cc-edges.N.MMMM.log".  Is that what you want?
Comment 6 User image :Ehsan Akhgari 2011-12-30 14:00:40 PST
Created attachment 585044 [details] [diff] [review]
Patch (v2)

Implements comment 5.
Comment 7 User image Andrew McCreight [:mccr8] 2011-12-30 20:08:16 PST
Comment on attachment 585044 [details] [diff] [review]
Patch (v2)

Review of attachment 585044 [details] [diff] [review]:

Thanks, that's better than the current behavior for my purposes!  Because you can end up with the logs being dumped in some weird subdirectory if you invoke Firefox using Mochitest.
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-01-01 14:53:31 PST
Could you update
Comment 10 User image Phil Ringnalda (:philor) 2012-01-01 21:10:58 PST
Comment 12 User image :Ehsan Akhgari 2012-01-03 15:43:56 PST
tmpnam() is broken on Windows. :(  I have a patch for this.
Comment 13 User image :Ehsan Akhgari 2012-01-03 15:44:30 PST
Created attachment 585589 [details] [diff] [review]
Windows fix followup
Comment 14 User image Andrew McCreight [:mccr8] 2012-01-03 16:55:11 PST
Comment on attachment 585589 [details] [diff] [review]
Windows fix followup

Review of attachment 585589 [details] [diff] [review]:

Looking at MS's documentation, GetTempPath returns a path that ends in \, whereas I think the other code path removes the trailing directory separator.  On Windows, it looks like the path will end up being something like C:\temp\\cc-edges.txt .  Is that a problem?
Comment 15 User image :Ehsan Akhgari 2012-01-03 18:30:04 PST
No.  (I actually tested the patch on Windows this time.  ;-))
Comment 16 User image Marco Bonardo [::mak] 2012-01-04 04:57:19 PST
Comment 17 User image Jim Jeffery not reading bug-mail 1/2/11 2012-01-04 06:25:14 PST
Verified Fixed using latest hourly m-c win32 build on Win7 x64 

today's m-i -> m-c merge

Note You need to log in before you can comment on or make changes to this bug.