912255 - Ghost window on http://n-e-r-v-o-u-s.com/cellCycle/

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Guilherme Lima]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130903030201

Steps to reproduce:

Open http://n-e-r-v-o-u-s.com/cellCycle/ and then close it.


Actual results:

Opened about:memory and there was a 
top(none)/ghost/window(http://n-e-r-v-o-u-s.com/cellCycle/)
Then opened about:compartments and there it was under "Ghost windows"


Expected results:

No Ghost Windows.

Comment 1

[Nicholas Nethercote [inactive]]

I can reproduce this.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This leaks through shutdown.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Interesting, if I minimize memory a couple times on ESR 17 it goes away eventually.  It seems to stick around forever on nightly.

Alice, do you have time to bisect this?

Comment 4

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/8e3a124c9c1a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130627 Firefox/25.0 ID:20130627193322
Bad:
http://hg.mozilla.org/mozilla-central/rev/ec8ceaf4b03a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130628 Firefox/25.0 ID:20130628063224
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e3a124c9c1a&tochange=ec8ceaf4b03a

Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9bbec788caf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130627 Firefox/25.0 ID:20130627132145
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/88b65229c78c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130627 Firefox/25.0 ID:20130627140921
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9bbec788caf&tochange=88b65229c78c

Regressed by: bug 738869

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Indeed, that bug changed cycle collection of WebGLContext.
mDefaultVertexArray for instance isn't traversed/unlinked.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks Alice, you rock.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I should have a patch... once my build is ready.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: traverse/unlink mDefaultVertexArray

Indeed, this patch added mBoundVertexArray and mDefaultVertexArray but only traversed/unlinked the former.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 799470 [details] [diff] [review]
traverse/unlink mDefaultVertexArray

Assuming you actually tested this helps :)

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Sorry, didn't realize that you already had a patch.

Tested locally, the patch does fix the leak for me.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36aacf27a24e

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/36aacf27a24e

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should add a mochitest for this.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Yes, but in fact, I don't understand why this wasn't caught already. This kind of leak is automatically detected when it happens on a mochitest, right?

Looking at the code, the cycle between mDefaultVertexArray and its WebGLContext is always, unconditionally created right on the creation of any WebGL context. Furthermore, even if for whatever reason the leak only occured when a non-default vertex array was bound, that too is tested by an existing, already-running mochitest, namely this WebGL conformance test page (run by the WebGL mochitest): conformance/extensions/oes-vertex-array-object.html

So I'm a bit clueless as to why the leak wasn't automatically detected; understanding that blocks doing anything about that; but I don't have time to do anything about it short-term myself (b2g 1.2 rush...).

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Benoit Jacob [:bjacob] from comment #14)
> Yes, but in fact, I don't understand why this wasn't caught already. This
> kind of leak is automatically detected when it happens on a mochitest, right?

Yes.  Since we leak through shutdown here, triggering it is enough to turn the tree orange.

> Looking at the code, the cycle between mDefaultVertexArray and its
> WebGLContext is always, unconditionally created right on the creation of any
> WebGL context.

Why do you believe that?  Where is the edge from the vertex array to the context?  I don't see it anywhere.  WebGLBoundObject holds a weak reference to the context afaict.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Oh, you are completely right. Been a long time since I actually did much WebGL coding :-) we do intentionally not have a strong reference from the WebGLContextBoundObject back to the WebGLContext, to avoid creating unnecessary cycles. So in fact, a mochitest would be as simple as creating a vertex array object, and in javascript manually creating a reference from it to the webgl context.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

This would actually be perfectly reasonable material to add to the existing, upstream WebGL conformance test,

  https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/oes-vertex-array-object.html

so we should just add it there (and then, we'd pick it up the next time that we update our copy of the conformance tests into our mochitest, which should happen more frequently than transits of Halley's comet).

Adding webgl-test-needed, which is how we remember to make needed contribution to the conformance tests...

Comment 18

[Nicholas Nethercote [inactive]]

> Indeed, this patch added mBoundVertexArray and mDefaultVertexArray but only
> traversed/unlinked the former.

Is there any way to automate the detection of these kinds of problems?

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #16)
> So in fact, a mochitest would be as simple as creating a
> vertex array object, and in javascript manually creating a reference from it
> to the webgl context.

On second thought this is probably more subtle than that? The cycle that we forgot to break was between the WebGL context and its default vertex array, which AFAICT is not exposed to JS... so I don't understand how the present testcase managed to create that cycle. Need to minimize it?

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> > Indeed, this patch added mBoundVertexArray and mDefaultVertexArray but only
> > traversed/unlinked the former.
> 
> Is there any way to automate the detection of these kinds of problems?

The 'refgraph' tool that I was working on in my spare time ( https://github.com/bjacob/mozilla-central/wiki/Refgraph ) does exactly that, but with all the b2g work lately, I haven't had time.

I've been thinking about how to move refgraph to "bjacob's side project on github" to something that could land in mozilla-central. I think there's a reasonable path to that that involves abandoning refgraph's custom malloc-replace library and instead porting it to use DMD's existing one, which is already in mozilla-central (maybe extending it a little bit along the way).

refgraph would still remain a non-default build option, like DMD is, but it could be useful to a lot more people in this way.

I'd like to convince people who have the ability to spend mozilla time on memory issues that this is a reasonable project :-) Justin and Andrew had expressed interest a while ago but Justin left, most unfortunately.

Comment 21

[Nicholas Nethercote [inactive]]

BTW, this was a great bug all round.  Guilherme provided a nice, reproducible test case, which took advantage of some good built-in tools.  I confirmed it reproduced, Kyle identified it was a regression, Alice bisected and found the regression, and Olli and Benoit fixed it.  All in just over 24 hours! :)

Comment 22

[Andrew McCreight [:mccr8]]

Well, the refgraph tool is great for figuring out why a cycle is occurring, but it doesn't help with the issue here, which is that we are lacking a test case to create a cycle.  Practically speaking, for these sorts of issues in DOM code we mostly depend on Jesse fuzzing up test cases that leak on shutdown, and because they are often weird looking leaks, they don't always get fixed quickly.

I spent some time on a static analysis to detect things missing from Traverse and Unlink, but it got bogged down in the ugly reality of Gecko code, and didn't really end up as a usable thing.  Plus the underlying static analysis framework is now defunct.  But the basic idea is that you take a list of fields in the object, filter out things that the CC should be told about, and then compare it to the list of things you do tell the CC about.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Andrew McCreight [:mccr8] from comment #22)
> Well, the refgraph tool is great for figuring out why a cycle is occurring,
> but it doesn't help with the issue here, which is that we are lacking a test
> case to create a cycle.

If fact, Refgraph could automatically detect the underlying problem --- that a non-cycle-collected C++ strong reference exists between objects that are exposed to JS --- without even the need for actually creating the cycle.

Refgraph knows the full graph of strong references, and know what part of it is known to the CC. Refgraph can also know which vertices (i.e., objects) are exposed to JS. So it could automatically find out when there is a path of strong references from one JS-exposed object to another, that is not known to the CC. That was my next task when working on refgraph, but I had to stop working on it for lack of time. But if anyone is interested, it could be implemented very quickly at this point.

Comment 24

[Andrew McCreight [:mccr8]]

Oh, I see.  That's pretty cool.  Yeah, that would certainly catch the most common problems with undeclared stuff.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

(Of course you have to create the one-way strong reference between C++ objects. But we already do that in the above-mentioned mochitest).

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 799470 [details] [diff] [review]
traverse/unlink mDefaultVertexArray

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 738869
User impact if declined: large memory leak on certain WebGL-using pages
Testing completed (on m-c, etc.): m-c for a few days now
Risk to taking this patch (and alternatives if risky): no risk at all
String or IDL/UUID changes made by this patch: none

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8cf8c00b210

Comment 28

[Manuela Muntean [Away]]

Verified as fixed with Firefox 25 beta 7 (build ID: 20131010180222) and latest Aurora (build ID: 20131011004001).