If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

M17 crashes when loading the page with two load event listeners

VERIFIED DUPLICATE of bug 31847

Status

()

Core
Event Handling
P1
critical
VERIFIED DUPLICATE of bug 31847
17 years ago
16 years ago

People

(Reporter: Martin Honnen, Assigned: joki (gone))

Tracking

({crash, relnote})

Trunk
mozilla0.9
x86
All
crash, relnote
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-][rtm-] relnote-devel)

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
<HTML>
<HEAD>
<SCRIPT>
window.addEventListener('load',
  function (evt) {
    alert('load 1 listener');
  },
  false
);

window.addEventListener('load',
  function (evt) {
    alert('load 2 listener');
  },
  false
);

</SCRIPT>
</HEAD>
<BODY>
Kibology
</BODY>
</HTML>

The crash occurs after the first alert is closed. It doesn't crash if only one
event listener is registered.
(Reporter)

Comment 1

17 years ago
Created attachment 13366 [details]
bug demo (crashes M17)
Seeing this on commercial Linux build from yesterday. Crash is bad, nominating
for nsbeta3.
Assignee: joki → heikki
Severity: major → critical
Keywords: crash, nsbeta3
OS: other → All
Priority: P3 → P1
We crash in file jslock.c, line 523:

static int
js_SuspendThread(JSThinLock *tl)
{
    JSFatLock *fl;
    JSStatus stat;

    if (tl->fat == NULL)
        fl = tl->fat = allocateFatlock(tl);
    else
        fl = tl->fat;
    JS_ASSERT(fl->susp >= 0);
    fl->susp++;
    PR_Lock(fl->slock);  // <= line 523 CRASH! fl is not 0
    js_UnlockGlobal(tl);
    stat = (JSStatus)PR_WaitCondVar(fl->svar,PR_INTERVAL_NO_TIMEOUT);
    JS_ASSERT(stat != JS_FAILURE);
    PR_Unlock(fl->slock);
Stack trace:

NTDLL! 77f6ce0c()
NTDLL! 77f67546()
js_SuspendThread(JSThinLock * 0x02e71df4) line 523 + 13 bytes
js_Enqueue(JSThinLock * 0x02e71df4, long 11087536) line 565 + 9 bytes
js_Lock(JSThinLock * 0x02e71df4, long 11087536) line 598 + 13 bytes
js_LockScope1(JSContext * 0x03b2a2e0, JSScope * 0x02e71dd0, long 11087536) line 
647 + 13 bytes
js_LockObj(JSContext * 0x03b2a2e0, JSObject * 0x02e71268) line 717 + 17 bytes
js_GetSlotWhileLocked(JSContext * 0x03b2a2e0, JSObject * 0x02e71268, unsigned 
long 2) line 275 + 13 bytes
JS_GetPrivate(JSContext * 0x03b2a2e0, JSObject * 0x02e71268) line 1724 + 98 
bytes
nsScriptSecurityManager::GetFunctionObjectPrincipal(nsScriptSecurityManager * 
const 0x01897a80, JSContext * 0x03b2a2e0, JSObject * 0x02e71268, nsIPrincipal * 
* 0x0012ecec) line 827 + 14 bytes
nsScriptSecurityManager::CheckFunctionAccess(nsScriptSecurityManager * const 
0x01897a80, JSContext * 0x03b2a2e0, void * 0x02e71268, void * 0x02dca8b0) line 
595 + 44 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x03b2cf00, void * 0x02dca8b0, 
void * 0x02e71268, unsigned int 1, void * 0x0012ed94, int * 0x0012ed90, int 0) 
line 850 + 38 bytes
nsJSDOMEventListener::HandleEvent(nsIDOMEvent * 0x046a2fb4) line 88 + 52 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x04676da0, 
nsIDOMEvent * 0x046a2fb4, nsIDOMEventTarget * 0x03b2a480, unsigned int 1, 
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x04626af0, nsEvent * 
0x0012f8d4, nsIDOMEvent * * 0x0012f1e8, nsIDOMEventTarget * 0x03b2a480, unsigned 
int 7, nsEventStatus * 0x0012f918) line 1361 + 39 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x03b2a470, 
nsIPresContext * 0x04626af0, nsEvent * 0x0012f8d4, nsIDOMEvent * * 0x0012f1e8, 
unsigned int 1, nsEventStatus * 0x0012f918) line 441
nsWebShell::OnEndDocumentLoad(nsWebShell * const 0x03b287bc, nsIDocumentLoader * 
0x03b2b2e0, nsIChannel * 0x046b8a40, unsigned int 0) line 963 + 56 bytes
nsDocLoaderImpl::FireOnEndDocumentLoad(nsDocLoaderImpl * 0x03b2b2e0, nsIChannel 
* 0x046b8a40, unsigned int 0) line 811
nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int 0) line 617
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x03b2b2e4, nsIChannel * 
0x046b8a40, nsISupports * 0x00000000, unsigned int 0, const unsigned short * 
0x100a3cc8 gCommonEmptyBuffer) line 545
nsLoadGroup::RemoveChannel(nsLoadGroup * const 0x03b2b280, nsIChannel * 
0x046b8a40, nsISupports * 0x00000000, unsigned int 0, const unsigned short * 
0x100a3cc8 gCommonEmptyBuffer) line 566 + 39 bytes
nsHTTPChannel::ResponseCompleted(nsIStreamListener * 0x0404e520, unsigned int 0, 
const unsigned short * 0x100a3cc8 gCommonEmptyBuffer) line 1844
nsHTTPServerListener::OnStopRequest(nsHTTPServerListener * const 0x043fa9f0, 
nsIChannel * 0x03c362e4, nsISupports * 0x046b8a40, unsigned int 0, const 
unsigned short * 0x100a3cc8 gCommonEmptyBuffer) line 726
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x046a5fc0) line 
302
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x046a5f70) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x046a5f70) line 587 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00b41260) line 528 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00540ad8, unsigned int 49427, unsigned int 0, 
long 11801184) line 1043 + 9 bytes
USER32! 77e71820()
00b41260()
Hmm... if I load the test case as command line argument, I get a different stack 
trace:

JS_GetPrivate(JSContext * 0x03a9b9f0, JSObject * 0x02e218d0) line 1724 + 10 
bytes
nsScriptSecurityManager::GetFunctionObjectPrincipal(nsScriptSecurityManager * 
const 0x01893f30, JSContext * 0x03a9b9f0, JSObject * 0x02e218d0, nsIPrincipal * 
* 0x0012ecec) line 827 + 14 bytes
nsScriptSecurityManager::CheckFunctionAccess(nsScriptSecurityManager * const 
0x01893f30, JSContext * 0x03a9b9f0, void * 0x02e218d0, void * 0x02e217c0) line 
595 + 44 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x03a9f870, void * 0x02e217c0, 
void * 0x02e218d0, unsigned int 1, void * 0x0012ed94, int * 0x0012ed90, int 0) 
line 850 + 38 bytes
nsJSDOMEventListener::HandleEvent(nsIDOMEvent * 0x03b3a9e4) line 88 + 52 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03c5da80, 
nsIDOMEvent * 0x03b3a9e4, nsIDOMEventTarget * 0x03a9f8e0, unsigned int 1, 
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x03c33a30, nsEvent * 
0x0012f8d4, nsIDOMEvent * * 0x0012f1e8, nsIDOMEventTarget * 0x03a9f8e0, unsigned 
int 7, nsEventStatus * 0x0012f918) line 1361 + 39 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x03a9f8d0, 
nsIPresContext * 0x03c33a30, nsEvent * 0x0012f8d4, nsIDOMEvent * * 0x0012f1e8, 
unsigned int 1, nsEventStatus * 0x0012f918) line 441
nsWebShell::OnEndDocumentLoad(nsWebShell * const 0x03aa175c, nsIDocumentLoader * 
0x03aa52e0, nsIChannel * 0x03c39d60, unsigned int 0) line 963 + 56 bytes
nsDocLoaderImpl::FireOnEndDocumentLoad(nsDocLoaderImpl * 0x03aa52e0, nsIChannel 
* 0x03c39d60, unsigned int 0) line 811
nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int 0) line 617
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x03aa52e4, nsIChannel * 
0x03c39d60, nsISupports * 0x00000000, unsigned int 0, const unsigned short * 
0x100a3cc8 gCommonEmptyBuffer) line 545
nsLoadGroup::RemoveChannel(nsLoadGroup * const 0x03aa5280, nsIChannel * 
0x03c39d60, nsISupports * 0x00000000, unsigned int 0, const unsigned short * 
0x100a3cc8 gCommonEmptyBuffer) line 566 + 39 bytes
nsHTTPChannel::ResponseCompleted(nsIStreamListener * 0x02f09d48, unsigned int 0, 
const unsigned short * 0x100a3cc8 gCommonEmptyBuffer) line 1844
nsHTTPServerListener::OnStopRequest(nsHTTPServerListener * const 0x031fd1d0, 
nsIChannel * 0x02d41bc4, nsISupports * 0x03c39d60, unsigned int 0, const 
unsigned short * 0x100a3cc8 gCommonEmptyBuffer) line 726
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x03b38b40) line 
302
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x03b38eb0) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x03b38eb0) line 587 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00b41260) line 528 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x008b0b12, unsigned int 49427, unsigned int 0, 
long 11801184) line 1043 + 9 bytes
USER32! 77e71820()
00b41260()

The offending function is:

JS_PUBLIC_API(void *)
JS_GetPrivate(JSContext *cx, JSObject *obj)
{
    jsval v;

    CHECK_REQUEST(cx);
    JS_ASSERT(OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE); // <= 1725
    v = OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE);
    if (!JSVAL_IS_INT(v))
	return NULL;
    return JSVAL_TO_PRIVATE(v);
}
So this basically looks like we are getting a bogus pointer somewhere. Depending 
a bit on the pointer we crash and burn in different places (the first time the 
OS notices we are doing something illegal). Somewhere between JS_GetPrivate and 
js_SuspendThread anyway. The start of the stack trace seems to be the same.
Triaging: nsbeta3+. Will need to see if this is a special case crash (two event 
listeners for the same object listening the same event). Still, the bottom line 
is we shouldn't crash so at least we should discard adding the second listener 
or something...
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3+]
Created attachment 13736 [details]
Testcase: Add 2 event listener functions, works
As the testcase that works shows, we have a simple workaround: move the function 
definitions outside of the call to addEventListener. An example:

function foo(evt) {
  alert('load 1 listener');
}

window.addEventListener('load',foo,false);
Created attachment 13768 [details]
Refined crashing testcase
This seems to be a tricky bug... The refined test case shows that ANY element 
with 2 event listeners registered for the same event (any event type) causes 
crash when the second event handler is being triggered.

The test case has a button that you use to attach two select listeners on the 
input text field. We do not yet crash at this point. When you select some text, 
the select event gets fired and we crash when the second event listener is being 
called.

While testing this I noticed that we will NOT crash if we add the event 
listeners "slowly", for example if your computer is being really slow, or if you 
set a breakpoint so that you will stop in debugger between each call to 
addEventListener.
Tom, Chris, I added you to Cc because this bug seems to be evading my tactics of 
figuring out what is wrong... If you have any ideas what I should be looking at 
I'd be happy to hear it. See my comments 8/30 (today).

Comment 13

17 years ago
PDT agrees P1

Comment 14

17 years ago
changing qa contact to ckritzer
QA Contact: janc → ckritzer

Comment 15

17 years ago
Just tested Martins testcase using a 83108 build on WinNT.

The first time the page loads, there is no crash, but only one alert() 
displays. When I hit reload, the first alert() still displays, but then the 
browser crashes.

The same behaviour is seen when I replace the first addEventListener with 
a "window.onload = ..." assignment statement.

I guess I'm also seeing the "slow" behaviour.

(Totally offtopic, but IE5 also has similar bugs when you assign nested 
functions to handlers.)
The correct behaviour for named functions is to discard the second addition, and 
we are doing that. 

In this case we have anonymous functions. One could argue that the second 
anonymous function should be discarded, and this is probably what is happening 
in the "slow" case. Oh, it seems the first load is "slow" with me as well. The 
crashes occur once I have done reload.

One might also argue that if we have anonymous functions we should check their 
internals to see if they are identical. Or we could also choose the perhaps easy 
way of allowing any number of anonymous functions, without any consideration to 
whether they are identical or not (from computer perspective they are always 
different in our current code).

It might be that the second anonymous function is not rooted properly and is 
garbage collected at a wrong time. Vidur suggested this, but it feels kind of 
weird if this is the case since I would suspect we'd crash in the "slow" case 
then (GC has time to run). We are also running GC way too often, and there are 
bugs to reduce the numbers.

Comment 17

17 years ago
I think that different anonymous functions should be treated as different 
functions.

That raises the question : how would you remove (one or more) anonymous event 
listeners? In the testcase provided by Martin, the code doesn't have references 
anymore to the function objects.

Is this a whole in DOM2 or am I missing something?

Comment 18

17 years ago
Ack. Read "hole" for "whole" in the last comment. It's getting late.

Comment 19

17 years ago
Since we are 1 week's worth of work away from the beta 3 freeze and this is an 
edge case, marking beta3- and futuring.
Whiteboard: [nsbeta3+] → [nsbeta3-]
Target Milestone: --- → Future

Comment 20

17 years ago
Updating QA Contact.
QA Contact: ckritzer → lorca

Comment 21

17 years ago
This is not good.  Nominating for RTM.
Keywords: rtm

Comment 22

17 years ago
Marking rtm-.  proposed relnote: "DOM2 Event listners: Multiple anonymous event
listeners on a single event will cause a crash. The workaround is to name your
event listeners. #49980."
Keywords: relnote
Whiteboard: [nsbeta3-] → [nsbeta3-][rtm-]

Updated

17 years ago
Whiteboard: [nsbeta3-][rtm-] → [nsbeta3-][rtm-] relnote-devel
Sending most of my events bugs to joki.
Assignee: heikki → joki
Status: ASSIGNED → NEW
Changing crasher bug milestone to mozilla0.9.
Target Milestone: Future → mozilla0.9

Comment 25

17 years ago
REMOVING nsbeta2/3 and replacing with nsbeta1 KW.  Also clearing 

Status Whiteboard of nsbeta2/3 result.
Keywords: nsbeta3 → nsbeta1
Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok

Comment 27

17 years ago
Is this bug still current? If so, please update the summary, status whiteboard
and keywords fields as appropriate.
(Assignee)

Comment 28

17 years ago
The crash part of this is a dupe of 31847 but I'm adding my comments on some of 
the other points raised before I dupe it.

I think that two anonymous functions should be treated as two separate event 
listeners.  Since two named functions could have equivalent function content and 
still both be registered I don't think this should be different.

As far as removing anonymous listeners I'd say you just can't do it.  If you 
choose to use anonymous functions then you have to let those functions get 
cleaned up along with the page.  Is this a hole?  I don't think so.  The DOM 
spec is language independent.  It doesn't know or care about anonymous 
functions, something which many other languages don't have anyway.  I'd say the 
equivalent would be registering a interface based listener then nulling out your 
reference to it.  If you choose to do it you live with the consequences.

*** This bug has been marked as a duplicate of 31847 ***
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → DUPLICATE

Comment 29

17 years ago
QA contact updated
QA Contact: gerardok → madhur

Comment 30

16 years ago
v. dup
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.