forked from bug 352064 the testcase in attachment 237646 [details] (zip file) in a 1.8.0 (but not 1.8.1) debug build with Java 6 from this morning. shows lots of JS engine warning: leaking GC root '&member_descriptor->invoke_func_obj' at 03EA3B34 warnings with a summary JS engine warning: 260 GC roots remain after destroying the JSRuntime. These roots may point to freed memory. Objects reachable through them have not been finalized.

I wonder if this is because the invoke_func_obj root is being added always, but only removed if the function ptr is non-null?

(In reply to comment #1) > I wonder if this is because the invoke_func_obj root is being added always, but > only removed if the function ptr is non-null? That would do it... Can you take this one? /be

Yeah. Only question is, should I NOT root if the function is null, or should I remove the root regardless? Which is the right direction to go?

Alternative #1

Keep whichever one of these you like better.

Wait, JS_GetFunctionObject will always return non-null after a successful JS_NewFunction return. So there's no point in the null-check guarding the call to JS_AddNamedRoot. Now the question is: why would the rooted pointer become null? The first patch is good in any event, and it may be a fix. But how does the pointer become null (if it does)? If it doesn't, then there's no actual bug biting here -- just the latent bug of the null-check guarding the JS_RemoveRoot call. /be

All of the memsets on the structure seem only to happen immediately following its allocation and I don't see any other obvious places where this pointer would be getting NULLed out....

If you are seeing JS engine warning: leaking GC root '&member_descriptor->invoke_func_obj' at 03EA3B34 then indeed the JS_RemoveRoots are not happening, or are passing the wrong address, being called too late, whatever. This needs debugging before patching. The first patch is a good change to the current code, but it won't fix the bug AFAICT. Please reproduce with it applied and debug why JS_RemoveRoot ain't working for these named roots. /be

Comment on attachment 260617 [details] [diff] [review] first proposal, remove root regardless This can't hurt, could help but we think it won't. Try it? /be

I can still see the same GC warning with the patch.

Well, I have tried now with 1.8, 1.8.0 and trunk nightly builds and cannot reproduce this bug on Mac OS X; it must be windows-only? I will try on my windows box later.

Forgot to mention, here is Firefox 1.8.0 debug build on Solaris Nevada.

jsj_class.c: 1.24 Landed the reviewed patch here, but not closing out the bug as it seems unlikely to fix the problem. Someone else who can reproduce this will have a better chance of fixing it than I did just auditing code, so I am releasing the bug into the wild.

Old bug with test case using Java Applet, so probably no longer actionable, therefore resolving as INCOMPLETE.