Closed Bug 437325 Opened 13 years ago Closed 12 years ago

gc & multiple rt confusion

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: igor)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 20 obsolete files)

26.36 KB, patch
Details | Diff | Splinter Review
v18
61.03 KB, patch
brendan
: review+
Details | Diff | Splinter Review
I tried hacking in multiple-runtime stuff into worker threads, given that they already don't share any state.  I'm running into this assertion, though:

 Assertion failure: requestDebit <= rt->requestCount, at ../../../mozilla/js/src/jsgc.cpp:3081

What's happening is this.. there are two threads, t0 (main thread) and t1 (worker thread).  Two runtimes, r0 (main script runtime), r1 (newly created rt for worker).  A context c1 for the worker, and a context c0 which is whatever context script is executing in on the page.

Initially, r1 and c1 are created on t0.  ClearContextThread is called on c1 at the end.  Then, operations are proxied over to t1, where SetCT/BeginReq/EndReq/ClearCT are called around the JS calls.  However, when it comes time to destroy the worker context/runtime, that happens on t0 (it doesn't need to, that's just the way the code's sset up right now, and it's a bit cleaner this way).

The problem is that, at that point, there are two contexts on t0 -- both c0 and c1.  c0 has an active request at the time, because it's coming in from content script.  The code at http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#3061 runs and counts the outstanding request count of every context on the thread, regardless of the context's runtime, and then at http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#3081 asserts based on the current runtime for which gc is actually executing.

It seems like gc should only be counting outstanding requests for contexts that belong to the same runtime here.
I should add, for extra fun: JS_DestroyContext/JS_DestroyRuntime end up being called from inside a GC on the main ScriptRuntime (via xpconnect's GCCallback).

I added a check for acx->runtime == rt in the requestDebit computation, which got me farther; now I'm crashing in JS_DestroyRuntime->js_FinishAtomState->..->js_string_uninterner->js_FinalizeStringRT, where malloc is telling me I'm freeing a non-aligned pointer.
Hmmm.  It looks like JSThread doesn't know which JSRuntime supplied the GCThings in its per-thread freelists.

I don't see what prevents js_NewGCThing from returning memory allocated from rt0 when called during the setup of rt1/cx1.  And the reverse--during the setup of rt1/cx1, if js_NewGCThing refills t0's gcFreeLists, it will fill them with memory from rt1, which may later be handed out to cx0.
Attached patch bug-437325-assertion.patch (obsolete) — Splinter Review
Vlad, this patch adds an assertion for the condition I described in comment 2.

JS_ASSERT won't hit in a release browser build, but you can tweak it to "if (!(...)) abort()" or whatever.
The patch from bug 378918 should address the issues with multiple runtimes and AFAICS this bug is the dup of that bug. For now I am just making this bug to depend on the bug 378918.
Depends on: 378918
(In reply to comment #4)
> AFAICS this bug is the dup of that bug.

I was wrong about it: this and bug 378918 are independent issues even if they both have roots is JSThread sharing between runtimes.

No longer depends on: 378918
Attached file bundle (obsolete) —
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #350297 - Flags: review?(igor)
(In reply to comment #7)
> Created an attachment (id=350297) [details]
> only count this runtime's requests

This is fine as a fix, but that means that any code that does such looping will have to deal with foreign runtime. At this point I suggest to start creating JSThread instances one per runtime and unless this shows up in benchmarks, take that approach.
Attached patch runtime-private JSThread (obsolete) — Splinter Review
Here is the implementation of that idea of not sharing JSThread instance among JS runtimes. For that the patch uses a hashtable in JSRuntime to store JSThread instances removing the need to use thread-private data and avoiding any leaks past JS_DestroyRuntime.

An extension to the patch can make unnecessary JS_SetContextThread and JS_ClearContextThread API as the JSThread can be allocated in JS_BegginRequest as necessary. But since JS_BeginRequest cannot fail, it would be necessary to preallocate JSThread instances complicating the code. So I left that for another bug.

The main drawback of the patch is that JSThread instances for destructed threads will stay alive until the next GC. This is not an issue for the browser even in presence of dom-workers, but if for some embedding that would be problematic, an explicit API can be provided to tell the engine that a particular native thread would no longer execute the JS for the given runtime.

I will test this for performance regressions before asking for a review.
Assignee: timeless → igor
Depends on: 477627
Comment on attachment 350297 [details] [diff] [review]
only count this runtime's requests

I incorporated this fix into the patch for the bug 477627.
Attachment #350297 - Flags: review?(igor)
I am making this bug to block the bug 477627 as with JSThread sharing among different runtimes the fix for that bug is more complex than necessary.

Here is for references the problem with shared JSThread: js_GC accesses JSThread structure for non-current threads to cleanup various caches there. But since at the same time another runtime can accesses the same caches, this inherently racy.
Blocks: 477627
No longer depends on: 477627
Attached patch runtime-private JSThread v2 (obsolete) — Splinter Review
The patch makes JSThread runtime-private and stores JSThread* in a JSRuntime hashtable. JSThread instances are initialized as necessary on JSContext creation and the GC destroys them when it detects that JSThread has no JSContext instances associated with it.
Attachment #323880 - Attachment is obsolete: true
Attachment #350280 - Attachment is obsolete: true
Attachment #350297 - Attachment is obsolete: true
Attachment #350973 - Attachment is obsolete: true
Attachment #363541 - Flags: review?(brendan)
To Mike Moening:

Do you have a chance to test the patch from the comment 12 in your embedding? Heavy multi-threaded tests are helpful here even if you use just one JSRuntime.
I'll try to test this today or tommorrow.
Attached patch runtime-private JSThread v3 (obsolete) — Splinter Review
The new version restore accidentally removed asserts in JS_(Begin|End)Request() and removes no longer necessary code in xpconnect to ensure thread destructor ordering.
Attachment #363541 - Attachment is obsolete: true
Attachment #363670 - Flags: review?(brendan)
Attachment #363541 - Flags: review?(brendan)
Blocks: 479934
I will review this tomorrow a.m. -- looking for the good word from MikeM too.

/be
One nit: (jsid) is used as type of thread->id in several places (implicitly in the JSDHashTableOps.matchEntry hook) but the type is jsword.

/be
Alright then...time to try out your patch Igor. Apparently bug# 479934 will be fixed by this same patch. Now you have my full attention!

Anybody know how to apply a patch using TortoiseHg? This ones is too big to apply by hand.
Igor,

I installed your patch and built the monkey.  The crash in bug# 479934 has disappeared. So that's good.

However, a new crash has now appeared in jstracer.cpp js_RecordTree() on line 3275 shown below:

ti->typeMap.captureTypes(cx, *globalSlots, 0/*callDepth*/);
ti->nStackTypes = ti->typeMap.length() - globalSlots->length();

The crash is because globalSlots is set to NULL.  When slots->length() is encountered inside captureTypes()....well you know.  Next line will crash it too.

Let me know if you need anything to debug it.
The crash is consistent.

Simple NULL pointer check?

Mike M.
(In reply to comment #19)
> I installed your patch and built the monkey.  The crash in bug# 479934 has
> disappeared. So that's good.

Oh, so you do use multiple runtimes, right?

> 
> However, a new crash has now appeared in jstracer.cpp js_RecordTree() on line
> 3275 shown below:

Could you attach the full stack trace or at least the stack trace until js_Interpret?

> The crash is consistent.

Could you check if js_GC is called before the crash?

> 
> Simple NULL pointer check?

No - globalSlots should not be null at that point.
I wonder if that issue with globalSlots can be related to the fact that Oracle is a singleton that is shared between different JSRuntime instances. The comments says that:

 -- The entire VM shares one oracle. Collisions and concurrent updates are tolerated and worst case cause performance regressions.

Does that also cover the case of concurrent invocations of js_FlushJITOracle? It is not possible with single JSRuntime per process as the function is only called during the GC. But with multiple JSRuntime instances it can happen.
I CC Gal to have his input on the comment 21.
> Oh, so you do use multiple runtimes, right?
Yes sir, I do.

> Could you check if js_GC is called before the crash?
Yes I'm pretty sure it was.

With more testing now I'm seeing another problem put up.
I can't say if they are related but here's the 2nd:

Assertion failure: RECURSION_LEVEL(table) > 0, at .\jsdhash.cpp:697

Call stack:
JS_Assert(const char * s=0x0096e1b8, const char * file=0x0096e1a8, int ln=697)  Line 58	C++
JS_DHashTableOperate(JSDHashTable * table=0x04b62298, const void * key=0x066514dc, JSDHashOperator op=JS_DHASH_LOOKUP)  Line 697 + 0x6c bytes	C++
js_LookupLocal(JSContext * cx=0x057e3ba0, JSFunction * fun=0x06652f18, JSAtom * atom=0x066514dc, unsigned int * indexp=0x06fac508)  Line 2535 + 0x10 bytes	C++
call_resolve(JSContext * cx=0x057e3ba0, JSObject * obj=0x06631c40, int idval=107287772, unsigned int flags=4, JSObject * * objp=0x06fac57c)  Line 888 + 0x15 bytes	C++
js_LookupPropertyWithFlags(JSContext * cx=0x057e3ba0, JSObject * obj=0x06631c40, int id=107287772, unsigned int flags=4, JSObject * * objp=0x06fac5c0, JSProperty * * propp=0x06fac5ac)  Line 3632 + 0x17 bytes	C++
js_FindPropertyHelper(JSContext * cx=0x057e3ba0, int id=107287772, JSObject * * objp=0x06facbb8, JSObject * * pobjp=0x06facc48, JSProperty * * propp=0x06facba4, JSPropCacheEntry * * entryp=0x06fac9c4)  Line 3749 + 0x23 bytes	C++
js_Interpret(JSContext * cx=0x057e3ba0)  Line 5133 + 0x26 bytes	C++
js_Invoke(JSContext * cx=0x057e3ba0, unsigned int argc=4, int * vp=0x057c425c, unsigned int flags=0)  Line 1333 + 0x9 bytes	C++
find_replen(JSContext * cx=0x057e3ba0, ReplaceData * rdata=0x06facf40, unsigned int * sizep=0x06facea8)  Line 1643 + 0x13 bytes	C++
replace_glob(JSContext * cx=0x057e3ba0, long count=0, GlobData * data=0x06facf40)  Line 1739 + 0x11 bytes	C++
match_or_replace(JSContext * cx=0x057e3ba0, int (JSContext *, long, GlobData *)* glob=0x007f7e40, void (JSContext *, GlobData *)* destroy=0x007f7e10, GlobData * data=0x06facf40, unsigned int argc=2, int * vp=0x057c423c)  Line 1333 + 0xf bytes	C++
js_StringReplaceHelper(JSContext * cx=0x057e3ba0, unsigned int argc=2, JSObject * lambda=0x06631c60, JSString * repstr=0x00000000, int * vp=0x057c423c)  Line 1851 + 0x1f bytes	C++
str_replace(JSContext * cx=0x057e3ba0, unsigned int argc=2, int * vp=0x057c423c)  Line 1775 + 0x19 bytes	C++
js_Interpret(JSContext * cx=0x057e3ba0)  Line 5006 + 0x17 bytes	C++
js_Execute(JSContext * cx=0x057e3ba0, JSObject * chain=0x066318a0, JSScript * script=0x04b5b1c0, JSStackFrame * down=0x00000000, unsigned int flags=0, int * result=0x06fae294)  Line 1564 + 0x9 bytes	C++
JS_ExecuteScript(JSContext * cx=0x057e3ba0, JSObject * obj=0x066318a0, JSScript * script=0x04b5b1c0, int * rval=0x06fae294)  Line 5140 + 0x19 bytes	C++

Let me know if this is worthy of another bug.

I'll try to get you a call stack on the slots NULL crash too.
Igor:

Right now I'm alternating between the two crashes.
Here is the call stack for the NULL globalslots crash:
----------------------

Queue<unsigned short>::length()  Line 123 + 0x3 bytes	C++
TypeMap::captureTypes(JSContext * cx=0x056daa48, Queue<unsigned short> & slots={...}, unsigned int callDepth=0)  Line 1108 + 0x8 bytes	C++
js_RecordTree(JSContext * cx=0x056daa48, JSTraceMonitor * tm=0x06129064, nanojit::Fragment * f=0x0614c5d0, nanojit::Fragment * outer=0x00000000, unsigned long globalShape=4294967295, Queue<unsigned short> * globalSlots=0x00000000)  Line 3276	C++
js_MonitorLoopEdge(JSContext * cx=0x056daa48, unsigned int & inlineCallCount=1)  Line 4209 + 0x1e bytes	C++
js_Interpret(JSContext * cx=0x056daa48)  Line 3712 + 0x530 bytes	C++
js_Execute(JSContext * cx=0x056daa48, JSObject * chain=0x06662e20, JSScript * script=0x04b68198, JSStackFrame * down=0x00000000, unsigned int flags=0, int * result=0x06f1e294)  Line 1564 + 0x9 bytes	C++
JS_ExecuteScript(JSContext * cx=0x056daa48, JSObject * obj=0x06662e20, JSScript * script=0x04b68198, int * rval=0x06f1e294)  Line 5140 + 0x19 bytes	C++

The intrpreter is sitting on 
RELATIONAL_OP(<);

Let me know if you want the text of the script I was running.  I can isolate it prettty well.
I'm seeing this debug info in the console BTW:
-----------------------------------------------------
recorder: started(1537), aborted(1264), completed(1338), different header(0), trees trashed(0), slot promoted(0), unstale loop variable(0), breaks(0), returns(0), unstableInnerCalls(0)
monitor: triggered(10091), exits(10071), type mismatch(0), global mismatch(13)
To Mike:  could you check if you would see the assert in RECURSION_LEVEL(table) if you disable the jit?
With JIT off I get zero asserts and no crashes (at least with a moderate amount of beating) Please don't take my JIT away... :-)
It seems that the RECURSION_LEVEL(table) assert is bogus. The double-hashing implementation contains debug-only recursion level that is incremented and decremented during hashing operations to assert that the hashing callbacks do not re-enter. But this increment/decrement code is not thread-safe which may lead to assert fairing when a read-only table is shared between multiple threads.
So I suppose either take it out or use an atomic increment/decrement?
Attached patch runtime-private JSThread v4 (obsolete) — Splinter Review
The new version should fix the RECURSION_LEVEL assert with the help of JS_DHashMarkTableImmutable. It also fixes jsid/jsword usage for thread ids. But I still have no clue about the jit crash.
Attachment #363670 - Attachment is obsolete: true
Attachment #363942 - Flags: review?(brendan)
Attachment #363670 - Flags: review?(brendan)
> I still have no clue about the jit crash.
Then who does?
Igor,
I had an old API pair of 

JS_ClearContextThread(cx); 
Do something on other js runtime now...
JS_SetContextThread(cx);

Which I still had in the code to safely handle the transition between different runtimes by the same thread.
Would this have an impact on this bug?

I took them out now.
Apparently that had no impact. 
The NULL pointer crash still exists.
(In reply to comment #32)
> JS_ClearContextThread(cx); 
> Do something on other js runtime now...
> JS_SetContextThread(cx);

With the patch those JS_(Clear|Set)ContextThread calls are no longer necessary.

(In reply to comment #33)
> Apparently that had no impact. 
> The NULL pointer crash still exists.

But did the newer patch fixed the RECURSION_LEVEL assert?
Yes I think so.  Longer term testing will prove that 100%.
The crash seems a little harder to trigger too.  But its still there.

So what do those JS_(Clear|Set)ContextThread calls actually do?
NoOp?
This patch makes trace oracle a field in JSRuntime instead of a global variable. 

To Mike: could you apply this patch on top of the patch from the comment 30 to see if it would address the globalSlots segfault?
(In reply to comment #35)
> So what do those JS_(Clear|Set)ContextThread calls actually do?
> NoOp?

The patch in this bug does not change the purpose of the functions to break/restore the link between JSThread/JSContext. An application must still call the functions when moving JSContext instances between threads.
CCing danderson. The JIT crash might be related to a bug he is working on. David what do you think?
Gal, what is the number for the other bug?
Mike Moening wrote to comment #35:
> The crash seems a little harder to trigger too.  But its still there.

Have you observed this crash *without* patches in this bug? I.e. I wonder if this is really a regression in the patch from the comment 30.
Igor,

I totally missed that "extra patch" with all the bug mail flying around. I'll apply it now and let you know how it works.

Mike M.
The change to make trace oracle a field in JSRuntime instead of a global
variable had no impact.
I'm convinced this bug is NOT related to having multiple runtimes.
That oracle change still might be a good design desicion in the long run. Especially if it reduces contention (not sure if it does this).
I'll leave that up to you.

I believe these patches are solid and do more good to increase stability than not. Multi-thread with Multi-RT land is WAY better with your patches than without.

The NULL crash seems to be more of a multi-threaded bug than anything.
I'm going to try to reproduce this with a shell test case (not sure if I can).
Attached patch runtime-private JSThread v5 (obsolete) — Splinter Review
The new version of the patch syncs the changes with TM tip and makes js_InitContextThread to take the GC lock and call js_waitForGC itself to share more code.
Attachment #363942 - Attachment is obsolete: true
Attachment #364180 - Flags: review?(brendan)
Attachment #363942 - Flags: review?(brendan)
To Mike Moening: thanks for the detailed memory leak report! It clearly showed the problem with the patch.
Attached patch runtime-private JSThread v6 (obsolete) — Splinter Review
The new patch fixes the memory leak in the previous version: there I forgot to call js_DestroyScriptsToGC before destroying the JSThread instance from the GC.
Attachment #364180 - Attachment is obsolete: true
Attachment #364298 - Flags: review?(brendan)
Attachment #364180 - Flags: review?(brendan)
Comment on attachment 364298 [details] [diff] [review]
runtime-private JSThread v6

The patch uses a wrong assert.
Attachment #364298 - Attachment is obsolete: true
Attachment #364298 - Flags: review?(brendan)
Attached patch runtime-private JSThread v7 (obsolete) — Splinter Review
The new version of patch avoids unnecessary zeroing of the property cache when destructing JSThread. It also fixes a bug in the previous version which would issue ASSERT_CACHE_IS_EMPTY on just allocated JSThread with JSPropertyCache.empty set to false due to zeroing in calloc.
Attachment #364303 - Flags: review?(brendan)
Some of the leaks are gone.  There are 3 that still remain (it may be 2 since 1 looks to be a child of another)
Here are the callstacks where the allocations take place:


PropTreeKidsChunk : 48 : bytes at 0f587af8 : [js\src\jsscope.cpp Line 527]
  Allocation location
  ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms) Request ID: 00261634
   0x005a3df9  NewPropTreeKidsChunk : [js\src\jsscope.cpp Line 527]
    522  : static PropTreeKidsChunk *
    523  : NewPropTreeKidsChunk(JSRuntime *rt)
    524  : {
    525  :     PropTreeKidsChunk *chunk;
    526  : 
    527  :     chunk = (PropTreeKidsChunk *) calloc(1, sizeof *chunk);
    528  :     if (!chunk)
    529  :         return NULL;
    530  :     JS_ASSERT(((jsuword)chunk & CHUNKY_KIDS_TAG) == 0);
    531  :     JS_RUNTIME_METER(rt, propTreeKidsChunks);
    532  :     return chunk;
   0x005a3da4  InsertPropertyTreeChild : [js\src\jsscope.cpp Line 662]
   0x005a382f  GetPropertyTreeChild : [js\src\jsscope.cpp Line 912]
   0x005a2fe5  js_AddScopeProperty : [js\src\jsscope.cpp Line 1271]
   0x004fd851  js_DefineNativeProperty : [js\src\jsobj.cpp Line 3518]
   0x004de312  DefinePropertyById : [js\src\jsapi.cpp Line 3172]
   0x004de29a  DefineProperty : [js\src\jsapi.cpp Line 3197]
   0x004de4c8  JS_DefineProperties : [js\src\jsapi.cpp Line 3274]
   0x004dd610  JS_InitClass : [js\src\jsapi.cpp Line 2867]
   0x0054523a  js_InitArrayClass : [js\src\jsarray.cpp Line 3204]
   0x004dadc7  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1386]
    
 JSScopeProperty* : 64 : bytes at 0f587360 : [js\src\jsscope.cpp Line 137]
  Allocation location
  ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms) Request ID: 00261609
   0x005a327b  CreateScopeTable : [js\src\jsscope.cpp Line 137]
    132  :         JS_ASSERT(scope->hashShift == JS_DHASH_BITS - MIN_SCOPE_SIZE_LOG2);
    133  :         sizeLog2 = MIN_SCOPE_SIZE_LOG2;
    134  :     }
    135  : 
    136  :     scope->table = (JSScopeProperty **)
    137  :         calloc(JS_BIT(sizeLog2), sizeof(JSScopeProperty *));
    138  :     if (!scope->table) {
    139  :         if (report)
    140  :             JS_ReportOutOfMemory(cx);
    141  :         return JS_FALSE;
    142  :     }
   0x005a30e8  js_AddScopeProperty : [js\src\jsscope.cpp Line 1304]
   0x004fd851  js_DefineNativeProperty : [js\src\jsobj.cpp Line 3518]
   0x004fd579  js_DefineProperty : [js\src\jsobj.cpp Line 3402]
   0x004f2765  js_DefineFunction : [js\src\jsfun.cpp Line 2170]
   0x004e2072  JS_DefineFunction : [js\src\jsapi.cpp Line 4710]
   0x004e1d09  JS_DefineFunctions : [js\src\jsapi.cpp Line 4692]
   0x004dd62e  JS_InitClass : [js\src\jsapi.cpp Line 2867]
   0x004f16fa  js_InitFunctionClass : [js\src\jsfun.cpp Line 2047]
   0x004daad8  js_InitFunctionAndObjectClasses : [js\src\jsapi.cpp Line 1296]
   0x004dadac  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1359]
    
 [jsapi.cpp:1864] : 56 : bytes at 0f587200 : [js\src\jsapi.cpp Line 1864]
  Allocation location
  ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms) Request ID: 00261604
   0x004dbaa5  JS_malloc : [js\src\jsapi.cpp Line 1864]
   0x005a23aa  js_NewScope : [js\src\jsscope.cpp Line 159]
    154  : js_NewScope(JSContext *cx, jsrefcount nrefs, JSObjectOps *ops, JSClass *clasp,
    155  :             JSObject *obj)
    156  : {
    157  :     JSScope *scope;
    158  : 
    159  :     scope = (JSScope *) JS_malloc(cx, sizeof(JSScope));
    160  :     if (!scope)
    161  :         return NULL;
    162  : 
    163  :     js_InitObjectMap(&scope->map, nrefs, ops, clasp);
    164  :     scope->object = obj;
   0x004fb9c7  js_NewObjectMap : [js\src\jsobj.cpp Line 2603]
   0x004fc2f9  js_NewObjectWithGivenProto : [js\src\jsobj.cpp Line 2915]
   0x004fbfbe  js_NewObject : [js\src\jsobj.cpp Line 2810]
   0x004dd360  JS_InitClass : [js\src\jsapi.cpp Line 2790]
   0x004f16fa  js_InitFunctionClass : [js\src\jsfun.cpp Line 2047]
   0x004daad8  js_InitFunctionAndObjectClasses : [js\src\jsapi.cpp Line 1296]
   0x004dadac  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1359]

Let me know your thoughts.
Mike Moening wrote in comment 48:

> Some of the leaks are gone.  There are 3 that still remain (it may be 2 since 1
> looks to be a child of another)

If you disable the jit, will the leaks still persists? Also do you know if the leaks happens without the patch?





> Here are the callstacks where the allocations take place:
> 
> 
> PropTreeKidsChunk : 48 : bytes at 0f587af8 : [js\src\jsscope.cpp Line 527]
>   Allocation location
>   ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms)
> Request ID: 00261634
>    0x005a3df9  NewPropTreeKidsChunk : [js\src\jsscope.cpp Line 527]
>     522  : static PropTreeKidsChunk *
>     523  : NewPropTreeKidsChunk(JSRuntime *rt)
>     524  : {
>     525  :     PropTreeKidsChunk *chunk;
>     526  : 
>     527  :     chunk = (PropTreeKidsChunk *) calloc(1, sizeof *chunk);
>     528  :     if (!chunk)
>     529  :         return NULL;
>     530  :     JS_ASSERT(((jsuword)chunk & CHUNKY_KIDS_TAG) == 0);
>     531  :     JS_RUNTIME_METER(rt, propTreeKidsChunks);
>     532  :     return chunk;
>    0x005a3da4  InsertPropertyTreeChild : [js\src\jsscope.cpp Line 662]
>    0x005a382f  GetPropertyTreeChild : [js\src\jsscope.cpp Line 912]
>    0x005a2fe5  js_AddScopeProperty : [js\src\jsscope.cpp Line 1271]
>    0x004fd851  js_DefineNativeProperty : [js\src\jsobj.cpp Line 3518]
>    0x004de312  DefinePropertyById : [js\src\jsapi.cpp Line 3172]
>    0x004de29a  DefineProperty : [js\src\jsapi.cpp Line 3197]
>    0x004de4c8  JS_DefineProperties : [js\src\jsapi.cpp Line 3274]
>    0x004dd610  JS_InitClass : [js\src\jsapi.cpp Line 2867]
>    0x0054523a  js_InitArrayClass : [js\src\jsarray.cpp Line 3204]
>    0x004dadc7  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1386]
> 
>  JSScopeProperty* : 64 : bytes at 0f587360 : [js\src\jsscope.cpp Line 137]
>   Allocation location
>   ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms)
> Request ID: 00261609
>    0x005a327b  CreateScopeTable : [js\src\jsscope.cpp Line 137]
>     132  :         JS_ASSERT(scope->hashShift == JS_DHASH_BITS -
> MIN_SCOPE_SIZE_LOG2);
>     133  :         sizeLog2 = MIN_SCOPE_SIZE_LOG2;
>     134  :     }
>     135  : 
>     136  :     scope->table = (JSScopeProperty **)
>     137  :         calloc(JS_BIT(sizeLog2), sizeof(JSScopeProperty *));
>     138  :     if (!scope->table) {
>     139  :         if (report)
>     140  :             JS_ReportOutOfMemory(cx);
>     141  :         return JS_FALSE;
>     142  :     }
>    0x005a30e8  js_AddScopeProperty : [js\src\jsscope.cpp Line 1304]
>    0x004fd851  js_DefineNativeProperty : [js\src\jsobj.cpp Line 3518]
>    0x004fd579  js_DefineProperty : [js\src\jsobj.cpp Line 3402]
>    0x004f2765  js_DefineFunction : [js\src\jsfun.cpp Line 2170]
>    0x004e2072  JS_DefineFunction : [js\src\jsapi.cpp Line 4710]
>    0x004e1d09  JS_DefineFunctions : [js\src\jsapi.cpp Line 4692]
>    0x004dd62e  JS_InitClass : [js\src\jsapi.cpp Line 2867]
>    0x004f16fa  js_InitFunctionClass : [js\src\jsfun.cpp Line 2047]
>    0x004daad8  js_InitFunctionAndObjectClasses : [js\src\jsapi.cpp Line 1296]
>    0x004dadac  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1359]
> 
>  [jsapi.cpp:1864] : 56 : bytes at 0f587200 : [js\src\jsapi.cpp Line 1864]
>   Allocation location
>   ThreadID: 00003748 Timestamp: 2/26 04:57:24 169ms (Lifetime:00:15:50:157ms)
> Request ID: 00261604
>    0x004dbaa5  JS_malloc : [js\src\jsapi.cpp Line 1864]
>    0x005a23aa  js_NewScope : [js\src\jsscope.cpp Line 159]
>     154  : js_NewScope(JSContext *cx, jsrefcount nrefs, JSObjectOps *ops,
> JSClass *clasp,
>     155  :             JSObject *obj)
>     156  : {
>     157  :     JSScope *scope;
>     158  : 
>     159  :     scope = (JSScope *) JS_malloc(cx, sizeof(JSScope));
>     160  :     if (!scope)
>     161  :         return NULL;
>     162  : 
>     163  :     js_InitObjectMap(&scope->map, nrefs, ops, clasp);
>     164  :     scope->object = obj;
>    0x004fb9c7  js_NewObjectMap : [js\src\jsobj.cpp Line 2603]
>    0x004fc2f9  js_NewObjectWithGivenProto : [js\src\jsobj.cpp Line 2915]
>    0x004fbfbe  js_NewObject : [js\src\jsobj.cpp Line 2810]
>    0x004dd360  JS_InitClass : [js\src\jsapi.cpp Line 2790]
>    0x004f16fa  js_InitFunctionClass : [js\src\jsfun.cpp Line 2047]
>    0x004daad8  js_InitFunctionAndObjectClasses : [js\src\jsapi.cpp Line 1296]
>    0x004dadac  JS_InitStandardClasses : [js\src\jsapi.cpp Line 1359]
> 
> Let me know your thoughts.
With JIT disabled I get NO leaks, its clean.
Without your patch I cannot test for leaks.  I never get far enough before something asserts and dies.
Igor, any leak news? I can review tomorrow but I was waiting to hear what you found out. Thanks,

/be
(In reply to comment #51)
> Igor, any leak news? 

I have not found any leaks with the patch. But I wish that TraceMonkey tinderbox would include the corresponding tests - then I could use it as a try server avoiding all the complexity of tests setup.
Attached patch runtime-private JSThread v8 (obsolete) — Splinter Review
The new version updates the patch to reflect TM tip changes.
Attachment #364303 - Attachment is obsolete: true
Attachment #364904 - Flags: review?(brendan)
Attachment #364303 - Flags: review?(brendan)
Igor,

Seems that js_FinishJIT() is being fired way more than it should.
It seems to get called pretty much every time JS_SetPrototype() is called.
We aren't even running scripts yet!
We have two globals that are chained togther via prototype so we call this a lot.

Please see the following call stack as an example.

js_FinishJIT(JSTraceMonitor * tm=0x05891064)  Line 4508	C++
DestroyThread(JSThread * thread=0x05881028)  Line 115 + 0xe bytes	C++
thread_flusher(JSDHashTable * table=0x04b9e910, JSDHashEntryHdr * hdr=0x04bc8ab0, unsigned long __formal=2, void * arg=0x058adfd0)  Line 243 + 0x9 bytes	C++
JS_DHashTableEnumerate(JSDHashTable * table=0x04b9e910, JSDHashOperator (JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *)* etor=0x0054ca00, void * arg=0x058adfd0)  Line 742 + 0x19 bytes	C++
js_FlushThreadsForGC(JSContext * cx=0x058adfd0)  Line 258 + 0x17 bytes	C++
js_GC(JSContext * cx=0x058adfd0, JSGCInvocationKind gckind=GC_LOCK_HELD)  Line 3469 + 0x9 bytes	C++
js_SetProtoOrParent(JSContext * cx=0x058adfd0, JSObject * obj=0x07ea01a0, unsigned long slot=0, JSObject * pobj=0x06de1020)  Line 306 + 0xb bytes	C++
JS_SetPrototype(JSContext * cx=0x058adfd0, JSObject * obj=0x07ea01a0, JSObject * proto=0x06de1020)  Line 2985 + 0x1b bytes	C++

Let me know your thoughts.
FYI. Brendan said via IRC: "we moved JIT cache flushing out of the GC on purpose"
Which is why I added the above comment. :-)
(In reply to comment #54)
> Igor,
> 
> Seems that js_FinishJIT() is being fired way more than it should.

In your embedding, do you use JS_ClearContextThread() extensively and do you have a long-running threads?

> It seems to get called pretty much every time JS_SetPrototype() is called.

This is just a consequence of the fact that JS_SetPrototype() is a very expensive operation now days (hint: try to avoid it!) as it calls js_GC(). So the problem is that with the patch js_GC() would destroy JSThread instances that have no associated JSContext and in your embedding this happens frequently.
>> In your embedding, do you use JS_ClearContextThread() extensively and do you
>> have a long-running threads?

We have a pool of threads and a pool of contexts.
Whenever we associate the two together we use:
JS_SetContextThread()
JS_BeginRequest().

When we are done executing scripts we do this:

JS_ClearNewbornRoots(cx);
JS_ClearRegExpStatics(cx);
JS_ClearPendingException(cx);
JS_ClearScope();
JS_SetGlobalObject(cx, NULL); 
JS_EndRequest(cx);	
JS_ClearContextThread(cx);
JS_SetContextPrivate(cx, NULL)

Then the thread and the context go back to their respective pools.

>> do you have a long-running threads?
They are pooled so they never die, we suspend them.  The scripts don't really run that long a few seconds max.

> This is just a consequence of the fact that JS_SetPrototype() is a very
> expensive operation now days (hint: try to avoid it!) as it calls js_GC().
We can't.  This was a recommendation from one of you mozilla boys (can't remember who).  This is how we preserve a "session" in our web server.  We have a "session global".  We have a "request global", which is per request, that has session global as its prototype.  It works quite well.

> So the problem is that with the patch js_GC() would destroy JSThread instances
> that have no associated JSContext and in your embedding this happens
> frequently.
Why does that GC need to be there??  Makes no sense.  Also it penalizes other threads which are about to run a script.  It would be better if this GC was done AFTER script execution.

Can't we just take out the GC call from JS_SetPrototype?
JS_SetPrototype does the GC-to-check-for-cycles only #ifdef DEBUG, note well. We could take that out of DEBUG too. I kept it in to find API user errors where a cycle would be created, but the iloops those cause are easy to diagnose.

Mainly I didn't want to take away the checking that the API has done for many years, but if it's hurting your DEBUG perf, we can lose it. You should not see it hitting an optimized build at all.

But this is all beside the point. Embeddings that keep threads around in a pool for a long time should not suffer high costs on account of JIT cache flushing. If a thread is in the pool with no contexts associated with it, this could be a transient state that happens often, at high frequency. Can we use time to avoid thrashing here?

/be
(In reply to comment #57)
> We have a pool of threads and a pool of contexts.
> Whenever we associate the two together we use:
> JS_SetContextThread()
> JS_BeginRequest().

Effectively Firefox also uses a pool of JSContext instances, but it is a pool per thread which removes the need for JS_(Clear|Set)ContextThread().

> 
> When we are done executing scripts we do this:
> 
> JS_ClearNewbornRoots(cx);
> JS_ClearRegExpStatics(cx);
> JS_ClearPendingException(cx);
> JS_ClearScope();
> JS_SetGlobalObject(cx, NULL); 
> JS_EndRequest(cx);    
> JS_ClearContextThread(cx);
> JS_SetContextPrivate(cx, NULL)

Given the amount of the code and relative lightness of JSContext I suspect that just using JS_NewContext/JS_DestroyContextNoGC() would be a better solution.

In any case, I got the picture and will fix the issue tomorrow. I will change the patch to make JS_ClearContextThread effectively NO-OP so the GC would not prematurely remove JSThread instances.
(In reply to comment #58)
> But this is all beside the point.

This is not entirely besides the point since the problem requires both cx-less threads and frequent GC calls. The latter was triggered by the use of JS_SetPrototype.

> Can we use time to avoid thrashing here?
 
I would prefer to avoid adding yet another arbitrary timeout value and will try that
(In reply to comment #60)
> I would prefer to avoid adding yet another arbitrary timeout value and will try
> that

I meant that idea from the comment 59.
Igor,

Are you taking the DEBUG only GC out of JS_SetPrototype() too?

> JS_ClearContextThread effectively NO-OP so the GC would not prematurely remove JSThread instances.
I'm not understanding 100%.  What is being done prematurely? When should it be done then and by what mechanism?

I need the JS for dummies explanation. :-)
Mike, seems to me Igor was talking about the premature removal (and recreation) of JSThread instances, entailing lots of JIT cache flushing, etc. -- the topic of your comment 54.

We should lose the DEBUG-only cycle detection too.

/be
I think I like your idea.
So when does JIT cache flushing actually get done then?
Context destruction?
Blocks: 481380
Attached patch runtime-private JSThread v9 (obsolete) — Splinter Review
The new patch removes DEBUG-only invocation of js_GC from JS_Set(Parent|Prototype) and reorganizes the code that flushes various caches from the GC so both JS_THREADSAFE and !JS_THREADSAFE stays close.

The patch does *not* try to fix that potentially too-frequent flushing of the jit cache. I am not sure that it will be helpful for an embedding that uses a pool of threads. The problem is that with such pooling there is no guarantee that a script is executed on the same native thread repeatedly. Thus a relatively slow generation of native code may happen several times. This may well eclipse the problem of too-frequent jit cache flushing.
Attachment #365419 - Flags: review?(brendan)
Attachment #364904 - Attachment is obsolete: true
Attachment #364904 - Flags: review?(brendan)
(In reply to comment #57)
> > This is just a consequence of the fact that JS_SetPrototype() is a very
> > expensive operation now days (hint: try to avoid it!) as it calls js_GC().
> We can't.  This was a recommendation from one of you mozilla boys (can't
> remember who).  This is how we preserve a "session" in our web server.  We have
> a "session global".  We have a "request global", which is per request, that has
> session global as its prototype.  It works quite well.

I don't understand.  Why do you need JS_SetPrototype for this?  It seems like you could pass the session global as the `proto` parameter to JS_NewObject.
We could keep the cycle-checking in a form that wouldn't be 100% airtight in JS_THREADSAFE applications that share objects across threads -- but would still catch cycles in practice.
About too-frequent jit cache flushing: this patch does not change that at all as the GC *always* forces flushing of JIT caches on all threads. Compared with that an extra free/malloc of JSThread structure cannot add any visible overhead. Thus the patch cannot regress in that area and too-frequent JIT cache flushing if any under heavy multithreading should go to another bug.
Attached patch runtime-private JSThread v10 (obsolete) — Splinter Review
The new version adds JSThreadData structure to share the code between JS_THREADSAFE and !JS_THREADSAFE builds.
Attachment #365419 - Attachment is obsolete: true
Attachment #365894 - Flags: review?(brendan)
Attachment #365419 - Flags: review?(brendan)
Igor,
I tried applying this patch to latest tip. It failed to apply. Could you update again?
Brendan,

Can you r+ the latest patch?
I will review a fresh patch.

/be
Attached patch runtime-private JSThread v11 (obsolete) — Splinter Review
updated patch to sync with TM tip changes
Attachment #365894 - Attachment is obsolete: true
Attachment #366544 - Flags: review?(brendan)
Attachment #365894 - Flags: review?(brendan)
Reviewing, will finish up at home in a bit.

/be
Attached patch v12 (obsolete) — Splinter Review
The new version merges with the changes due to the bug 478336 plus I renamed various TraceSomethingRelatedToThread methods into FlushSomethingRelatedToThreadForGC. In all cases the code was protected with IS_GC_MARKING_TRACER(). Now those repeated useless chekcs are removed and the flush routins are called before the GC marking phases starts.
Attachment #366810 - Flags: review?(brendan)
Attachment #366544 - Attachment is obsolete: true
Attachment #366544 - Flags: review?(brendan)
Comment on attachment 366810 [details] [diff] [review]
v12

Looks good. How about ifdef reduction by always defining js_{Init,Finish}TraceThreads (the extern decls in jscntxt.h look unifdef'ed already) and making them have the ifdef JS_THREADSAFEs?

Good to move away from trace vs. trace confusion, but flush is not the right verb for anything that marks or scans for live things. It's good for cache flushing (purge is better IMHO). Having "ForGC" at the end is awkward if we can avoid it but for now this patch makes an improvement. No nit, just suggesting more work needed on our trace/flush-for-gc phrasing.

A few nits:

>+        /*
>+         * During the above wait after we are notified about the state change
>+         * but before we wake up, another thread can enter the GC from

s/can/could/

>+         * js_DestroyContext, bug 478336. So we must wait here to ensure that
>+         * when we exit the loop with the first set to true, that GC is

s/the first set/the first flag set/

>+struct JSThreadData {
>+    /*
>+     * The GSN cache is per thread, not per JSContext to save space. Any
>+     * embedding (Firefox or another Gecko application) that uses many
>+     * contexts per thread is unlikely to interleave js_GetSrcNote-intensive
>+     * loops in the decompiler among two or more contexts running script in
>+     * one thread.

This could be shortened to say "The GSN cache is per thread since even multi-cx-per-thread embeddings do not interleave js_GetSrcNote calls."

/be
Attachment #366810 - Flags: review?(brendan) → review+
(In reply to comment #76)
> just suggesting
> more work needed on our trace/flush-for-gc phrasing.

I like the idea of using just "Purge" to replace "Flush...ForGC". The usage of the verb in function names can be made to mean consistently "free the garbage and prepare for a new cycle".

I update the patch to use just Purge in place of "Flush...ForGC" or "Clear".
Attached patch v13 (obsolete) — Splinter Review
The new version addresses the nits and moves all JS_THREADSAFE or not logic for js_(Init|Finish|Flush)Threads into the functions itself. The version still uses FlushSomethingForGC, not PurgeSomething - I will do it in the next version for references.
Attachment #366810 - Attachment is obsolete: true
Attached patch v14 (obsolete) — Splinter Review
The new version renames ClearSomething, FlushSomething(ForGC)? into PurgeSomething. For consistency in the patch I replaced even when FlushSomething is not used during GC, like in js_FlushPropertyCacheForScript becoming js_PurgePropertyCacheForScript. 

But that may go too far, so I ask for a review again.
Attachment #366911 - Flags: review?(brendan)
(In reply to comment #79)
> Created an attachment (id=366911) [details]
> v14

Bugzilla's interdiff against v13 shows all the renames.
Comment on attachment 366911 [details] [diff] [review]
v14

The great flush/clear purge is complete! Thanks,

/be
Attachment #366911 - Flags: review?(brendan) → review+
Igor, How to we get these wonderful fixes landed? Mike
(In reply to comment #82)
> get these wonderful fixes landed?

I will try to land the patch tomorrow morning European time, when I am facing http://tinderbox.mozilla.org/showbuilds.cgi?tree=TraceMonkey alone.
landed to TM - http://hg.mozilla.org/tracemonkey/rev/4159ebdfe31e
Whiteboard: fixed-in-tracemonkey
I backed out the patch to fix some typos.
Whiteboard: fixed-in-tracemonkey
Attached patch v15 (obsolete) — Splinter Review
In the new patch I fixed a bad typo:

@@ -567,17 +567,17 @@ js_DestroyContext(JSContext *cx, JSDestr
     if (cx->requestDepth == 0)
         js_WaitForGC(rt);
     js_RevokeGCLocalFreeLists(cx);
 #endif
     JS_REMOVE_LINK(&cx->link);
     last = (rt->contextList.next == &rt->contextList);
     if (last)
         rt->state = JSRTS_LANDING;
-    if (last || mode == JSDCM_FORCE_GC || mode == mode == JSDCM_MAYBE_GC
+    if (last || mode == JSDCM_FORCE_GC || mode == JSDCM_MAYBE_GC
 #ifdef JS_THREADSAFE
         || cx->requestDepth != 0
 #endif
         ) {
         JS_UNLOCK_GC(rt);

         if (last) {
Attachment #366888 - Attachment is obsolete: true
Attachment #366911 - Attachment is obsolete: true
Attachment #367201 - Flags: review+
re-landed to TM - http://hg.mozilla.org/tracemonkey/rev/57de81309176
Whiteboard: fixed-in-tracemonkey
Flags: blocking1.9.1+
I backed out the landed patch.
Whiteboard: fixed-in-tracemonkey
I cannot reproduce the leaks neither locally nor with a try server build.
With the patch I have reproducible assert with mochi tests when running a debug build.
What ASSERT are you hitting?
I have found out the reason for mochi tests asserts. With the patch js_PurgeTraceMonitor is called from js_GC when it calls js_PurgeThreads before the label restart. This is wrong since the jit-reserved objects are preserved across the GC and, as such, they must be marked on each mark-and-sweep cycle, not once before the GC iterations starts. 

The thread workers tests from the mochi suite exposed this bug as with them the GC can be invoked several times increasing JSRuntime.gcLevel triggering several GC iterations. The bug is also visible with the following test case:

@watson-32~> cat ~/s/x.js
function f() {
    options("jit");
    for (var j = 0; j != 100; ++j) {
        for (var i = 0; i != 100; ++i) {
            var tmp = [];
        }
        gc();
    }
}

scatter([f, f]);
@watson-32~> ~/build/js32.tm.dbg/js ~/s/x.js
before 40960, after 24576, break 0827d000
before 45056, after 24576, break 0827d000
before 45056, after 32768, break 0827d000
Assertion failure: SCOPE_HAS_PROPERTY(scope, sprop), at /home/igor/m/tm/js/src/jsobj.cpp:5697
Trace/breakpoint trap
Attached patch v16 (obsolete) — Splinter Review
The new version fixes the issue from the comment 93. Now reservedObjects from the trace monitor are marked on each GC loop iteration as it is done currently without the patch.

I will ask for a review after more testing.
Attachment #367201 - Attachment is obsolete: true
Comment on attachment 367791 [details] [diff] [review]
v16

The new version passes mochi tests. The bug in the previous version could caused leaks although I am suprised it have not lead to crashes.
Attachment #367791 - Flags: review?(brendan)
Comment on attachment 367791 [details] [diff] [review]
v16

>+JSBool
>+js_InitContextThread(JSContext *cx)
> {
>+    JSRuntime *rt;
>+    jsword id = js_CurrentThreadId();
>+    const void *key = (const void *) id;
>+    JSThreadsHashEntry *entry;
>     JSThread *thread;
> 
>-    thread = (JSThread *)PR_GetThreadPrivate(threadTPIndex);
>-    if (!thread) {
>-        thread = (JSThread *) malloc(sizeof(JSThread));
>+    JS_ASSERT(!cx->thread);
>+
>+    rt = cx->runtime;
>+    JS_LOCK_GC(rt);

Nit: arbitrarily-ordered phalanx of initialized and uniniitalized declarations -- suggest moving in C++ style to before first use and initializing always.

Looks great otherwise.

/be
Attachment #367791 - Flags: review?(brendan) → review+
Attached patch v17 (obsolete) — Splinter Review
The new patch addresses the nit from the comment 96.
Attachment #367791 - Attachment is obsolete: true
Attachment #368676 - Flags: review+
landed to TM - http://hg.mozilla.org/tracemonkey/rev/e117c22cc1d1
Whiteboard: fixed-in-tracemonkey
I backed out the landed patch as it contains a shutdown leak. The preallocated objects from the trace monitor are always marked during the GC including the case of the js_GC call for the last context.
Whiteboard: fixed-in-tracemonkey
Attached patch v18Splinter Review
After backing out and fixing the patch not to leak JSRuntime.reservedObjects on shutdown I realized that the leak exists without the patch as well (bug 484633). So the new version of the patch fixes that bug as well. Here is a delta for convenience compared with the previous version:

Index: tm/js/src/jscntxt.cpp
===================================================================
--- tm.orig/js/src/jscntxt.cpp  2009-03-22 10:47:00.000000000 +0100
+++ tm/js/src/jscntxt.cpp       2009-03-22 10:30:45.000000000 +0100
@@ -109,10 +109,12 @@ PurgeThreadData(JSContext *cx, JSThreadD
     tm->needFlush = JS_TRUE;

     /*
-     * We want to keep tm->reservedObjects after the GC. So we don't purge
-     * them here and rather mark them during the GC, see MarkReservedObjects
-     * in jsgc.cpp.
+     * We want to keep tm->reservedObjects after the GC. So, unless we are
+     * shutting down, we don't purge them here and rather mark them during
+     * the GC, see MarkReservedObjects in jsgc.cpp.
      */
+    if (cx->runtime->state == JSRTS_LANDING)
+        tm->reservedObjects = NULL;
 # endif

     /* Destroy eval'ed scripts. */
Index: tm/js/src/jsgc.cpp
===================================================================
--- tm.orig/js/src/jsgc.cpp     2009-03-22 10:47:00.000000000 +0100
+++ tm/js/src/jsgc.cpp  2009-03-22 10:30:25.000000000 +0100
@@ -3163,7 +3163,8 @@ js_TraceRuntime(JSTracer *trc, JSBool al
             JS_CALL_OBJECT_TRACER(trc, rt->builtinFunctions[i], "builtin function");
     }

-    if (IS_GC_MARKING_TRACER(trc)) {
+    /* Mark the reserved objects unless we are shutting down. */
+    if (IS_GC_MARKING_TRACER(trc) && rt->state != JSRTS_LANDING) {
 #ifdef JS_THREADSAFE
         JS_DHashTableEnumerate(&rt->threads, reserved_objects_marker, NULL);
 #else
Attachment #368676 - Attachment is obsolete: true
Attachment #368771 - Flags: review?(brendan)
(In reply to comment #100)
> After backing out and fixing the patch not to leak JSRuntime.reservedObjects on
> shutdown I realized that the leak exists without the patch as well (bug
> 484633).

And that was wrong - the leak exists only with the versions of the patch prior v18. The difference comes from the fact that with the patch the runtime see all alive JSThread instances. That includes threads holding JSContext instances that are already decoupled from JSRuntime.contextList in js_DestroyContext but still exists on JSThread.contextList.
Attachment #368771 - Flags: review?(brendan) → review+
Blocks: 484861
Blocks: 408416
landed to TM - http://hg.mozilla.org/tracemonkey/rev/3b7dd1156e40
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/3b7dd1156e40
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 620251
You need to log in before you can comment on or make changes to this bug.