Closed Bug 1069689 Opened 10 years ago Closed 10 years ago

A few minor processSingleLeakFile cleanups

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(3 files, 1 obsolete file)

      No description provided.
I don't know if there's some weird reason to do this in Python, but this regexp is just used with a search so a leading .* isn't useful.  Also the ? is redundant as far as I can tell.
Attachment #8492247 - Flags: review?(jmaher)
This is kind of a silly patch but it annoyed me a little.
Attachment #8492249 - Flags: review?(jmaher)
The code that decides on the name of the leak log file is here:
  http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcnt.cpp#736

The process type PID format is not used when it is a default process, ergo if the file name doesn't match the PID format it is a default process. This just makes the output a little nicer, and avoids some annoying None checking.
Attachment #8492253 - Flags: review?(jmaher)
Attachment #8492253 - Attachment is obsolete: true
Attachment #8492253 - Flags: review?(jmaher)
Attachment #8492284 - Flags: review?(jmaher)
Comment on attachment 8492247 [details] [diff] [review]
part 1 - .*? at the start of a search regexp isn't useful.

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

pending my question, r=me

::: build/automationutils.py
@@ +330,3 @@
>    if leakFileBase[-4:] == ".log":
>      leakFileBase = leakFileBase[:-4]
> +    fileNameRegExp = re.compile(r"_([a-z]*)_pid\d*.log$")

are there any concerns with platform specific leak logs?  android, windows, b2g, etc.?
Attachment #8492247 - Flags: review?(jmaher) → review+
Well, for part 1, my patch should not affect the behavior at all. In general, the one weird platform specific thing that I'm aware of is that B2G has to copy the leak log off of the device, and it copies it into a temp file (from a hard-coded file name on the device), so you end up with the default process log being copied into some bizarrely named file. However, there's no attempt to copy leak logs off the device from any other process, so that's not a problem yet.

Whatever future Marionette code has to deal with that will need to copy things into a file matching the format here, but it should probably do that anyways to produce useful messages.
Comment on attachment 8492249 [details] [diff] [review]
part 2 - Don't generate a regexp then throw it away.

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

much much better!
Attachment #8492249 - Flags: review?(jmaher) → review+
Comment on attachment 8492284 [details] [diff] [review]
part 3 - Make 'default' the default process type.

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

are there any concerns with integration into tbpl with this?  I assume this might change some bugs that have leaks as the error signature will change.
Attachment #8492284 - Flags: review?(jmaher) → review+
Can you add a comment as to where we expect the log and what assumptions we have about the name and what is in it?  This is sort of backfilling a lack of documentation.
(In reply to Joel Maher (:jmaher) from comment #8)
> are there any concerns with integration into tbpl with this?  I assume this
> might change some bugs that have leaks as the error signature will change.

Hmm, that's true.  I think it will still be close enough.

Ryan, do you think it will be a problem for starring if failures like
  TEST-UNEXPECTED-FAIL | leakcheck | 549403 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ...)
become
  TEST-UNEXPECTED-FAIL | leakcheck | default process: 549403 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ...)
Flags: needinfo?(ryanvm)
Shouldn't make a difference. The search term TBPL would use for your example would be "AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ..." so I don't forsee anything changing.

https://mxr.mozilla.org/webtools-central/source/tbpl/php/inc/AnnotatedSummaryGenerator.php#85
Flags: needinfo?(ryanvm)
Joel, I could also just make it so the process string stays the same as it is now when the processType is default.  So, something like:

if processType == "default":	
  processString = ""
else:
  processString = " %s process:" % processType

Then changing the output can be done separately if we decide that's a good idea.  I'm on the fence as to whether spamming "default process" everywhere is an improvement or not.
If there is no concern from the sheriffs, this helps us explicitly call out which process is leaking.  That seems like a good thing as we start dealing with e10s everywhere.  is 'default' the same as 'main' ?
(In reply to Joel Maher (:jmaher) from comment #13)
> If there is no concern from the sheriffs, this helps us explicitly call out
> which process is leaking. That seems like a good thing as we start dealing
> with e10s everywhere.  is 'default' the same as 'main' ?

Yeah, it seems like a good idea to specify the process type, but I don't know how useful "default" necessarily is to somebody trying to understand.  But I suppose it is standard terminology, and the alternative is to R
...the alternative is to require people dig into the code to know that blank == default.

Default is just the process type you get when you don't specify one otherwise.  This includes the main process, but can also be other ones for reasons I don't entirely understand.  One example is that in mochitest chrome we do some app tests which involve spinning up a whole new Gecko and doing stuff with that, and that new process is also a default process, but not the main process we're testing with.
Thanks for the fast reviews.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: