Last Comment Bug 543839 - JS_Assert failure "Thin_GetWait(tl->owner)" in jslock.cpp:1157 [@ThinUnlock]
: JS_Assert failure "Thin_GetWait(tl->owner)" in jslock.cpp:1157 [@ThinUnlock]
Status: RESOLVED FIXED
fixed-in-tracemonkey
: crash, regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
: 543903 (view as bug list)
Depends on:
Blocks: 538275
  Show dependency treegraph
 
Reported: 2010-02-02 14:12 PST by Chris Pearce (:cpearce)
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
alpha1+


Attachments
fix v1 (3.63 KB, patch)
2010-02-03 15:07 PST, Igor Bukanov
jorendorff: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2010-02-02 14:12:21 PST
I ran into a JS_ASSERT failure while running media mochitests today (content/media/test/test_closing_connections.html). I've heavily modified the ogg decoder backend, but this assert is in JS code, so I'm confident it's not caused by me.

Running trunk on Windows7 Pro 64bit (default 32 bit FF build).

My tip is:
changeset:   37801:afff5c13d296
tag:         qparent
user:        Ben Turner <bent.mozilla@gmail.com>
date:        Mon Feb 01 12:35:48 2010 -0800
summary:     Bug 542915 - 'Protect DelayedReleaseGCCallback from reentering and double-freeing NPObjects' r=jst+josh.

The assertion that fails is at js/src/jslock.cpp:1157 "JS_ASSERT(Thin_GetWait(tl->owner));"

Main thread stack:

KernelBase.dll!_DebugBreak@0()  + 0x2 bytes	
mozjs.dll!JS_Assert(const char * s=0x67974a08, const char * file=0x679749e0, int ln=1157)  Line 67	C++
mozjs.dll!ThinUnlock(JSThinLock * tl=0x063577b4, int me=6562664)  Line 1157 + 0x20 bytes	C++
mozjs.dll!js_DropAllEmptyScopeLocks(JSContext * cx=0x031b32f0, JSScope * scope=0x063577a8)  Line 1295 + 0x16 bytes	C++
mozjs.dll!js_GetMutableScope(JSContext * cx=0x031b32f0, JSObject * obj=0x03578a20)  Line 132 + 0x40 bytes	C++
mozjs.dll!js_NonEmptyObject(JSContext * cx=0x031b32f0, JSObject * proto=0x054f9000)  Line 3072 + 0x13 bytes	C++
028bc654()	
mozjs.dll!js::ExecuteTrace(JSContext * cx=0x031b32f0, nanojit::Fragment * f=0x095ee3b8, js::InterpState & state={...})  Line 6538 + 0x6 bytes	C++
mozjs.dll!js::ExecuteTree(JSContext * cx=0x031b32f0, js::TreeFragment * f=0x095ee3b8, unsigned int & inlineCallCount=0, js::VMSideExit * * innermostNestedGuardp=0x0034c660)  Line 6634 + 0x11 bytes	C++
mozjs.dll!js::MonitorLoopEdge(JSContext * cx=0x031b32f0, unsigned int & inlineCallCount=0, js::RecordReason reason=Record_Branch)  Line 7128 + 0x15 bytes	C++
mozjs.dll!js_Interpret(JSContext * cx=0x031b32f0)  Line 923 + 0x671 bytes	C++
mozjs.dll!js_Invoke(JSContext * cx=0x031b32f0, unsigned int argc=1, int * vp=0x06890688, unsigned int flags=0)  Line 1396 + 0x9 bytes	C++
xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x06425e90, unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x03bb3030, nsXPTCMiniVariant * nativeParams=0x0034d154)  Line 1696 + 0x1b bytes	C++
xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=3, const XPTMethodDescriptor * info=0x03bb3030, nsXPTCMiniVariant * params=0x0034d154)  Line 571	C++
xul.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0643ca38, unsigned int methodIndex=3, unsigned int * args=0x0034d214, unsigned int * stackBytesToPop=0x0034d204)  Line 114 + 0x21 bytes	C++
xul.dll!SharedStub()  Line 142	C++
xul.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x0454c210, nsIDOMEventListener * aListener=0x0643ca38, nsIDOMEvent * aDOMEvent=0x0bd621e8, nsPIDOMEventTarget * aCurrentTarget=0x06171ae8, unsigned int aPhaseFlags=6, nsCxPusher * aPusher=0x0034d544)  Line 1056 + 0x12 bytes	C++
xul.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x063602a0, nsEvent * aEvent=0x0b985560, nsIDOMEvent * * aDOMEvent=0x0034d534, nsPIDOMEventTarget * aCurrentTarget=0x06171ae8, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0034d538, nsCxPusher * aPusher=0x0034d544)  Line 1174	C++
xul.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, int aMayHaveNewListenerManagers=0, nsCxPusher * aPusher=0x0034d544)  Line 201	C++
xul.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000, int aMayHaveNewListenerManagers=0, nsCxPusher * aPusher=0x0034d544)  Line 327	C++
xul.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x06a75c88, nsPresContext * aPresContext=0x063602a0, nsEvent * aEvent=0x0b985560, nsIDOMEvent * aDOMEvent=0x0bd621e8, nsEventStatus * aEventStatus=0x00000000, nsDispatchingCallback * aCallback=0x00000000, nsCOMArray<nsPIDOMEventTarget> * aTargets=0x00000000)  Line 600 + 0x1e bytes	C++
xul.dll!nsEventDispatcher::DispatchDOMEvent(nsISupports * aTarget=0x06a75c88, nsEvent * aEvent=0x00000000, nsIDOMEvent * aDOMEvent=0x0bd621e8, nsPresContext * aPresContext=0x063602a0, nsEventStatus * aEventStatus=0x00000000)  Line 663 + 0x1d bytes	C++
xul.dll!nsPresContext::FireDOMPaintEvent()  Line 2043 + 0x1f bytes	C++
xul.dll!nsRunnableMethod<nsPresContext,void>::Run()  Line 283	C++
xul.dll!nsThread::ProcessNextEvent(int mayWait=0, int * result=0x0034d6d4)  Line 527 + 0x19 bytes	C++
xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x005d11e8, int mayWait=0)  Line 250 + 0x16 bytes	C++
xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x005d64a8)  Line 118 + 0xe bytes	C++
xul.dll!MessageLoop::RunInternal()  Line 212	C++
xul.dll!MessageLoop::RunHandler()  Line 195	C++
xul.dll!MessageLoop::Run()  Line 169	C++
xul.dll!nsBaseAppShell::Run()  Line 180	C++
xul.dll!nsAppShell::Run()  Line 240	C++
xul.dll!nsAppStartup::Run()  Line 183 + 0x1c bytes	C++
xul.dll!XRE_main(int argc=5, char * * argv=0x0070a868, const nsXREAppData * aAppData=0x005ccc48)  Line 3476 + 0x25 bytes	C++
firefox.exe!NS_internal_main(int argc=5, char * * argv=0x0070a868)  Line 158 + 0x12 bytes	C++
firefox.exe!wmain(int argc=5, wchar_t * * argv=0x005c7010)  Line 120 + 0xd bytes	C++
firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
firefox.exe!wmainCRTStartup()  Line 403	C
kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

The other threads look pretty healthy and normal.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-02-02 20:11:51 PST
*** Bug 543903 has been marked as a duplicate of this bug. ***
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-02-02 20:41:30 PST
I can repro this easily on http://technet.microsoft.com/en-us/sysinternals/default.aspx.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-02-02 21:43:41 PST
David, I'm curious why you changed the component to Plug-ins ... when I repro the crash on the page above, no plugins have been loaded (and so Ben's new code from afff5c13d296 hasn't run).
Comment 4 Igor Bukanov 2010-02-03 06:29:59 PST
This looks like a regression from the bug 538275. I will check this later today.
Comment 5 Benjamin Smedberg [:bsmedberg] 2010-02-03 09:40:55 PST
Is this just a bad assert, or something that will cause problems for the alpha in release mode?
Comment 6 Igor Bukanov 2010-02-03 10:13:12 PST
This is indeed a regression from the bug 538275. I was too zealous optimizing js_DropAllEmptyScopeLocks.

> Is this just a bad assert, or something that will cause problems for the alpha
> in release mode?

Unfortunately this will cause problems.
Comment 7 David Mandelin [:dmandelin] 2010-02-03 10:16:57 PST
(In reply to comment #3)
> David, I'm curious why you changed the component to Plug-ins ... when I repro
> the crash on the page above, no plugins have been loaded (and so Ben's new code
> from afff5c13d296 hasn't run).

I misread comment 0. Usually when I see a changeset block like that, it is a regression point, so I read it that way. But now I see it is not. Sorry about that.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-02-03 10:28:46 PST
(In reply to comment #7)
> (In reply to comment #3)
> > David, I'm curious why you changed the component to Plug-ins ... when I repro
> > the crash on the page above, no plugins have been loaded (and so Ben's new code
> > from afff5c13d296 hasn't run).
> 
> I misread comment 0. Usually when I see a changeset block like that, it is a
> regression point, so I read it that way. But now I see it is not. Sorry about
> that.

No problem at all, I hope my tone didn't come across as accusatory; I was curious if you knew something that I didn't ;).
Comment 9 Igor Bukanov 2010-02-03 13:50:41 PST
The changes for the bug 538275 assumed that js_GetMutableScope is always called with the object locked. But this is not the case for jit-only js_NonEmptyObject. The function assumes that that it can skip JS_LOCK_OBJ. But I do not see how it can do it. The empty scopes stored in the prototype can become thread-shared with the prototype itself still been single-threaded. This is happens, for example, when an empty object is shared across threads. The jit would not be aware of this leading to unbalanced unlock.

brendan, jorendorff, could you comment on this?
Comment 10 Brendan Eich [:brendan] 2010-02-03 14:16:29 PST
(In reply to comment #9)
> The changes for the bug 538275 assumed that js_GetMutableScope is always called
> with the object locked. But this is not the case for jit-only
> js_NonEmptyObject.

Oops, what comes to mind. Sorry about that, please fix by locking.

/be
Comment 11 Igor Bukanov 2010-02-03 15:07:50 PST
Created attachment 425086 [details] [diff] [review]
fix v1

Fix adds the missing lock/unlock code to js_NonEmptyObject. The patch also includes the test case for the bug that uses evalcx to stay closer to the reported stack traces. Using scatter is little bit involved AFAICS to expose the bug.
Comment 12 Jason Orendorff [:jorendorff] 2010-02-03 15:46:14 PST
Comment on attachment 425086 [details] [diff] [review]
fix v1

OK, but I think we can skip the UNLOCK here since we cannot be here if there is a debugger object hook (that inhibits the JIT).

You can assert that the new mutable scope still has ownercx==cx and that there is no debugger object hook.

Ultimately we do not want to call js_NewObjectWithClassProto here, since that calls JSObject::initSharingEmptyScope, which is the opposite of what we want. Ideally we could skip the LOCK as well as the UNLOCK. Filed follow-up bug 544126 for that.
Comment 13 David Baron :dbaron: ⌚️UTC-10 2010-02-04 17:16:59 PST
Can we get this on to mozilla-central before the alpha freeze in 6 hours and 44 minutes?  (It's marked as an alpha1 blocker.)
Comment 15 David Baron :dbaron: ⌚️UTC-10 2010-02-05 00:09:28 PST
Is this safe to transplant to mozilla-central without the rest of what's on tracemonkey?
Comment 16 David Baron :dbaron: ⌚️UTC-10 2010-02-05 00:26:55 PST
If it is, please feel free to do so.  I closed the tree for the freeze, but this can land.
Comment 18 Igor Bukanov 2010-02-05 08:17:52 PST
Followup to fix the added test. I forgot to check if evalcx is available that lead to jsreftest range:

https://hg.mozilla.org/tracemonkey/rev/e643badaabc2
Comment 19 David Baron :dbaron: ⌚️UTC-10 2010-02-05 09:29:08 PST
I landed the followup on m-c as well:
http://hg.mozilla.org/mozilla-central/rev/15e35edb57b8
Comment 20 Brendan Eich [:brendan] 2010-02-05 10:04:25 PST
(In reply to comment #19)
> I landed the followup on m-c as well:
> http://hg.mozilla.org/mozilla-central/rev/15e35edb57b8

Dissenting left-brace style for the "then" (but not the else) block in that test code :-/.

/be

Note You need to log in before you can comment on or make changes to this bug.