82051 - dangling pointer in nsJSEventListener causes crash

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[vidur (gone)]

Johnny's checkin to implement nsIEventTarget for content nodes as a tear-off
introduced a crash. Script event handlers (implemented in
dom/src/events/nsJSEventListener.cpp) keep a weak reference to their target
object. The tear-offs were getting passed as the target object to the handler
wrappers, resulting in a dangling reference.

Specifically, see nsJSEventListener:35:

nsJSEventListener::nsJSEventListener(nsIScriptContext *aContext, 
                                     nsISupports *aObject) 
{
  NS_INIT_REFCNT();

  // Both of these are weak references. We are guaranteed
  // because of the ownership model that this object will be
  // freed (and the references dropped) before either the context
  // or the owner goes away.
  mContext = aContext;
  mObject = aObject;

Comment 1

[vidur (gone)]

The following patch helps fix the problem. The patch ensures that we pass the
actual receiver and not the tear-off as the target object.

Index: nsDOMClassInfo.cpp
===================================================================
RCS file: /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v
retrieving revision 1.22
diff -u -r1.22 nsDOMClassInfo.cpp
--- nsDOMClassInfo.cpp  2001/05/21 12:37:38     1.22
+++ nsDOMClassInfo.cpp  2001/05/22 02:18:46
@@ -1930,10 +1930,10 @@
   NS_ENSURE_TRUE(atom, NS_ERROR_OUT_OF_MEMORY);

   if (compile) {
-    rv = manager->CompileScriptEventListener(script_cx, receiver, atom,
+    rv = manager->CompileScriptEventListener(script_cx, native, atom,
                                              did_compile);
   } else {
-    rv = manager->RegisterScriptEventListener(script_cx, receiver, atom);
+    rv = manager->RegisterScriptEventListener(script_cx, native, atom);
   }

   return rv;

Comment 2

[Johnny Stenback (:jst)]

Looks good to me, I lxr'ed around for a while and I think this is the only case
where we'd need changes like these. r/sr=jst

Comment 3

[vidur (gone)]

For PDT: This bug will be triggered by a JavaScript garbage collection cycle
(opening a new window is one of many trigger points) on any page that has a
JavaScript event handler. It's a regression that was introduced on Friday last
week and should be fixed ASAP.

Comment 4

[Peter Van der Beken [:peterv]]

r=peterv

Comment 5

[Johnny Stenback (:jst)]

Thanks for the fix and the reviews, fix checked in.

Comment 6

[Vladimir Ermakov]

verifying per developer's coments