942346 - Bug 933882 broke non-ion builds

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Landry Breuil (:gaston)]

It seems this bug (well, a recent commit to jscompartment.cpp & Debugger.cpp, so likely https://hg.mozilla.org/mozilla-central/rev/9ee5c2664d78) broke non-ion builds: http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/620/steps/build/logs/stdio (fails when linking js shell)

../libjs_static.a(jscompartment.o)(.text+0x3ed4): In function `JSCompartment::removeDebuggee(js::FreeOp*, js::GlobalObject*, js::detail::HashTable<js::GlobalObject* const, js::HashSet<js::GlobalObject*, js::DefaultHasher<js::GlobalObject*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::Enum*)':
/home/buildslave/mozilla-central-sparc64/build/js/src/jscompartment.cpp:824: undefined reference to `js::AutoDebugModeInvalidation::~AutoDebugModeInvalidation()'
../libjs_static.a(jscompartment.o)(.text+0x53a4): In function `JSCompartment::addDebuggee(JSContext*, js::GlobalObject*)':
/home/buildslave/mozilla-central-sparc64/build/js/src/jscompartment.cpp:795: undefined reference to `js::AutoDebugModeInvalidation::~AutoDebugModeInvalidation()'
../libjs_static.a(Debugger.o)(.text+0xaf0c): In function `js::Debugger::addAllGlobalsAsDebuggees(JSContext*, unsigned int, JS::Value*)':
/home/buildslave/mozilla-central-sparc64/build/js/src/vm/Debugger.cpp:1965: undefined reference to `js::AutoDebugModeInvalidation::~AutoDebugModeInvalidation()'
../libjs_static.a(Debugger.o)(.text+0xaf30):/home/buildslave/mozilla-central-sparc64/build/js/src/vm/Debugger.cpp:1969: undefined reference to `js::AutoDebugModeInvalidation::~AutoDebugModeInvalidation()'
../libjs_static.a(Debugger.o)(.text+0xafc0): In function `js::Debugger::addDebuggeeGlobal(JSContext*, JS::Handle<js::GlobalObject*>)':
/home/buildslave/mozilla-central-sparc64/build/js/src/vm/Debugger.cpp:2127: undefined reference to `js::AutoDebugModeInvalidation::~AutoDebugModeInvalidation()'

shot in the dark, but maybe adding ~AutoDebugModeInvalidation(){}; for !JS_ION in jscompartment.h would help, but i'm not sure its' the best way.

Comment 1

[Shu-yu Guo [:shu]]

Ah you're right, this should have JS_ION guards. I'll have a patch in a bit.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: bug942346-invalidation-noion.patch

Brian, with this patch applied we can compile --disable-ion, but the JS shell then asserts in refillFreeList:

Assertion failure: !cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess() in ArenaLists::refillFreeList

Is that assert wrong?

Comment 3

[Shu-yu Guo [:shu]]

Is that assertion perhaps supposed to be the following?

    JS_ASSERT_IF(!cx->isJSContext() && allowGC,
                 cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess());

Comment 4

[Brian Hackett [Laid off!]]

That assert is correct.  If we try to GC while holding the exclusive access lock we will deadlock.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8337205 [details] [diff] [review]
bug942346-invalidation-noion.patch

Review of attachment 8337205 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine except for the assertion failure you're getting.  What is the stack when you hit that?

Comment 6

[Shu-yu Guo [:shu]]

Here's the assert:

Assertion failure: !cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess(), at ../jsgc.cpp:1535
Process 8651 stopped
* thread #1: tid = 0x1b0c19, 0x0000000100240d9b js`void* js::gc::ArenaLists::refillFreeList<(cx=0x0000000100e0bbd0, thingKind=FINALIZE_TYPE_OBJECT)1>(js::ThreadSafeContext*, js::gc::AllocKind) + 507 at jsgc.cpp:1534, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100240d9b js`void* js::gc::ArenaLists::refillFreeList<(cx=0x0000000100e0bbd0, thingKind=FINALIZE_TYPE_OBJECT)1>(js::ThreadSafeContext*, js::gc::AllocKind) + 507 at jsgc.cpp:1534
   1531	                 zone->gcBytes > zone->gcTriggerBytes;
   1532	
   1533	#ifdef JS_THREADSAFE
-> 1534	    JS_ASSERT_IF(cx->isJSContext() && allowGC,
   1535	                 !cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess());
   1536	#endif
   1537	
(lldb) bt
* thread #1: tid = 0x1b0c19, 0x0000000100240d9b js`void* js::gc::ArenaLists::refillFreeList<(cx=0x0000000100e0bbd0, thingKind=FINALIZE_TYPE_OBJECT)1>(js::ThreadSafeContext*, js::gc::AllocKind) + 507 at jsgc.cpp:1534, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000100240d9b js`void* js::gc::ArenaLists::refillFreeList<(cx=0x0000000100e0bbd0, thingKind=FINALIZE_TYPE_OBJECT)1>(js::ThreadSafeContext*, js::gc::AllocKind) + 507 at jsgc.cpp:1534
    frame #1: 0x0000000100246279 js`js::types::TypeObject* js::gc::NewGCThing<js::types::TypeObject, (cx=0x0000000100e0bbd0, kind=FINALIZE_TYPE_OBJECT, thingSize=56, heap=TenuredHeap)1>(js::ThreadSafeContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap) + 729 at jsgcinlines.h:441
    frame #2: 0x0000000100201b62 js`js::types::TypeCompartment::newTypeObject(this=0x0000000101046260, cx=0x0000000100e0bbd0, clasp=0x00000001006c9610, proto=Handle<js::TaggedProto> at 0x00007fff5fbfeeb8, unknown=true) + 210 at jsinfer.cpp:1703
    frame #3: 0x00000001001d0119 js`js::ExclusiveContext::getNewType(this=0x0000000100e0bbd0, clasp=0x00000001006c9610, proto_=TaggedProto at 0x00007fff5fbff1b0, fun_=0x0000000000000000) + 1401 at jsinfer.cpp:3786
    frame #4: 0x00000001002b2d97 js`js::NewObjectWithGivenProto(cxArg=0x0000000100e0bbd0, clasp=0x00000001006c9610, proto_=TaggedProto at 0x00007fff5fbff350, parent_=0x0000000000000000, allocKind=FINALIZE_OBJECT16_BACKGROUND, newKind=SingletonObject) + 759 at jsobj.cpp:1356
    frame #5: 0x000000010044083d js`js::NewObjectWithGivenProto(cx=0x0000000100e0bbd0, clasp=0x00000001006c9610, proto=TaggedProto at 0x00007fff5fbff398, parent=0x0000000000000000, newKind=SingletonObject) + 77 at jsobjinlines.h:803
    frame #6: 0x00000001003ede48 js`js::NewObjectWithGivenProto(cx=0x0000000100e0bbd0, clasp=0x00000001006c9610, proto=0x0000000000000000, parent=0x0000000000000000, newKind=SingletonObject) + 88 at jsobjinlines.h:810
    frame #7: 0x000000010039f6dc js`js::GlobalObject::create(cx=0x0000000100e0bbd0, clasp=0x00000001006c9610) + 156 at GlobalObject.cpp:430
    frame #8: 0x00000001001aee4e js`JS_NewGlobalObject(cx=0x0000000100e0bbd0, clasp=0x00000001006c9610, principals=0x0000000000000000, hookOption=DontFireOnNewGlobalHook, options=0x00007fff5fbff7d8) + 686 at jsapi.cpp:2553
    frame #9: 0x000000010045bfa1 js`JSRuntime::initSelfHosting(this=0x000000010101ee00, cx=0x0000000100e0bbd0) + 337 at SelfHosting.cpp:724
    frame #10: 0x00000001001a670e js`js::NewContext(rt=0x000000010101ee00, stackChunkSize=8192) + 238 at jscntxt.cpp:203
    frame #11: 0x00000001001a660d js`JS_NewContext(rt=0x000000010101ee00, stackChunkSize=8192) + 29 at jsapi.cpp:768
    frame #12: 0x000000010000349c js`NewContext(rt=0x000000010101ee00) + 28 at js.cpp:5218
    frame #13: 0x0000000100002030 js`main(argc=1, argv=0x00007fff5fbffa88, envp=0x00007fff5fbffa98) + 3152 at js.cpp:5878
    frame #14: 0x00000001000013d4 js`start + 52

Comment 7

[Shu-yu Guo [:shu]]

Oh I see, it's always true when JS_ION is not defined:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Runtime.h?from=currentThreadHasExclusiveAccess#803

Comment 8

[Shu-yu Guo [:shu]]

Quick r? on 
 
-#ifdef JS_THREADSAFE
+#ifdef JS_WORKER_THREADS
     JS_ASSERT_IF(cx->isJSContext() && allowGC,
                  !cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess());
 #endif

Comment 10

[Landry Breuil (:gaston)]

brian, i think comment 8 was for you ?

Comment 11

[Brian Hackett [Laid off!]]

I gave this an r+ on IRC.

Comment 12

[Shu-yu Guo [:shu]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4ace9f82b671

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ace9f82b671

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Fix AutoDebugModeInvalidation for builds without Ion. (

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8341342 [details] [diff] [review]
Fix AutoDebugModeInvalidation for builds without Ion. (

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 754201 most recently. Not sure what introduced AutoDebugModeGC initially.

User impact if declined: 
  Firebug unusable with many tabs. Firebug has worked around this by disabling JITs permanently if it's on, which is in some ways worse since it slows down *all* tabs.

Testing completed (on m-c, etc.): 
  On m-c.

Risk to taking this patch (and alternatives if risky): 
  GC behavior changes surfacing existing leaks/OOMs. When landing on m-c new leaks were surfaced (and eventually fixed). I am nominating those fixes for uplift also.

String or IDL/UUID changes made by this patch:
  None

The Firebug slowness metabug is bug 815603. List of bugs I'm nominating for uplift in this batch:

 - bug 936143
 - bug 935228
 - bug 933882
 - bug 935470 
 - bug 942346
 - bug 934799

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8341342 [details] [diff] [review]
Fix AutoDebugModeInvalidation for builds without Ion. (

We can take this on Aurora and thus it will be on Beta in a week, but we're way past the point of taking untracked, major change to Firefox 26 as we are going to build our final candidate today as well as the RC.

Comment 17

[Shu-yu Guo [:shu]]

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

Comment 18

[u279076]

Landry, can you please confirm this is fixed in Firefox 27 and 28?

Comment 19

[Landry Breuil (:gaston)]

The build is now busted for other fallout of other changes on sparc64, see

http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/647 - build failure, going to file another bug
http://buildbot.rhaalovely.net/builders/mozilla-aurora-sparc64/builds/271 - startup cache precompilation failure, probable js engine bustage, going to file another bug..


but i think that original linking issue was fixed by that commit.

Comment 20

[u279076]

(In reply to Landry Breuil (:gaston) from comment #19)
> The build is now busted for other fallout of other changes on sparc64...
> but i think that original linking issue was fixed by that commit.

Please link the other bugs here when reported.

Comment 21

[Landry Breuil (:gaston)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #20)
> (In reply to Landry Breuil (:gaston) from comment #19)
> > The build is now busted for other fallout of other changes on sparc64...
> > but i think that original linking issue was fixed by that commit.
> 
> Please link the other bugs here when reported.

That was #953410/#953120 (those are now fixed), and right now #953211/#950513 also come into play.