1194627 - Fix intermittent LSan-detected leak, bug 1166041

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Filing a separate bug because bug 1166041 is pretty hard to read with all the Treeherder comments.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Track allocations

This patch tracks allocation and deallocation of slots and other object buffers. It fails very intermittently on Try, I think/hope it's the same issue as the one LSan detects. Would be great if we could get a reliable shell testcase.

I'm interested in any assertion failures that include: allocatedObjectBuffers_

Fuzzers might find false positives (problems with this patch) though.

Should apply to m-c revision 4e883591bb5d

Comment 2

[Jan de Mooij [:jandem]]

Phew, after many Try builds and staring at log files I finally understand the problem. Here's what happens:

(1) We have a JSOP_NEWOBJECT (object literal) where we allocate ~20 objects with external/dyamic slots. These are put in the ObjectGroup's PreliminaryObjectArray.

(2) One of these objects dies, is swept and removed from the PreliminaryObjectArray.

(3) We allocate another object that takes its place in the PreliminaryObjectArray.

(4) We give the group an unboxed layout. This makes all objects in the PreliminaryObjectArray UnboxedPlainObjects.

(5) Now the object swept in (2) is finalized in the background, however the isNative() check incorrectly returns false for this object because we changed the group's clasp while giving it an unboxed layout. This means we don't free its slots/elements.

So this is basically a race: the main thread has to give the group an unboxed class before the object is finalized. When that happens, we don't free its slots/elements.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Testcase on top of allocation tracking patch

Here's a test that fails with --no-ggc and the allocation tracking patch I attached previously, on OS X 32-bit:

$ dist/bin/js --no-ggc test.js
Assertion failure: allocatedObjectBuffers_ == 0, at js/src/vm/Runtime.cpp:462

This is very fragile though; it probably won't fail on other platforms due to timing/threading/allocation differences.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8647946 [details] [diff] [review]
Track allocations

Cancelling feedback because I found the problem. Thanks!

Comment 5

[Jan de Mooij [:jandem]]

Brian, the simplest fix I can think of is to js_delete the slots/elements in PreliminaryObjectArray::sweep(). Does that sound okay to you? Is there a better way?

I'll see if this fixes it.

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Brian, the simplest fix I can think of is to js_delete the slots/elements in
> PreliminaryObjectArray::sweep(). Does that sound okay to you? Is there a
> better way?

Yes, that looks like the best way to handle this.  Thanks so much for tracking this down!

Comment 7

[Jan de Mooij [:jandem]]

Hm this fix is wrong for unboxed arrays though, JSObject::finalize will call the UnboxedArrayObject finalize hook on a NativeObject.

I wonder if it'd be simpler to have sweep() call setGroup with a stable PlainObject/ArrayObject ObjectGroup..

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Hm this fix is wrong for unboxed arrays though, JSObject::finalize will call
> the UnboxedArrayObject finalize hook on a NativeObject.
> 
> I wonder if it'd be simpler to have sweep() call setGroup with a stable
> PlainObject/ArrayObject ObjectGroup..

Hmm, good point.  I don't think you need a PlainObject or ArrayObject group specifically, but any group such that clasp->isNative() and the clasp won't change in the future (this would eliminate the race entirely too, which is nice).  I think the group pointer of the compartment's global should suffice, though I can't tell if it is guaranteed to be non-NULL when we hit PreliminaryObjectGroup::sweep.  If the global has been cleared though then the compartment is dead and the preliminary objects will never be analyzed anyways.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #8)
> I think the group pointer of the compartment's global should
> suffice, though I can't tell if it is guaranteed to be non-NULL when we hit
> PreliminaryObjectGroup::sweep.  If the global has been cleared though then
> the compartment is dead and the preliminary objects will never be analyzed
> anyways.

Makes sense. I was thinking of a global `static const` struct that has a Class* pointer at offset 0, but using the global's group might be nicer.

We have to make sure clasp->finalize is nullptr though for the global, but we can just assert that.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8647946 [details] [diff] [review]
Track allocations

In any case, this patch didn't throw up anything on 1 machine overnight. feedback+ for posterity.

Comment 11

[Jan de Mooij [:jandem]]

We also need to make sure PreliminaryObjectArray::sweep is always called before JSObject::finalize. Not sure if that's guaranteed with background finalization of objects, will look into this later.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch

The plan to use the global's group didn't work because the global has a finalize hook in the browser.

This patch uses the Object.prototype group. All green and it fixes the leak:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0c039534f7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

My hero!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca20318c3627

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta approval when you get a chance.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8648381 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1162199.
[User impact if declined]: Memory leaks.
[Describe test coverage new/current, TreeHerder]: Known to fix this leak on Treeherder.
[Risks and why]: Low risk. 
[String/UUID change made/needed]: None.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8648381 [details] [diff] [review]
Patch

Memleaks are bad, let's uplift to Aurora and Beta.

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0007d7e6e8ab

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/aaca68a75140