Closed Bug 845342 Opened 8 years ago Closed 8 years ago

Move memory dumps to the downloads dir

Categories

(Toolkit :: about:memory, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter 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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/36b60ec153af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Please can someone explain to me why about:memory dumps need to be world readable?
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)
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 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+
You need to log in before you can comment on or make changes to this bug.