Closed
Bug 1166239
Opened 9 years ago
Closed 9 years ago
possible memory leak in report modifier mixin
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: pyang, Assigned: shinglyu)
References
Details
(Keywords: pi-marionette-runner)
Attachments
(1 file)
1.38 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Depends on: MTBF-2015Q1
Reporter | ||
Updated•9 years ago
|
Blocks: MTBF-2015Q1
No longer depends on: MTBF-2015Q1
Updated•9 years ago
|
Keywords: ateam-marionette-runner
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e24866ccd95
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
Reporter | ||
Comment 5•9 years ago
|
||
When exception occurs, marionette will dump a lot and place them into leaked objects.
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → slyu
https://hg.mozilla.org/mozilla-central/rev/2e38346f89af
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•