845342 - Move memory dumps to the downloads dir

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

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.

Comment 1

[Nicholas Nethercote [inactive]]

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?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(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

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/36b60ec153af

Comment 4

[Mark Goodwin [:mgoodwin]]

Please can someone explain to me why about:memory dumps need to be world readable?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/633557c901a9
https://hg.mozilla.org/releases/mozilla-beta/rev/49237d1425f5