If |js_new<JaegerCompartment>| fails, JSCompartment::init() calls FinishJIT, and then fails. Once it fails, its parent NewCompartment calls |js_delete(compartment)|, which calls FinishJIT again. Solution: don't call FinishJIT the first time. Here's the log, which is fixed by the patch. An allocation failure at allocation 72/260 in ../jit-test/tests/arguments/args-createontrace.js causes problems (detected using bug 624094) Command (from obj directory, using patch from bug 624094): shell/js -A 72 -m -j -p -e "const platform='darwin'; const libdir='../jit-test/lib/';" -f ../jit-test/lib/prolog.js -f ../jit-test/tests/arguments/args-createontrace.js stdout, stderr, exitcode: ('', 'js(91218,0x7fff70672ca0) malloc: *** error for object 0x10180fc00: pointer being freed was not allocated\n*** set a breakpoint in malloc_error_break to debug\n', -6) Diagnosis: - Double-free Stack trace (from valgrind): The site of the failed allocation is: at: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4477) by: js_malloc (jsutil.h:232) by: js::mjit::JaegerCompartment* js_new<js::mjit::JaegerCompartment>() (jsutil.h:277) by: JSCompartment::init() (jscompartment.cpp:122) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:367) by: js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: js::SystemAllocPolicy::free(void*) (jstl.h:257) by: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::destroyTable(js::SystemAllocPolicy&, js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry*, unsigned int) (jshashtable.h:307) by: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: js::SystemAllocPolicy::free(void*) (jstl.h:257) by: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::destroyTable(js::SystemAllocPolicy&, js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry*, unsigned int) (jshashtable.h:307) by: js::detail::HashTable<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, unsigned long, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7821) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:367) by: js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: js::SystemAllocPolicy::free(void*) (jstl.h:257) by: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::destroyTable(js::SystemAllocPolicy&, js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry*, unsigned int) (jshashtable.h:307) by: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: js::SystemAllocPolicy::free(void*) (jstl.h:257) by: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::destroyTable(js::SystemAllocPolicy&, js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Entry*, unsigned int) (jshashtable.h:307) by: js::detail::HashTable<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::~HashTable() (jshashtable.h:368) by: js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::~HashMap() (jshashtable.h:767) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy> >(js::HashMap<unsigned char*, js::LoopProfile*, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7822) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid read of size x at: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: avmplus::BitSet::~BitSet() (avmplus.h:285) by: js::Oracle::~Oracle() (jstracer.h:282) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:307) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Invalid free() / delete / delete[] at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::~JSCompartment() (jscompartment.cpp:81) by: void js_delete<JSCompartment>(JSCompartment*) (jsutil.h:307) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2863) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) Address 0x is bytes inside a block of size free'd at: free (vg_replace_malloc.c:366) by: js_free (jsutil.h:247) by: void js_delete<js::Oracle>(js::Oracle*) (jsutil.h:308) by: js::FinishJIT(js::TraceMonitor*) (jstracer.cpp:7823) by: JSCompartment::init() (jscompartment.cpp:124) by: js::gc::NewCompartment(JSContext*, JSPrincipals*) (jsgc.cpp:2862) by: JS_NewCompartmentAndGlobalObject (jsapi.cpp:2959) by: NewGlobalObject(JSContext*) (js.cpp:5341) by: Shell(JSContext*, int, char**, char**) (js.cpp:5389) by: main (js.cpp:5563) The site of the failed allocation is: at: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4477) by: js_malloc (jsutil.h:232) by: js::SystemAllocPolicy::malloc(unsigned long) (jstl.h:255) by: js::detail::HashTable<unsigned long const, js::HashSet<unsigned long, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::createTable(js::SystemAllocPolicy&, unsigned int) (jshashtable.h:295) by: js::detail::HashTable<unsigned long const, js::HashSet<unsigned long, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::changeTableSize(int) (jshashtable.h:508) by: js::detail::HashTable<unsigned long const, js::HashSet<unsigned long, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::checkUnderloaded() (jshashtable.h:550) by: js::detail::HashTable<unsigned long const, js::HashSet<unsigned long, js::AtomHasher, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::Enum::~Enum() (jshashtable.h:214) by: js_SweepAtomState(JSContext*) (jsatom.cpp:457) by: MarkAndSweep(JSContext*, JSGCInvocationKind) (jsgc.cpp:2444) by: GCUntilDone(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2728) by: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2799) by: js_DestroyContext(JSContext*, JSDestroyContextMode) (jscntxt.cpp:1075) The site of the failed allocation is: at: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4477) by: js_malloc (jsutil.h:232) by: js::GCHelperThread::replenishAndFreeLater(void*) (jsgc.cpp:2126) by: js::GCHelperThread::freeLater(void*) (jsgc.h:928) by: JSContext::free(void*) (jscntxt.h:2052) by: JSString::finalize(JSContext*) (jsstrinlines.h:309) by: void FinalizeArenaList<JSString>(JSCompartment*, JSContext*, unsigned int) (jsgc.cpp:1952) by: JSCompartment::finalizeStringArenaLists(JSContext*) (jsgc.cpp:2029) by: MarkAndSweep(JSContext*, JSGCInvocationKind) (jsgc.cpp:2468) by: GCUntilDone(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2728) by: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2799) by: js_DestroyContext(JSContext*, JSDestroyContextMode) (jscntxt.cpp:1075) The site of the failed allocation is: at: VALGRIND_PRINTF_BACKTRACE (valgrind.h:4477) by: js_malloc (jsutil.h:232) by: js::GCHelperThread::replenishAndFreeLater(void*) (jsgc.cpp:2126) by: js::GCHelperThread::freeLater(void*) (jsgc.h:928) by: JSContext::free(void*) (jscntxt.h:2052) by: JSString::finalize(JSContext*) (jsstrinlines.h:309) by: void FinalizeArenaList<JSString>(JSCompartment*, JSContext*, unsigned int) (jsgc.cpp:1952) by: JSCompartment::finalizeStringArenaLists(JSContext*) (jsgc.cpp:2029) by: MarkAndSweep(JSContext*, JSGCInvocationKind) (jsgc.cpp:2468) by: GCUntilDone(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2728) by: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2799) by: js_DestroyContext(JSContext*, JSDestroyContextMode) (jscntxt.cpp:1075)

Yeah, I guess this is pretty broken. I think there's more than this to fix though. For example, if crossCompartmentWrappers.init() fails, then we're going to call FinishJIT without having called InitJIT. It seems like we might need a flag to say whether InitJIT succeeded or not. Yuck. Also, the JSCompartment constructor should be initializing regExpAllocator and jaegerCompartment to NULL. That way, if we js_delete them in the destructor without them having been allocated (due to OOM), we're safe.

I agree with your assessment, there are more problems here. I think this patch addresses them. We could be calling the destructor before InitJIT is called, so we need to clear traceMonitor in the constructor. We need to allocate the pointers, as you say. This runs clean on my OOM checker, so I'm relatively confident it gets everything. (Looking one step higher: I think I may follow this up with a dehydra analysis that asserts that pointers are initialized to NULL in constructors. What do you think?)

This basically works, but I think it's still slightly wrong. It relies on FinishJIT succeeding when it's called on zero'd memory, and I think it crash. It looks like we might call FragProfiling_showResults(tm) during FinishJIT. FragProfiling_showResults does this: FragStatsMap::Iter iter(*tm->profTab); Although this doesn't directly do a dereference (since FragStatsMap::Iter takes a reference param), the constructor does eventually dereference the parameter, which will crash. I'm not 100% sure that path is feasible, but I'd just feel a lot better if we had a flag somewhere that determined whether FinishJIT should happen. I know it's ugly, but it's a lot safer and more future-proof. Also, I think that we're trying to transition memset to PodZero, which does the same thing.

(In reply to comment #3) > It looks like we might call FragProfiling_showResults(tm) during FinishJIT. > I'm not 100% sure that path is feasible, but I'd just feel a lot better if we > had a flag somewhere that determined whether FinishJIT should happen. I know > it's ugly, but it's a lot safer and more future-proof. It looks to me like we can hit this under TMFLAGS=something. I'm not sure at all about a flag, mostly because I don't know how that would work. What if we allocate most of an object, but not all. Then the flag is set and we leak. I think there is an implicit flag: each pointer is NULL or not-NULL, and must be checked before dereferencing during deallocation. Perhaps the solution here is to move the logging code out of the deallocation path?

If InitJIT returns an error, we should never call FinishJIT. If InitJIT succeeds, we should always call FinishJIT. This is how the code is currently written, and I think that in the future, people will assume that it's supposed to work this way. I realize that this is different from the ctor/init/dtor pattern. But that's because we can't stop dtors from being called. We can stop FinishJIT from being called. So I think that JSCompartment should have a flag that says whether to call FinishJIT. The flag would be set to false in the constructor, and would be set to true in init() of InitJIT succeeds. I guess you could also move the logging code to a different place, but then we should have a comment at the top of FinishJIT that says "This code must be prepared to work regardless of whether InitJIT succeeded or failed." Otherwise people will just keep adding stuff like the logging code to it. Personally, I think most people would assume that a finish-style function that is not a dtor will only be called if the init function succeeded. That's the typical pattern in C, at least.

(In reply to comment #5) > If InitJIT returns an error, we should never call FinishJIT. If InitJIT > succeeds, we should always call FinishJIT. This is how the code is currently > written, and I think that in the future, people will assume that it's supposed > to work this way. > > So I think that JSCompartment should have a flag that says whether to call > FinishJIT. The flag would be set to false in the constructor, and would be set > to true in init() of InitJIT succeeds. Then how will we clean up the memory partially-allocated by InitJIT? > I guess you could also move the logging code to a different place, but then we > should have a comment at the top of FinishJIT that says "This code must be > prepared to work regardless of whether InitJIT succeeded or failed." Otherwise > people will just keep adding stuff like the logging code to it. A comment is a good idea. > Personally, I think most people would assume that a finish-style function that > is not a dtor will only be called if the init function succeeded. That's the > typical pattern in C, at least. Right, this kinda points to the real problem: FinishJIT is a destructor, but it's also not. I looked at making FinishJIT a destructor, but that was a much larger change than I think we should make at this stage in the release.

I guess I was wrong. I assumed that InitJIT would clean up after itself if it failed, but it doesn't. I guess we should add the comment and fix FinishJIT works even when InitJIT failed. The easiest way is probably to guard FragProfiling_showResults(tm) by |if (tm->profTab)|. It looks like that is safe.

Another alternative is to ignore this. This is protected by TMFLAGS, if I understand correctly, so it'll never happen in real life.

Where by "ignore it", I mean add a comment saying that this area of code isn't OOM-safe, because it's never run except under the shell.

I dunno. It seems easy enough to fix. Just something along the lines of: /* Warning: FinishJIT must work regardless of whether InitJIT succeeds. */ ... if (tm->profTab) FragProfiling_showResults(tm) Then we don't have to worry about it any more.

This also includes another few dereferences in FinishJIT.

Comment on attachment 507711 [details] [diff] [review] Check around fragprofile Looks good. tm->vmfragments is an array, so the test on it isn't necessary. In fact, I think it will always be taken, since you're just getting &tm->vmfragments[0].

So the problem is not lack of init : finish :: (ctor-then-init) : dtor parallelism, right? It's just that InitJIT did not clean up after itself on failure so as to relieve its caller(s) from having to FinishJIT. Right? If so that ought to be fixed the usual way (zero-init, null-safe finish). Just trying to keep score at home here. /be

Bug 626547 is more or less the same as this bug. Brendan weighed in there...

In bug 626547 comment 2, Brendan indicated a preference for InitJIT calling FinishJIT on failure. I'll rework this patch so.

Take into account thgat FinishJIT may be called twice (as njn points out in bug 626547 comment 0).

Hmm, I thought the consensus on the mailing list was that goto error paths were to be avoided. Having said that, I can see its usefulness here. So in the absence of other information: continue on!

Just to mention that I removed the checks on vmfragments, as they'd always be true. The snippet below looked to me like a pointer to an array, not an array of pointers, which is what confused me, so I fixed it. But I didn't fix the rest of the file (jscompartment.h), which doesn't follwo the coding style in relation to the position of the |*|. - TreeFragment* vmfragments[FRAGMENT_TABLE_SIZE]; + TreeFragment *vmfragments[FRAGMENT_TABLE_SIZE];

(In reply to comment #18) > Hmm, I thought the consensus on the mailing list was that goto error paths were > to be avoided. Having said that, I can see its usefulness here. I took that to mean that we prefer the C++ style (no finish() function, cleanup in a destructor) to the C style (init() calls finish(), no destructor), but that when using the C style, gotos are often necessary and tolerated.

(In reply to comment #20) > > I took that to mean that we prefer the C++ style (no finish() function, cleanup > in a destructor) to the C style (init() calls finish(), no destructor), but > that when using the C style, gotos are often necessary and tolerated. Right, yes: the C++ style should be used in new code. Thanks!

Yeah, I think the InitJIT/FinishJIT stuff could do with shifting to C++-style, but it isn't a simple refactor, which is why I didn't do it.

hg bisect tells me this patch is responsible for this leak: ==29360== 128 bytes in 1 blocks are definitely lost in loss record 1 of 2 ==29360== at 0x47E9F20: malloc (vg_replace_malloc.c:236) ==29360== by 0x805B824: js_malloc (jsutil.h:210) ==29360== by 0x80784C2: js::SystemAllocPolicy::malloc(unsigned int) (jstl.h:255) ==29360== by 0x82412E6: js::detail::HashTable<JSScript* const, js::HashSet<JSScript*, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::createTable(js::SystemAllocPolicy&, unsigned int) (jshashtable.h:295) ==29360== by 0x823F49D: js::detail::HashTable<JSScript* const, js::HashSet<JSScript*, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::init(unsigned int) (jshashtable.h:351) ==29360== by 0x823D1B3: js::HashSet<JSScript*, js::DefaultHasher<JSScript*>, js::SystemAllocPolicy>::init(unsigned int) (jshashtable.h:997) ==29360== by 0x8208BA0: js::InitJIT(js::TraceMonitor*) (jstracer.cpp:7769) ==29360== by 0x80A9FB5: JSCompartment::init() (jscompartment.cpp:121) ==29360== by 0x805FA8A: JSRuntime::init(unsigned int) (jsapi.cpp:657) ==29360== by 0x806BC63: JS_Init (jsapi.cpp:768) ==29360== by 0x80570A7: main (js.cpp:5563) tm->traceScripts' memory isn't being freed in some way.

tm->tracedScripts was held by value in TraceMonitor, and its destructor was called by the JSCompartment destructor (since TraceMonitor is held by value in the compartment). The PodZero(tm) thing in FinishJIT messed this up. For now, I removed the PodZero, since it's not really needed right now (and it was causing orange). http://hg.mozilla.org/tracemonkey/rev/e510e0303de5 In the future, we should probably make TraceMonitor use the normal ctor/init/dtor pattern. Now that it's in the compartment, I don't think this should be a problem. That way, problems like this will be much less likely to crop up.

(In reply to comment #19) > Just to mention that I removed the checks on vmfragments, as they'd always be > true. The snippet below looked to me like a pointer to an array, not an array > of pointers, which is what confused me, so I fixed it. But I didn't fix the > rest of the file (jscompartment.h), which doesn't follwo the coding style in > relation to the position of the |*|. > > - TreeFragment* vmfragments[FRAGMENT_TABLE_SIZE]; > + TreeFragment *vmfragments[FRAGMENT_TABLE_SIZE]; More evidence that the C++ creator style of cuddling * with type not declarator is misleading, since the grammar works generally via an inside-out-spiral. I blame gal :-/. /be

So we were missing the free on tm->tracedScripts, as billm said. Calling thedestructor seems to solve the problem. I've seen bugs go past discussing calling a destructor like this, and I'm not sure it's totally legit, so asking luke for secondary review. I've also gone through InitJIT systematically, to make sure I didn't miss something else like this. Looks OK, and I reorganized the free calls in reverse order of allocation as I went. Seems easier to follow, so I didn't throw it away. Checked this with |make check-valgrind| as well as OOM.py; both the leaks and OOM problems are gone.

(In reply to comment #26) > More evidence that the C++ creator style of cuddling * with type not declarator > is misleading, since the grammar works generally via an inside-out-spiral. More likely the C++ creator would say to use vector<> or something like that. :-)

(In reply to comment #27) > Created attachment 508968 [details] [diff] [review] > Fix OOM without leaking Can't we just move to a ctor/init/dtor pattern for the TraceMonitor? That would avoid the grossness of explicit destructor calls, as well as the abstraction breaking that we're doing with HashTable (by zeroing it instead of calling the ctor, and by requiring the dtor to work with all zeros). You said you tried this, Paul. What problems did you run into?

(In reply to comment #29) > You said you tried this, Paul. What problems did you run into? I didn't try it, cause it looked non-simple. But yeah, at this point, I think doing it properly is the right solution.

Comment on attachment 508968 [details] [diff] [review] Fix OOM without leaking I agree with Bill (and as of comment 30, Paul too :).

This looks a lot better, isn't weird, and passes most tests (the rest are running now).

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/03a995099483 http://hg.mozilla.org/mozilla-central/rev/e510e0303de5

I guess we should remove [fixed-in-tracemonkey] on backouts to avoid cdleary-bot marking things as fixed.

Couldn't we just remove InitJIT and FinishJIT entirely and move this stuff into init() and the destructor?

(In reply to comment #36) > Couldn't we just remove InitJIT and FinishJIT entirely and move this stuff into > init() and the destructor? I think you mean into Tracemonitor, not JSCompartment. The functionality from InitJIT/FinishJIT doesn't really belong in TraceMonitor to my mind. We could split InitJIT into SetupJITLogging, CheckProcessorFeatures and setupJITStats; and FinishJIT into printJITStats and displayJITLogging. But it doesn't look like that would be any prettier than this patch.

I see. I guess I'd be okay having InitJIT and FinishJIT if they didn't take a tm parameter and were just called when initializing/finalizing the runtime. Then we could deal with the processor features and jit stats there. This would also eliminate a needless race. It seems like the logging stuff is specific to a TraceMonitor, so I don't see why we can't shouldn't it in init() and the dtor. Also, it seems like your patch deletes the code starting with the comment "/* Set up fragprofiling, if required. */". I don't see it getting moved anywhere else. Is that right?

(In reply to comment #38) > I see. I guess I'd be okay having InitJIT and FinishJIT if they didn't take a > tm parameter and were just called when initializing/finalizing the runtime. OK, done this. > Then we could deal with the processor features and jit stats there. This would > also eliminate a needless race I moved these to the runtime. As I understand it, we can have multiple runtimes in the same process, so the race will still exist, but maybe Firefox doesn't do that. > It seems like the logging stuff is specific to a TraceMonitor, so I don't see > why we can't shouldn't it in init() and the dtor. I moved it into its own function, called by the dtor. > Also, it seems like your patch deletes the code starting with the comment "/* > Set up fragprofiling, if required. */". I don't see it getting moved anywhere > else. Is that right? The code in that comment just allocates. I made it allocate unconditionally to simplify the logic in the constructor and destructor. It's behind a DEBUG ifdef, so it should be fine.

Comment on attachment 511910 [details] [diff] [review] Fix reviewer comments I'm really sorry this took me so long, Paul. It's a really nice cleanup. - -#ifdef JS_TRACER - /* InitJIT expects this area to be zero'd. */ PodZero(&traceMonitor); -#endif - The traceMonitor field in JSCompartment is guarded by |#ifdef JS_TRACER|, so I think the #ifdef here is still needed. However, I don't think we want to be doing the PodZero at all here. The traceMonitor constructor will already have run at this point, and it should have initialized everything to zero. It would be confusing if someone actually tried to initialize something to non-zero in the ctor, and this PodZero overwrote it. + JS_ASSERT(!traceMonitor.recorder); + JS_ASSERT(!traceMonitor.profile); Why not put this stuff in TraceMonitor::~TraceMonitor? -#ifdef JS_TRACER - if (!InitJIT(&traceMonitor)) + if (!traceMonitor.init()) return false; -#endif This also still needs the |#ifdef JS_TRACER|. In ~TraceMonitor, if we're not doing fragment profiling, we should assert !tm->profTab and !tm->profAlloc. r+ with these addressed.

Paul's patch ported to latest TM tip

The attached patch is a port of Paul's original patch with some of Bill's remarks addressed and it passes all reftests. I removed the PodZero call entirely and left the #ifdef's where marked. For compatibility, I extended the TraceMonitor to store the JSRuntime* as well for allocations. In TraceMonitor::init() I moved the flush() behind the tracedScripts.init() call, is this correct? Otherwise, valgrind spilled out use of uninitialized values. I just noticed that I forgot to move the two JS_ASSERTs to ~TraceMonitor though, will do that tomorrow in a followup version. About tm->profTab/tm->profAlloc, how exactly should I assert on those, i.e. what's the condition to check for exactly?

(In reply to comment #42) > About tm->profTab/tm->profAlloc, how exactly should I assert on those, i.e. > what's the condition to check for exactly? Thanks a lot! I just wanted assertions that they're both NULL. Also, I noticed one other nit. Putting commas at the beginning of each line in the TraceMonitor constructor goes against SpiderMonkey style. They should go at the end.

> Thanks a lot! I just wanted assertions that they're both NULL. Sorry, I meant how to check that we're doing fragment profiling at all (i.e. the condition surrounding the JS_ASSERTS).

Comment on attachment 528040 [details] [diff] [review] Ported patch Nice cleanup, thanks! I don't think you need to include rt as a member of TraceMonitor, you can just pass it as an argument. If there is a good reason to keep it as a member, then you should use it to deallocate in ~TraceMonitor too.

Paul, in the old code the profTab stuff was allocated conditionally (only when fragment profiling was done). With your patch, it's done unconditionally (which is also the reason why I can't assert on it being NULL when no fragment profiling is done). Was that intended or just a mistake? If it's not what was intended, I can make the allocation conditional again + the asserts if you think that's a good idea.

(In reply to comment #47) I deliberately switched to allocating unconditionally, because I felt it was simpler. If it turns out not to be simpler, yeah, switch back and put the asserts back in.

(In reply to comment #48) I agree it's easier and I don't think it's worth re-adding that just to be able to assert on it. I just wanted to make sure there was no other reason :) I'll update the patch soon.

Updated patch that fixes nits (leading commas removed where not required) and makes JSRuntime only a param of TraceMonitor::init instead of using a member.

decoder, do you need to mark the old patches as obsolete and request review on the new patch?

Fixes a small bug. Try server link: http://tbpl.mozilla.org/?tree=Try&rev=8fe240d66a5d

I have seen a11 errors with one of my patches as well. Shifting/exchanging some code blocks fixed that...

With this landed, we see a11y oranges in M(other). dvanders patch from bug 638680 also triggered those errors, and was backed out. So backing this out for now, and if things stay orange, I'll push it again, otherwise I'll block on bug 652459. Backout: http://hg.mozilla.org/tracemonkey/rev/41b74f0b32fe Merge: http://hg.mozilla.org/tracemonkey/rev/b5302c13e059

That orange is still there, so it's not due to this :) Philor advised closing the tree, so gonna wait till things are greener before relanding.

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/334ada87e329 http://hg.mozilla.org/mozilla-central/rev/41b74f0b32fe (backout) Note: not marking as fixed because last changeset is a backout.

Things are green again, so relanded: http://hg.mozilla.org/tracemonkey/rev/84edaddc432f

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/84edaddc432f