519570 - Leaktest + Electrolysis: count leaks for each subprocess

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Benjamin Smedberg]

The leaktest framework should be able to measure and warn about leaks from each process. Currently the leak output is intermingled and is likely to cause problems.

Comment 1

[Jonathan Griffin (:jgriffin)]

Currently, all leaks are dumped into runtests_leaks.log (from setting XPCOM_MEM_BLOAT_LOG).  I'm thinking each child process could use its own log, named [XPCOM_MEM_BLOAT_LOG]_childX.log, where X starts at 0 and increments as needed.  It would be easy then to adapt processLeakLog() to read all the log files and write out the data in a reasonable way.

The logs are written in nsTraceRefcntImpl.cpp.  It seems it can be easily adapted per the above.  It looks like I could use XRE_GetProcessType() to determine whether I was in a child process or not. 

Does this sound like a good approach?

Comment 2

[Benjamin Smedberg]

Yes.

Comment 3

[Jonathan Griffin (:jgriffin)]

Attachment: patch

This patch causes separate leak logs to be spun off for each child process.  Will post a separate patch to handle the python end of the arrangement.

Comment 4

[Jonathan Griffin (:jgriffin)]

Attachment: python patch

Here's the python half of the patch.  

In the log, this adds a process identifier to the first line, e.g.

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, Primary Process
 - or -
== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, Child Process 0

We could also add this process identifier into the TEST-UNEXPECTED-FAIL lines, but this may add too much clutter:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | child 0 | leaked 1 instance of nsStringBuffer with size 8 bytes

Comment 5

[Benjamin Smedberg]

Comment on attachment 408915 [details] [diff] [review]
patch

>diff --git a/xpcom/base/nsTraceRefcntImpl.cpp b/xpcom/base/nsTraceRefcntImpl.cpp

>     else {
>-      FILE *stream = ::fopen(value, "w");
>+      FILE *stream;
>+      int fnamesize = strlen(value) + 20;
>+      char* fname = (char*)PR_Malloc(fnamesize + 1);
>+      strncpy(fname, value, fnamesize);

Better memory management with strings:
nsCAutoString fname(value);

if (!default) {
  fname.AppendLiteral("_child");
  fname.AppendInt(counter);


>+#ifdef MOZ_IPC
>+      if (XRE_GetProcessType() != GeckoProcessType_Default) {
>+        FILE *exists = NULL;

This whole block seems complicated and unfortunate. It's like nsIFile.createUnique but it's still subject to race conditions. So I'd prefer that you either use nsIFile.createUnique, or use an idenfitier which is actually unique per-process, for example the PID. Unfortunately we don't seem to have a cross-platform way of getting the PID, but I'd be fine if you just did

#ifdef XP_WIN
#include <process.h>
#define getpid _getpid
#else
#include <unistd.h>
#endif

And then used getpid. This has the added advantage that you can associate the leak log with the PID.

Comment 6

[Jonathan Griffin (:jgriffin)]

Attachment: patch v2

Thanks for the review, bsmedberg.  Patch updated per your comments.  Python patch will be updated too, to match.

Comment 7

[Jonathan Griffin (:jgriffin)]

Attachment: patch v3

This version uses the process type string instead of the generic "child" in the logfile name, and adds the PID and process type to the header inside the log.

Comment 8

[Jonathan Griffin (:jgriffin)]

Attachment: python patch v2

Here's the python half.  This does add the process type and PID to the TEST-UNEXPECTED-FAIL lines, for non-default processes, e.g:

TEST-UNEXPECTED-FAIL | plugin process 552 | automationutils.processLeakLog() | l
eaked 8 bytes during test execution

Comment 9

[Benjamin Smedberg]

Comment on attachment 409772 [details] [diff] [review]
python patch v2

>diff --git a/build/automationutils.py b/build/automationutils.py

>-def processLeakLog(leakLogFile, leakThreshold = 0):
>+def processLeakLog(leakLogFile, leakThreshold = 0, checkChildren = False):

No reason to default to false: just remove this parameter and always checkChildren.


>+  (leakLogFileDir, leakFileBase) = os.path.split(leakLogFile)
>+  pidRegExp = ".*?_([a-z]*)_pid(\d*)$"
>+  if checkChildren and leakFileBase[-4:] == ".log":
>+    leakFileBase = leakFileBase[:-4]
>+    pidRegExp = ".*?_([a-z]*)_pid(\d*).log$"

You will be better off if you precompile the regexp objects. Also, please use r'' quoting for regexps: the \d in this case is being interpreted as a string quote, not a regexp quote!

  pidRegExp = re.compile(r'.*?_([a-z]*)_pid(\d*)$')

>+  for fileName in os.listdir(leakLogFileDir):

This re-indent makes the entire file get pretty indented. Perhaps the logic for processing one specific file could be separated out into a function?

Comment 10

[Jonathan Griffin (:jgriffin)]

Attachment: python patch v3

Comment 11

[Jonathan Griffin (:jgriffin)]

Pushed as http://hg.mozilla.org/projects/electrolysis/rev/246ea55e42c3