437325 - gc & multiple rt confusion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: bug-437325-assertion.patch

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.

Comment 4

[Igor Bukanov]

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.

Comment 5

[Igor Bukanov]

(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.

Comment 6

[timeless]

Attachment: bundle

Comment 7

[timeless]

Attachment: only count this runtime's requests

Comment 8

[Igor Bukanov]

(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.

Comment 9

[Igor Bukanov]

Attachment: runtime-private JSThread

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.

Comment 10

[Igor Bukanov]

Comment on attachment 350297 [details] [diff] [review]
only count this runtime's requests

I incorporated this fix into the patch for the bug 477627.

Comment 11

[Igor Bukanov]

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.

Comment 12

[Igor Bukanov]

Attachment: runtime-private JSThread v2

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.

Comment 13

[Igor Bukanov]

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.

Comment 14

[Mike Moening]

I'll try to test this today or tommorrow.

Comment 15

[Igor Bukanov]

Attachment: runtime-private JSThread v3

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.

Comment 16

[Brendan Eich [:brendan]]

I will review this tomorrow a.m. -- looking for the good word from MikeM too.

/be

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Mike Moening]

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.

Comment 19

[Mike Moening]

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.

Comment 20

[Igor Bukanov]

(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.

Comment 21

[Igor Bukanov]

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.

Comment 22

[Igor Bukanov]

I CC Gal to have his input on the comment 21.

Comment 23

[Mike Moening]

> 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.

Comment 24

[Mike Moening]

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.

Comment 25

[Mike Moening]

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)

Comment 26

[Igor Bukanov]

To Mike:  could you check if you would see the assert in RECURSION_LEVEL(table) if you disable the jit?

Comment 27

[Mike Moening]

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... :-)

Comment 28

[Igor Bukanov]

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.

Comment 29

[Mike Moening]

So I suppose either take it out or use an atomic increment/decrement?

Comment 30

[Igor Bukanov]

Attachment: runtime-private JSThread v4

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.

Comment 31

[Mike Moening]

> I still have no clue about the jit crash.
Then who does?

Comment 32

[Mike Moening]

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.

Comment 33

[Mike Moening]

Apparently that had no impact. 
The NULL pointer crash still exists.

Comment 34

[Igor Bukanov]

(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?

Comment 35

[Mike Moening]

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?

Comment 36

[Igor Bukanov]

Attachment: extra patch to make trace oracle JSRuntime field

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?

Comment 37

[Igor Bukanov]

(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.

Comment 38

[Andreas Gal :gal]

CCing danderson. The JIT crash might be related to a bug he is working on. David what do you think?

Comment 39

[Mike Moening]

Gal, what is the number for the other bug?

Comment 40

[Igor Bukanov]

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.

Comment 41

[Mike Moening]

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.

Comment 42

[Mike Moening]

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).

Comment 43

[Igor Bukanov]

Attachment: runtime-private JSThread v5

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.

Comment 44

[Igor Bukanov]

To Mike Moening: thanks for the detailed memory leak report! It clearly showed the problem with the patch.

Comment 45

[Igor Bukanov]

Attachment: runtime-private JSThread v6

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.

Comment 46

[Igor Bukanov]

Comment on attachment 364298 [details] [diff] [review]
runtime-private JSThread v6

The patch uses a wrong assert.

Comment 47

[Igor Bukanov]

Attachment: runtime-private JSThread v7

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.

Comment 48

[Mike Moening]

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.

Comment 49

[Igor Bukanov]

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.

Comment 50

[Mike Moening]

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.

Comment 51

[Brendan Eich [:brendan]]

Igor, any leak news? I can review tomorrow but I was waiting to hear what you found out. Thanks,

/be

Comment 52

[Igor Bukanov]

(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.

Comment 53

[Igor Bukanov]

Attachment: runtime-private JSThread v8

The new version updates the patch to reflect TM tip changes.

Comment 54

[Mike Moening]

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.

Comment 55

[Mike Moening]

FYI. Brendan said via IRC: "we moved JIT cache flushing out of the GC on purpose"
Which is why I added the above comment. :-)

Comment 56

[Igor Bukanov]

(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.

Comment 57

[Mike Moening]

>> 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?

Comment 58

[Brendan Eich [:brendan]]

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

Comment 59

[Igor Bukanov]

(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.

Comment 60

[Igor Bukanov]

(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

Comment 61

[Igor Bukanov]

(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.

Comment 62

[Mike Moening]

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. :-)

Comment 63

[Brendan Eich [:brendan]]

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

Comment 64

[Mike Moening]

I think I like your idea.
So when does JIT cache flushing actually get done then?
Context destruction?

Comment 65

[Igor Bukanov]

Attachment: runtime-private JSThread v9

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.

Comment 66

[Jason Orendorff [:jorendorff]]

(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.

Comment 67

[Jason Orendorff [:jorendorff]]

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.

Comment 68

[Igor Bukanov]

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.

Comment 69

[Igor Bukanov]

Attachment: runtime-private JSThread v10

The new version adds JSThreadData structure to share the code between JS_THREADSAFE and !JS_THREADSAFE builds.

Comment 70

[Mike Moening]

Igor,
I tried applying this patch to latest tip. It failed to apply. Could you update again?

Comment 71

[Mike Moening]

Brendan,

Can you r+ the latest patch?

Comment 72

[Brendan Eich [:brendan]]

I will review a fresh patch.

/be

Comment 73

[Igor Bukanov]

Attachment: runtime-private JSThread v11

updated patch to sync with TM tip changes

Comment 74

[Brendan Eich [:brendan]]

Reviewing, will finish up at home in a bit.

/be

Comment 75

[Igor Bukanov]

Attachment: v12

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.

Comment 76

[Brendan Eich [: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

Comment 77

[Igor Bukanov]

(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".

Comment 78

[Igor Bukanov]

Attachment: v13

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.

Comment 79

[Igor Bukanov]

Attachment: v14

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.

Comment 80

[Igor Bukanov]

(In reply to comment #79)
> Created an attachment (id=366911) [details]
> v14

Bugzilla's interdiff against v13 shows all the renames.

Comment 81

[Brendan Eich [:brendan]]

Comment on attachment 366911 [details] [diff] [review]
v14

The great flush/clear purge is complete! Thanks,

/be

Comment 82

[Mike Moening]

Igor, How to we get these wonderful fixes landed? Mike

Comment 83

[Igor Bukanov]

(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.

Comment 84

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/4159ebdfe31e

Comment 85

[Igor Bukanov]

I backed out the patch to fix some typos.

Comment 86

[Igor Bukanov]

Attachment: v15

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) {

Comment 87

[Igor Bukanov]

re-landed to TM - http://hg.mozilla.org/tracemonkey/rev/57de81309176

Comment 88

[Igor Bukanov]

The patch caused similar-looking leak reports on Linux and Mac (but on Windows), 

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1236944350.1236955331.22803.gz

and

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1236956940.1236965088.9766.gz

Comment 89

[Igor Bukanov]

I backed out the landed patch.

Comment 90

[Igor Bukanov]

I cannot reproduce the leaks neither locally nor with a try server build.

Comment 91

[Igor Bukanov]

With the patch I have reproducible assert with mochi tests when running a debug build.

Comment 92

[Mike Moening]

What ASSERT are you hitting?

Comment 93

[Igor Bukanov]

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

Comment 94

[Igor Bukanov]

Attachment: v16

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.

Comment 95

[Igor Bukanov]

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.

Comment 96

[Brendan Eich [: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

Comment 97

[Igor Bukanov]

Attachment: v17

The new patch addresses the nit from the comment 96.

Comment 98

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/e117c22cc1d1

Comment 99

[Igor Bukanov]

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.

Comment 100

[Igor Bukanov]

Attachment: v18

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

Comment 101

[Igor Bukanov]

(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.

Comment 102

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/3b7dd1156e40

Comment 103

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3b7dd1156e40

Comment 104

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b7bcac17cd91