1370786 - Permaorange ASan devtools LeakSanitizer | leak at lul::LUL::NotifyAfterMap, read_procmaps, SamplerThread::SamplerThread when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Phil Ringnalda (:philor)]

This is new bustage from something in the range https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c6289f56812&tochange=5801aa478de1 since I pushed a simulation of trunk as beta to try yesterday and didn't hit this, but now we have permaorange https://treeherder.mozilla.org/logviewer.html#?job_id=105058608&repo=try

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1

Comment 1

[Julien Cristau [:jcristau]]

This is a blocker for 55.0b1

Comment 2

[Brian Grinstead [:bgrins]]

Just FYI, the `TypeError: DebuggerServer.removeContentServerScript is not a function` error message that is directly above the LeakSanitizer error is being fixed in Bug 1370925. But I'm assuming it's not related to this particular issue.

Comment 3

[Tom Tromey :tromey]

One hypothesis for the leak is that AddSecMap can early-return without deleting the passed-in SecMap.
https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/tools/profiler/lul/LulMain.cpp#541-543
Ownership here would be clearer with UniquePtr.
I'm not sure how to test this though ... ?

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1370786 - use UniquePtr for SecMap in LUL;

Review commit: https://reviewboard.mozilla.org/r/146794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146794/

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8875369 [details]
Bug 1370786 - use UniquePtr for SecMap in LUL;

Could you give some feedback on this?
And maybe tell me, if you know, how to reproduce the failure here?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8875369 [details]
Bug 1370786 - use UniquePtr for SecMap in LUL;

This patch seems good; it might be a little better to store UniquePtr inside mSecMaps, but I understand that would be a more far-reaching change.

As for testing this, you can replicate philor's try-push by grabbing his patches, I guess?  If you ask nicely, philor might even simulate beta with your patch added on top.

Comment 7

[Tom Tromey :tromey]

From irc:

<philor> tromey: oh, I
	 see. https://hg.mozilla.org/try/rev/a3e416c66e714e2e63e2afb35529f6d698b60eb9
	 and
	 https://hg.mozilla.org/try/rev/e45e86a84f88c54475eb733a116c51a49010d1df
	 and try: -b o -p linux64-asan -u mochitest-dt -t none

Comment 8

[Tom Tromey :tromey]

Hopefully this is correct:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaa8bcb9f5a938801305c376f837e7bebd3f3eb5

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8875369 [details]
Bug 1370786 - use UniquePtr for SecMap in LUL;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146794/diff/1-2/

Comment 10

[Tom Tromey :tromey]

Here's a try run for the new patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a983f98e35ff8cd44f2628c29837fac7a53c14
This run included :philor's patches.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8875369 [details]
Bug 1370786 - use UniquePtr for SecMap in LUL;

https://reviewboard.mozilla.org/r/146794/#review151590

Above and beyond! \o/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf13425e15f7
use UniquePtr for SecMap in LUL; r=froydnj

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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