Closed Bug 942346 Opened 6 years ago Closed 6 years ago

Bug 933882 broke non-ion builds

Categories

(Core :: JavaScript Engine: JIT, defect)

26 Branch
Sun
OpenBSD
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- wontfix
firefox27 --- verified
firefox28 --- verified

People

(Reporter: gaston, Assigned: shu)

References

Details

Attachments

(2 files)

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.
Blocks: 933882
Ah you're right, this should have JS_ION guards. I'll have a patch in a bit.
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?
Assignee: nobody → shu
Attachment #8337205 - Flags: review?(bhackett1024)
Is that assertion perhaps supposed to be the following?

    JS_ASSERT_IF(!cx->isJSContext() && allowGC,
                 cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess());
That assert is correct.  If we try to GC while holding the exclusive access lock we will deadlock.
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?
Attachment #8337205 - Flags: review?(bhackett1024) → review+
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
Quick r? on 
 
-#ifdef JS_THREADSAFE
+#ifdef JS_WORKER_THREADS
     JS_ASSERT_IF(cx->isJSContext() && allowGC,
                  !cx->asJSContext()->runtime()->currentThreadHasExclusiveAccess());
 #endif
Duplicate of this bug: 943493
brian, i think comment 8 was for you ?
Flags: needinfo?(bhackett1024)
I gave this an r+ on IRC.
Flags: needinfo?(bhackett1024)
https://hg.mozilla.org/mozilla-central/rev/4ace9f82b671
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
Attachment #8341342 - Flags: approval-mozilla-beta?
Attachment #8341342 - Flags: approval-mozilla-aurora?
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.
Attachment #8341342 - Flags: approval-mozilla-beta?
Attachment #8341342 - Flags: approval-mozilla-beta-
Attachment #8341342 - Flags: approval-mozilla-aurora?
Attachment #8341342 - Flags: approval-mozilla-aurora+
Landry, can you please confirm this is fixed in Firefox 27 and 28?
Flags: needinfo?(landry)
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.
Flags: needinfo?(landry)
(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.
Status: RESOLVED → VERIFIED
(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.
You need to log in before you can comment on or make changes to this bug.