81115 - event.target is bogus

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[alex.dent]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9+) Gecko/
BuildID:    2001051508

try the attached testcase. the div gets a click event and a custom variable
gumbo is set in the divs object. this variable does NOT exist in the event
target, but it does exist in event.target.nextSibling.previousSibling, which
should be the same as event.target! this shows that some evil hack is going on
in the background that creates a new, bogus node as the event.target, with most,
but not all, properties copied over from the original node.

this needs to be fixed asap, as it creates a lot of problems even in mozillas
own scripts.

Reproducible: Always

Comment 1

[alex.dent]

Attachment: testcase to click

Comment 2

[Boris Zbarsky [:bzbarsky]]

seeing this on linux too...

Comment 3

[alex.dent]

forgot to mention that not only event.target is bogus, but it is also the same
as 'this' in the event handler, ie. this.gumbo is also undefined in the test
case above.

Comment 4

[joki (gone)]

From a quick look it seems we may be making different XPCDOM wrappers for the 
content object depending on whether we see it as an nsIDOMNode or an 
nsIDOMEventTarget.  This would cause some identity problems.

Comment 5

[Fabian Guisset]

Adding myself to CC because this *might* be causing the problems with context
menus for bookmarks sidebar and personnal toolbar not working.

Comment 6

[Anatoly Kochergin]

Testcase is working well in Mozilla 0.9
Regression?

Comment 7

[Johnny Stenback (:jst)]

Right on, joki! When I read your comment it occured to me that this is probably
a problem with the nsIContent/nsIDOMEvent(Target/Listener) aggregation we do for
XML/HTML elements and some other nodes as well, and I was right, that whole
aggregation/tearoff scheme violates the one [XP]COM rule that can not be broken
in a situation like this, namely the identity rule. Whatever really happens
behind the scene when something like this is done doesn't really matter as long
as QI'ing any one of these tearoff objects (nsEventListenerManager in this case)
to the nsISuppotys interface *always* results the *same* pointer, this was not
happening. Here's a patch that fixes this, and other problems as well (at least
bug 80929).

Index: content/events/src/nsEventListenerManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/events/src/nsEventListenerManager.cpp,v
retrieving revision 1.99
diff -u -r1.99 nsEventListenerManager.cpp
--- nsEventListenerManager.cpp  2001/05/11 00:43:25     1.99
+++ nsEventListenerManager.cpp  2001/05/16 06:58:53
@@ -184,13 +184,13 @@
     AddRef();
     return NS_OK;
   }
+  if (mTarget) {
+    return mTarget->QueryInterface(aIID, aInstancePtr);
+  }
   if (aIID.Equals(NS_GET_IID(nsISupports))) {
     *aInstancePtr = (void*)(nsISupports*)(nsIEventListenerManager*)this;
     AddRef();
     return NS_OK;
-  }
-  if (mTarget) {
-    return mTarget->QueryInterface(aIID, aInstancePtr);
   }
   return NS_NOINTERFACE;
 }

Tom, I don't know how scary this is from the event listener manager point of
view, but it does fix the problem and doesn't introduce any obvious problems
that I've noticed in testing this for only a few minutes. Whaddaya think?

Comment 8

[Johnny Stenback (:jst)]

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

Comment 9

[joki (gone)]

Yeah, I can see it being a bit scary, or maybe just weird in that 
nsEventListenerManager doesn't really do nsISupports at all anymore, but given 
our aggregation scheme it seems like the correct choice.

Comment 10

[Johnny Stenback (:jst)]

Vidur, sr=?

Comment 11

[joki (gone)]

For the sake of process,

r=joki

Comment 12

[Keyser Sose]

Seeing this too hope this is fixed for 0.9.1 because it breaks a lot of menus.

Comment 13

[vidur (gone)]

Scary. nsGenericElement is the only user of the nsEventListenerManager, right?
Joki, can you confirm? If so, then we can assume in the implementation that it
is a tear-off and Johnny's patch is correct. Assuming enough testing has been
done, sr=vidur.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Proposed more correct fix.

Comment 15

[Johnny Stenback (:jst)]

The patch I just attached has been checked in, that patch creates a new tearoff
class that's used for implementing nsIDOMEventReceiver and nsIDOMEventTarget on
nsGenericElement and nsGenericDOMDataNode. This fixes this immediate problem and
also makes QI'ing a nsGenericElement not create the event listener manager and
holding on to it for the rest of the elements lifetime.

Fixed!

Comment 16

[alex.dent]

after 3 days a new mac build is finally availiable and the bug is fixed just
fine so far.

but probably related is bug 82092

Comment 17

[Vladimir Ermakov]

Verifying fixed on build 2001-05-22-06-trunk windows 98