Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed) Reporter
	Description • 9 years ago

https://treeherder.mozilla.org/logviewer.html#?job_id=5251702&repo=fx-team

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed) Reporter
	Comment 1 • 9 years ago

Possibly fallout from bug 1208747?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 2 • 9 years ago

Most likely, yes.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 3 • 9 years ago

I'll wait until we have a few more samples to determine what's going on.

	Andrew McCreight [:mccr8]
	Comment 4 • 9 years ago

This is happening very frequently, across a number of test suites.

	Andrew McCreight [:mccr8]
	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.

	Andrew McCreight [:mccr8]
	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.

	David Teller [:Yoric] - still alive but not very active 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?

	David Teller [:Yoric] - still alive but not very active 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.

	Andrew McCreight [:mccr8]
	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.

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 11 • 9 years ago

Bug 1217218 - nsPerformanceStatsService now clears the reference to mTopGroup in Dispose();r?froydnj

	Andrew McCreight [:mccr8]
	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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed) 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)

	Nathan Froyd [:froydnj]
	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

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active 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

	Andrew McCreight [:mccr8]
	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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 21 • 9 years ago

I don't know why MozReview => Try doesn't work for me atm.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 22 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adb0414c2149

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 25 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f3bd205f87

	David Teller [:Yoric] - still alive but not very active 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.

	Nathan Froyd [:froydnj]
	Comment 29 • 9 years ago

https://reviewboard.mozilla.org/r/22973/#review21155

	Nathan Froyd [:froydnj]
	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?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 31 • 9 years ago

Yeah, see bug 1219646.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 32 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f3bd205f87

	Carsten Book [:Tomcat]
	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 ?

	David Teller [:Yoric] - still alive but not very active 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.

	David Teller [:Yoric] - still alive but not very active 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

	David Teller [:Yoric] - still alive but not very active 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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 38 • 9 years ago

Rebased.

	Pulsebot
	Comment 41 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2255c56d2d00

	Carsten Book [:Tomcat]
	Comment 43 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2255c56d2d00

	Carsten Book [:Tomcat]
	Comment 44 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2255c56d2d00

	Carsten Book [:Tomcat]
	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