Closed
Bug 845342
Opened 11 years ago
Closed 11 years ago
Move memory dumps to the downloads dir
Categories
(Toolkit :: about:memory, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(1 file)
4.65 KB,
patch
|
n.nethercote
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We currently dump the about:memory dumps to the OS temp dir and do some android-specific umask fiddling to make these dumps world-readable. Instead we can just dump to a dir on the sdcard. There is the possibility that the external storage will not be available (i.e. sd card removed) but as this is primarily a debugging feature it shouldn't matter so much. I tested the patch locally and ensured the memory dump comes out in /sdcard/Downloads on my device. Try build at https://tbpl.mozilla.org/?tree=Try&rev=87cfa4de00c9 to ensure it builds everywhere else.
Attachment #718434 -
Flags: review?(n.nethercote)
Comment 1•11 years ago
|
||
Comment on attachment 718434 [details] [diff] [review] Patch Review of attachment 718434 [details] [diff] [review]: ----------------------------------------------------------------- I'll give r+ because this is a functional improvement and the ugliness of the code you've added is no worse than the ugliness of the code you removed :) But it would be better to factor out the duplication. ::: xpcom/base/nsCycleCollector.cpp @@ +1612,5 @@ > + NS_NewNativeLocalFile(nsCString(env), /* followLinks = */ true, > + getter_AddRefs(logFile)); > + } > +#endif > + if (!logFile) { So we weren't checking if logFile was NULL before? Seems like a worthwhile change. ::: xpcom/base/nsMemoryInfoDumper.cpp @@ +742,5 @@ > +#ifdef ANDROID > + // For Android, first try the downloads directory which is world-readable > + // rather than the temp directory which is not. > + if (char *env = PR_GetEnv("DOWNLOADS_DIRECTORY")) { > + NS_NewNativeLocalFile(nsCString(env), /* followLinks = */ true, aFile); It would be nicer if this repeated behaviour was factored out. Can this function be exported and used by nsCycleCollector.cpp? Or could you modify NS_GetSpecialDirectory to try the downloads directory first?
Attachment #718434 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > So we weren't checking if logFile was NULL before? Seems like a worthwhile > change. The old code set logFile in one of two branches, and had an assert afterwards to ensure it was non-null. I wanted to keep the behaviour where the code uses $MOZ_CC_LOG_DIRECTORY if it is set, but then fall back to DOWNLOADS_DIRECTORY (on Android) and OS_TEMP_DIR progressively so I replaced the branches with a linear sequence of checks guarded by the null checks. It seemed like the most appropriate idiom for the new structure of the code. > ::: xpcom/base/nsMemoryInfoDumper.cpp > It would be nicer if this repeated behaviour was factored out. Can this > function be exported and used by nsCycleCollector.cpp? Or could you modify > NS_GetSpecialDirectory to try the downloads directory first? There is some duplicated code there that can be refactored, but it feels wrong to expose a helper method in nsMemoryInfoDumper to the world without moving it to some utility file. I've filed bug 845804 as a follow-up for that as I expect it will take a couple of rounds to figure out the right way to do it. https://hg.mozilla.org/integration/mozilla-inbound/rev/36b60ec153af
Comment 3•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36b60ec153af
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 4•11 years ago
|
||
Please can someone explain to me why about:memory dumps need to be world readable?
Assignee | ||
Comment 5•11 years ago
|
||
The about:memory dumps are intended to be a diagnostic tool, so that we can look at instances of FF "in the wild" and see what's using up their memory. If the dump is not world-readable, then only the Android app that creates it is allowed to read it, and so pulling the dump and viewing it is impossible. (i.e. even a shell user cannot view the file)
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 718434 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: can't uplift bug 844832 Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): pretty low-risk; it just moves the folder to which some diagnostic files are dumped. only affects android String or UUID changes made by this patch: none
Attachment #718434 -
Flags: approval-mozilla-beta?
Attachment #718434 -
Flags: approval-mozilla-aurora?
Comment 7•11 years ago
|
||
Comment on attachment 718434 [details] [diff] [review] Patch Low risk and android only, approving for uplift since this is needed for bug 844832 uplift in FF20
Attachment #718434 -
Flags: approval-mozilla-beta?
Attachment #718434 -
Flags: approval-mozilla-beta+
Attachment #718434 -
Flags: approval-mozilla-aurora?
Attachment #718434 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/633557c901a9 https://hg.mozilla.org/releases/mozilla-beta/rev/49237d1425f5
You need to log in
before you can comment on or make changes to this bug.
Description
•