339774 - nsDOMEvent::DuplicatePrivateData should copy more things from the original event

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently for example keyCode and charCode are set to 0, not copied from the
original event.
This is not needed once nsEvent is gone (and this may take still some time), 
but before that I'd like it to work properly.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ugly, but still proposed patch

This is a bit ugly, but so is everything related to ::DuplicatePrivateData.
I decided to copy all possible members of nsEvent-classes. Members which
refer to some external object are set to nsnull (by the constructors of the 
nsEvent classes). nsTextEvent is a bit different, it really can't be copied,
and nsPopupBlockedEvent is used always only with DOM Events (otherwise the 
handling of the two owning pointers wouldn't really work)
Because of Bug 339697 this is needed also in branch.
The branch patch will be a bit different, mainly just the handling of the 
target of the event.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And I removed that invalid comment about trusted events.
Event will be trusted after duplication, but not perhaps after Init()

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 223963 [details] [diff] [review]
ugly, but still proposed patch

r+sr=jst

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Checking in nsDOMEvent.cpp;
/cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v  <--  nsDOMEvent.cpp
new revision: 1.210; previous revision: 1.209
done

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

prometheus didn't like this

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

It just needed curly brackets.
Checking in nsDOMEvent.cpp;
/cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v  <--  nsDOMEvent.cpp
new revision: 1.212; previous revision: 1.211
done

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Balsa showed some leaks.
I checked in the following to see whether it helps
-      NS_STATIC_CAST(nsXULCommandEvent*, newEvent)->sourceEvent =
-        NS_STATIC_CAST(nsXULCommandEvent*, mEvent)->sourceEvent;
+      /* Disabling for now. This creates some leaks.
+       NS_STATIC_CAST(nsXULCommandEvent*, newEvent)->sourceEvent =
+         NS_STATIC_CAST(nsXULCommandEvent*, mEvent)->sourceEvent; */
At least locally I did see that with that code nsDOMXULCommandEvents
were leaked, which means also their targets were leaked.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er... That sounds like we leak nsEvent structs.  Could we add some CTOR/DTOR logging to nsEvent and see what balsa shows?  I can't believe we don't have any such logging now...

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I commented out the code related to mutation events.
That helps too (at least locally). Otherwise nsDOMAttributes are leaked.
still investigating.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fixes leaks

This re-enables proper DuplicatePrivateData for mutation and command events and
fixes the leaks.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224419 [details] [diff] [review]
fixes leaks

Looks good, I guess.  When we move away to combining nsEvent and nsDOMEvent, this fragility will hopefully go away...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 224419 [details] [diff] [review]
fixes leaks

Checked in. Leaks ok.