Nathan Froyd [:froydnj] Reporter
	Description • 6 years ago

LoadLoadableRootsTask::Dispatch creates a thread for itself to run on, but never shuts the thread down.  So we have this single-use thread hanging out in the parent process, consuming resources (mostly the memory for its stack) for no reason.

We should shut down the thread after LoadLoadableRootsTask has finished its work, or find a separate, non-transient thread to do this work on.

	Nathan Froyd [:froydnj] Reporter
	Comment 2 • 6 years ago
mozreview-review

Comment on attachment 8960276 [details]
bug 1439383 - clean up the load loadable roots thread when we're done with it

https://reviewboard.mozilla.org/r/229044/#review234816

Thank you!

	J.C. Jones [:jcj] (he/him)
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8960276 [details]
bug 1439383 - clean up the load loadable roots thread when we're done with it

https://reviewboard.mozilla.org/r/229044/#review234946

LGTM.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 4 • 6 years ago

Thanks for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac374b018d9154e1327827d9f7996ce46874725

	Pulsebot
	Comment 5 • 6 years ago

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5be07e86738e
clean up the load loadable roots thread when we're done with it r=froydnj,jcj

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 6 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5be07e86738e

	Pulsebot
	Comment 7 • 6 years ago

Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c23c7481957f
Backed out changeset 5be07e86738e for causing leaks (bug 1401883). a=backout

	Cosmin Sabou [:CosminS]
	Comment 8 • 6 years ago

Backed out for causing leaks. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1401883#c64

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171756681&repo=mozilla-inbound&lineNumber=4133

Backout link: https://hg.mozilla.org/mozilla-central/rev/c23c7481957f7b982cffc0ce1d25979c69ca2c2f

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 9 • 6 years ago

I tried some things, but I have no idea why shutting down a thread would cause leaks. :froydnj any ideas?

	Nathan Froyd [:froydnj] Reporter
	Comment 10 • 6 years ago

(In reply to David Keeler [:keeler] (use needinfo) from comment #9)
> I tried some things, but I have no idea why shutting down a thread would
> cause leaks. :froydnj any ideas?

I have no idea either, that seems bizarre.  The one thing that comes to mind is that with this patch, you're now sending an extra event through the event loop, and that does...something.  It changes some sort of timing window just enough that the conditions for the leak happen more consistently?

Unfortunately, the leaks only seem to happen on Windows, so we don't have rr available...maybe MS's time travel debugging could be used to capture this?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 12 • 6 years ago

Now that bug 1401883 has (hopefully?) been fixed, I think it's safe to re-land this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c3f8a32fbf4b4a0db55df2acd7e555da061bab&selectedJob=182414098

	Pulsebot
	Comment 13 • 6 years ago

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffd4b055fb8b
clean up the load loadable roots thread when we're done with it r=froydnj,jcj

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ffd4b055fb8b