304249 - Assertion failure: scope->ownercx == cx, at .../jslock.c:1254

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tuukka Tolvanen (sp3000)]

crashes every few days on this.

Assertion failure: scope->ownercx == cx, at
/home/tt/src/mozilla/HEAD/mozilla/js/src/jslock.c:1254

Comment 1

[Tuukka Tolvanen (sp3000)]

Attachment: output + bt

tweaking a textarea somewhere on http://www.okcupid.com/tests (not
reproducible) build firefox-debug_2005-08-08-06Z

Comment 2

[Tuukka Tolvanen (sp3000)]

Attachment: output + bt + jsstack

same build, navigating somewheres

Comment 3

[Brendan Eich [:brendan]]

In js_DefineNativeProperty, what is *clasp?

These stacks show (a) clasp->addProperty not a stub; (b) clasp->addProperty
failing; (c) after failure, scope is unlocked or single-thread-locked by another
context -- suggesting strongly that the failing addProperty wrongly unlocked its
obj param (which would unlock scope).

This should be investigated and fixed pronto.

/be

Comment 4

[Brendan Eich [:brendan]]

jst, why would defining "onselect" or "onload" run into an addProperty failure,
or should I ask: which nsDOMClassInfo.cpp scriptable helper hook might be to blame?

More confusion: even if something in an addProperty helper "unlocked" obj, if
obj is single-threaded, scope->ownercx would remain unchanged.  For it to
change, obj would have to be multi-threaded, and contended for by more than one
thread (at some point in its life).

Reporter: please list all extensions installed.

/be

Comment 5

[Tuukka Tolvanen (sp3000)]

> In js_DefineNativeProperty, what is *clasp?

just killed the process, sorry :\

> Reporter: please list all extensions installed.

domi, reporter, livehttpheaders

Comment 6

[Tuukka Tolvanen (sp3000)]

> > Reporter: please list all extensions installed.
> domi, reporter, livehttpheaders
...and magpie(disabled), downthemall(disabled)

Comment 7

[Brendan Eich [:brendan]]

Oh, and what is *scope?  All these for next time, unless you dumped *scope in an
attachment and I missed it.

/be

Comment 8

[Johnny Stenback (:jst)]

*** Bug 303809 has been marked as a duplicate of this bug. ***

Comment 9

[Mats Palmgren (inactive)]

Attachment: Another stack

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase

Warning this testcase opens many windows in a rather uncontrolled manner.

STEPS TO REPRODUCE
1. load the testcase, this should give you two windows, one with the testcase
   itself and one with Google in it. Note that it's the Google page that has
   the "onload" that focus its text input.
2. if you give focus back to the testcase window by clicking on the window bar
   there will be a new window spawned, do this a few times...
3. close some windows

repeat 2 and 3 for a while - I can get it to crash in less than 30 seconds.

Comment 11

[Mike Schroepfer]

Brendan can you take a look?

Comment 12

[Johnny Stenback (:jst)]

So what I found in my investigation for this is that we're executing JS code on
a chrome context and we're dealing with a scope owned by the current page's
context. This was triggerd by clicking a bookmarklet, so not the exact testcase
in this bug. But the problem seems likely to be in
nsEventReceiverSH::RegisterCompileHandler() since it makes a call to
nsJSUtils::GetStaticScriptContext() to find the context to run event handlers
in... That's about all I know so far...

Comment 13

[Brendan Eich [:brendan]]

jst's getting close, he updated me on IRC:

<jst> brendan: this problem was introduced by the second hunk in
https://bugzilla.mozilla.org/attachment.cgi?id=191095&action=diff#mozilla/content/events/src/nsEventListenerManager.cpp_sec7
<jst> brendan: we're executing code on the page that does textarea.select()
<jst> that ends up firing DOM events...
<jst> which are captured by our chrome code
<jst> our chrome code ends up having to compile an event handler...
<jst> and in compiling the event handler we do a JS_DefineProperty()
<jst> when caps does the security check
<jst> the code it finds is the code that called textarea.select()
<jst> uses that as the subject principal
<brendan> we should push the script_cx on the XPCThreadJSContextStack, eh?
<jst> yeah, something like that...
<jst> need to crash tho
<brendan> ok
<jst> I'll dig more tomorrow
<brendan> want to take the bug?
<jst> feel free to give it to me, if not I'll take it tomorrow

/be

Comment 14

[Johnny Stenback (:jst)]

Attachment: Make nsJSContext::BindCompiledEventHandler() push cx onto the context stack

Here's what I believe is the right fix for this problem. We're compiling a
chrome event handler in code that's being triggered from content code
(textarea.select()), and as we do that, we need the compilation to occure w/o
us thinking we're called directly from content code.

Comment 15

[Johnny Stenback (:jst)]

While this is not exactly *dependent* on the split window work, this was
introduced by fixing a bug in the event listener manager code when the split
window work landed. The change in question was to make us do the security check
in nsEventListenerManager::RegisterScriptEventListener() all the time, not just
the first time we get there :)

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193061 [details] [diff] [review]
Make nsJSContext::BindCompiledEventHandler() push cx onto the context stack

r=mrbkap

Comment 17

[Johnny Stenback (:jst)]

*** Bug 303809 has been marked as a duplicate of this bug. ***

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 193061 [details] [diff] [review]
Make nsJSContext::BindCompiledEventHandler() push cx onto the context stack

Thanks, this looks good to me -- there shouldn't be other calls into the JS
engine (thence back into dom and caps code) outside of this section, that also
need mContext pushed.

/be

Comment 19

[Johnny Stenback (:jst)]

Fixed on trunk and branch.