845804 - Refactor duplicated code in nsMemoryInfoDumper and nsCycleCollector.cpp

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

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

The code in the OpenTempFile function in nsMemoryInfoDumper is pretty much duplicated in nsCycleCollectorLogger::CreateTempFile in nsCycleCollector.cpp and should be refactored.

Comment 1

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

Attachment: Patch

This is what I imagine you meant in bug 845342 comment 1, but it seems wrong to have this random OpenTempFile floating around in no namespace. Is there a better place to move it to?

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 718966 [details] [diff] [review]
Patch

Review of attachment 718966 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely heading in the right direction, but shouldn't nsMemoryInfoDumper.cpp call OpenTempFile() somewhere?

jlebar might have an opinion about the best place for OpenTempFile.  It feels like it belongs in some lower-level header that can be imported by both nsMemoryInfoDumper.cpp and nsCycleCollector.cpp.

Comment 3

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

nsMemoryInfoDumper already calls OpenTempFile; I didn't modify the signature of the function so the existing calls to it don't show up in the patch. Or am I misunderstanding your question?

Comment 4

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

Comment on attachment 718966 [details] [diff] [review]
Patch

:jlebar, any thoughts on where I should relocate OpenTempFile to so that it can be cleanly reused across nsCycleCollector and nsMemoryInfoDumper?

Comment 5

[Justin Lebar (not reading bugmail)]

Maybe OpenTempFile could be a static method on nsMemoryInfoDumper?  I'd like that better than a global function, since that lacks any context as to why you're opening a temp file.

Comment 6

[Nicholas Nethercote [inactive]]

> nsMemoryInfoDumper already calls OpenTempFile; I didn't modify the signature
> of the function so the existing calls to it don't show up in the patch.

Right!  Carry on :)

Comment 7

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

Attachment: Patch (v2)

Made it a static method on nsMemoryInfoDumper

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 719603 [details] [diff] [review]
Patch (v2)

Review of attachment 719603 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment below addressed.

::: xpcom/base/nsCycleCollector.cpp
@@ +1412,4 @@
>              NS_NewNativeLocalFile(nsCString(env), /* followLinks = */ true,
>                                    getter_AddRefs(logFile));
>          }
> +        nsresult rv = nsMemoryInfoDumper::OpenTempFile(filename,

Don't you need to check |logFile| before calling OpenTempFile()?

Comment 9

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

(In reply to Nicholas Nethercote [:njn] from comment #8)
> 
> Don't you need to check |logFile| before calling OpenTempFile()?

No, because OpenTempFile does the check internally. Note that we want to execute the tail end of OpenTempFile regardless of whether logFile is set or not at the call site; it's just that it if it's not set we want to do the additional work of finding the temp dir. It is a little confusing though, I can add some documentation to that effect.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eda65741914

Comment 11

[Ed Morley [:emorley]]

Backed out for xpcshell failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1eda65741914

https://hg.mozilla.org/integration/mozilla-inbound/rev/fedd96a37a7c

Comment 12

[Ed Morley [:emorley]]

Sorry for the false alarm here! :-0

Comment 13

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

No problem :)

RyanVM re-landed this as https://hg.mozilla.org/integration/mozilla-inbound/rev/8392e41895df once it was determined to be free of blame for the xpcshell failures.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8392e41895df