Closed Bug 1166239 Opened 9 years ago Closed 9 years ago

possible memory leak in report modifier mixin

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: pyang, Assigned: shinglyu)

References

Details

(Keywords: pi-marionette-runner)

Attachments

(1 file)

in __init__ of ./runner/mixins/reporting.py
class HTMLReportingTestResultMixin(object):
    def __init__(self, *args, **kwargs):
        self.result_modifiers.append(self.html_modifier)

HTMLReportingTestResultMixin(self) -> self.result_modifiers -> self.html_modifier -> self
create circular refs


same pattern in __init__ of ./runner/mixins/b2g.py
class B2GTestResultMixin(object):
    def __init__(self, *args, **kwargs):
        self.result_modifiers.append(self.b2g_output_modifier)
2 possible solutions
- let result_modifiers a weakref list so gc can work on them
- modifier function should not be an instance function. make it out of class or classmethod should be ok.
Depends on: MTBF-2015Q1
Blocks: MTBF-2015Q1
No longer depends on: MTBF-2015Q1
before 
---- SUMMARY ------------------------------------------------------------------
before run                               active      0     B      average   pct
__main__.LeakyTestResult                    0      0     B      0     B    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    0      0     B      0     B    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    1     17.45 KB     17.45 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    2     26.21 KB     13.11 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    2     26.21 KB     13.11 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    3     34.97 KB     11.66 KB    0%
after run                                active      0     B      average   pct
__main__.LeakyTestResult                    0     80     B      0     B    0%
-------------------------------------------------------------------------------

after
---- SUMMARY ------------------------------------------------------------------
before run                               active      0     B      average   pct
__main__.LeakyTestResult                    0      0     B      0     B    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    0      0     B      0     B    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    1     17.42 KB     17.42 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    1     17.42 KB     17.42 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    1     17.44 KB     17.44 KB    0%
Before runner init                       active      0     B      average   pct
__main__.LeakyTestResult                    1     17.45 KB     17.45 KB    0%
after run                                active      0     B      average   pct
__main__.LeakyTestResult                    0     80     B      0     B    0%
-------------------------------------------------------------------------------

Notice that before this patch, the number of TestResult object will grow as the test runs multiple times. After applying the patch, the number of active TestResult objects will remain one.
Attachment #8657698 - Flags: review?(ato)
Comment on attachment 8657698 [details] [diff] [review]
bug-1166239-fix.patch

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

Overall I feel that this isn’t the Right Solution, but because this is so horribly coded in the first place, I can’t see this doing more harm.
Attachment #8657698 - Flags: review?(ato) → review+
When exception occurs, marionette will dump a lot and place them into leaked objects.
(In reply to Andreas Tolfsen from comment #4)

I agree, I'll open a follow up bug to remind myself (or anyone) to refactor it.

Thanks for the r+.

> Review of attachment 8657698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall I feel that this isn’t the Right Solution, but because this is so 
> horribly coded in the first place, I can’t see this doing more harm.
Keywords: checkin-needed
Assignee: nobody → slyu
See Also: → 1202529
https://hg.mozilla.org/mozilla-central/rev/2e38346f89af
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.