Closed
Bug 1217218
Opened 9 years ago
Closed 9 years ago
Intermittent LeakSanitizer | leak at nsPerformanceGroup::Make, nsPerformanceStatsService::nsPerformanceStatsService, nsPerformanceStatsServiceConstructor
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox45 fixed)
RESOLVED
FIXED
Firefox 45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: KWierso, Assigned: Yoric)
References
(Blocks 1 open bug)
Details
(Keywords: intermittent-failure, memory-leak, Whiteboard: [MemShrink])
Attachments
(1 file)
Assignee | ||
Comment 2•9 years ago
|
||
Most likely, yes.
Assignee | ||
Comment 3•9 years ago
|
||
I'll wait until we have a few more samples to determine what's going on.
Flags: needinfo?(dteller)
Comment 4•9 years ago
|
||
This is happening very frequently, across a number of test suites.
Comment 5•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #4) > This is happening very frequently, across a number of test suites. Well, all ASan. I just meant, in different mochitest chunks.
Comment 6•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3) > I'll wait until we have a few more samples to determine what's going on. Do you have a fix for this, or should bug 1208747 get backed out? Thanks.
Flags: needinfo?(dteller)
Assignee | ||
Comment 7•9 years ago
|
||
I still have only a single log to work on, so there are only so many things I can do at this stage. Andrew, is it always a leak of a single PerformanceGroup? However, based on this log, it looks like a single js::PerformanceGroup is leaking. There are very few structures that can hold references to a js::PerformanceGroup: - a JS Compartment holds one (released when the compartment is destroyed); - a JS Runtime holds a few (any leaking reference is released when the JS Runtime is destroyed); - the nsPerformanceGroupService holds one. There are also a few references on the stack. Could it be possible that a single JS Compartment is never garbage-collected? Could it be possible that we finish the process from a nested event loop which prevents the stack-based reference from being released?
Flags: needinfo?(dteller) → needinfo?(continuation)
Assignee | ||
Comment 8•9 years ago
|
||
Oh, I missed some information. I see which reference is not collected, I hope to have a patch shortly.
Comment 9•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7) > I still have only a single log to work on, so there are only so many things > I can do at this stage. If you click on the link in the "Orange factor" section in the upper right part of this page, you'll get a list of all of the times this has been marked.
Flags: needinfo?(continuation)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #9) > If you click on the link in the "Orange factor" section in the upper right > part of this page, you'll get a list of all of the times this has been > marked. I have no such link, either on Bugzilla or in the Treeherder link.
Assignee | ||
Comment 11•9 years ago
|
||
Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r?froydnj
Attachment #8677608 -
Flags: review?(nfroyd)
Comment 12•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10) > I have no such link, either on Bugzilla or in the Treeherder link. Here's the link: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1217218
Reporter | ||
Comment 13•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10) > (In reply to Andrew McCreight [:mccr8] from comment #9) > > If you click on the link in the "Orange factor" section in the upper right > > part of this page, you'll get a list of all of the times this has been > > marked. > > I have no such link, either on Bugzilla or in the Treeherder link. I believe that's not available with the new modal interface. (bug 1153105)
Comment hidden (Intermittent Failures Robot) |
Comment 15•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj https://reviewboard.mozilla.org/r/22973/#review20565
Attachment #8677608 -
Flags: review?(nfroyd) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dteller
Assignee | ||
Comment 16•9 years ago
|
||
I don't think this is really MemShrink, as solving this bug won't change the memory footprint of Firefox by a single byte.
Whiteboard: [MemShrink]
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r=froydnj
Attachment #8677608 -
Attachment description: MozReview Request: Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r?froydnj → MozReview Request: Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r=froydnj
Comment 18•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #16) > I don't think this is really MemShrink, as solving this bug won't change the > memory footprint of Firefox by a single byte. It obviously isn't a major issue, but maintaining the integrity of our memory testing infrastructure is an important part of monitoring Firefox's memory usage.
Whiteboard: [MemShrink]
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 21•9 years ago
|
||
I don't know why MozReview => Try doesn't work for me atm.
Assignee | ||
Comment 22•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adb0414c2149
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Attachment #8677608 -
Attachment description: MozReview Request: Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r=froydnj → MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r?froydnj
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r?froydnj In e10s tests, the content process version of nsPerformanceStatsService doesn't always shut down correctly. Apparently, this is due to content-child-shutdown not always being received - or possibly just not being received on time. We fix this by extending shutdown to also shutdown the service in reaction to xpcom-will-shutdown. This raises an additional issue with JS code being able to request a snapshot after shutdown of the service, so we add a sanity check to ensure that such calls always fail once the service is shutdown.
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 25•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f3bd205f87
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Oh, forgot to re-r? it.
Attachment #8677608 -
Flags: review?(nfroyd)
Comment 30•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Apparently reviewboard got confused because I had already reviewed the patch, or something?
Attachment #8677608 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 31•9 years ago
|
||
Yeah, see bug 1219646.
Assignee | ||
Comment 32•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f3bd205f87
Keywords: checkin-needed
Comment hidden (Intermittent Failures Robot) |
Comment 34•9 years ago
|
||
this failed to apply : patching file toolkit/components/perfmonitoring/nsPerformanceStats.cpp Hunk #4 FAILED at 501 Hunk #5 FAILED at 641 2 out of 5 hunks FAILED -- saving rejects to file toolkit/components/perfmonitoring/nsPerformanceStats.cpp.rej patch failed to apply could you rebase this too ?
Flags: needinfo?(dteller)
Keywords: checkin-needed
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r?froydnj In e10s tests, the content process version of nsPerformanceStatsService doesn't always shut down correctly. Apparently, this is due to content-child-shutdown not always being received - or possibly just not being received on time. We fix this by extending shutdown to also shutdown the service in reaction to xpcom-will-shutdown. This raises an additional issue with JS code being able to request a snapshot after shutdown of the service, so we add a sanity check to ensure that such calls always fail once the service is shutdown.
Attachment #8677608 -
Flags: review+
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj
Attachment #8677608 -
Attachment description: MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r?froydnj → MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8677608 [details] MozReview Request: Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj Bug 1217218 - Consolidating shutdown of nsPerformanceStatsService;r=froydnj
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Updated•9 years ago
|
Summary: Intermittent TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at nsPerformanceGroup::Make, nsPerformanceStatsService::nsPerformanceStatsService, nsPerformanceStatsServiceConstructor → Intermittent LeakSanitizer | leak at nsPerformanceGroup::Make, nsPerformanceStatsService::nsPerformanceStatsService, nsPerformanceStatsServiceConstructor
Comment 41•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2255c56d2d00
Keywords: checkin-needed
Comment hidden (Intermittent Failures Robot) |
Comment 43•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2255c56d2d00
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 44•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2255c56d2d00
status-b2g-v2.5:
--- → fixed
Comment 45•9 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Comment hidden (Intermittent Failures Robot) |
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•