Make setting options.leakThresholds and options.ignoreMissingLeaks defaults less fragile

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
I was missing two places where this had to be done, plus there was an additional place where I wasn't setting them correctly at all.  It is also scary because it apparently fails without turning the tree orange when you forget to do it, which could potentially cause us to hide leaks.

I think the easiest way to fix this is to change
  options.leakThresholds to options.get("leakThresholds", {})
  options.ignoreMissingLeaks to options.get("ignoreMissingLeaks", [])

or something like that.  Then if somebody forgets to set it, we default to the strictest setting.  We'd also want to rip out the existing code that just uses these default values.
(Assignee)

Updated

4 years ago
Assignee: nobody → continuation
(Assignee)

Comment 1

4 years ago
Created attachment 8500548 [details] [diff] [review]
processLeakLog should come up with reasonable defaults itself.

Instead of grabbing attributes off options at every call site, pass
in the options object to processLeakLog, and attempt to get the attributes
there. If not present, use a restrictive default value.

This will prevent silent harness failures if one of the many ways to invoke
processLeakLog fails to set up these options, and makes it so they
don't have to set it up if they don't care.

try run: https://tbpl.mozilla.org/?tree=Try&rev=e1c2327be395

I checked that Android, Linux and B2G don't throw in Python like they did with older stuff I had.
Attachment #8500548 - Flags: review?(jmaher)
Comment on attachment 8500548 [details] [diff] [review]
processLeakLog should come up with reasonable defaults itself.

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

this patch seems to be much more complete.  I know this is a frustrating system to work within, so many different variations of the same thing.

::: layout/tools/reftest/runreftest.py
@@ +510,5 @@
>          self.error("cannot specify focusFilterMode with parallel tests")
>        if options.debugger is not None:
>          self.error("cannot specify a debugger with parallel tests")
>  
>      options.leakThresholds = {"default": options.defaultLeakThreshold}

do we still need this instance of leakThreadholds?
Attachment #8500548 - Flags: review?(jmaher) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Joel Maher (:jmaher) from comment #2)
> this patch seems to be much more complete.  I know this is a frustrating
> system to work within, so many different variations of the same thing.

I think I'm starting to learn where all the places are, so hopefully it will be a little easier for me in the future. ;)

> >      options.leakThresholds = {"default": options.defaultLeakThreshold}
> do we still need this instance of leakThreadholds?

Yeah, if in theory somebody actually set this command line variable, then we need to set this.
https://hg.mozilla.org/mozilla-central/rev/1b74418f4245
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.