1477640 - AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) does not care dom::Event::mEvent

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) just grabs instance of aEvent as is. However, if it were created with Widget*Event allocated in stack, it'll keep referring outdated address.  Fortunately, there is no such bug in current code. However, I think that we should make the constructor:

* call dom::Event::DuplicatePrivateData()

or

* insert MOZ_ASSERT to check dom::Event::mEventIsInternal

How do you think, smaug?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, the code is here:
https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/events/AsyncEventDispatcher.h#97-103

Comment 2

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

I think I'd prefer the assert. AsyncEventDispatcher isn't supposed to be used with stack allocated stuff.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b533081bc3b028ec68c2babb0a961f4e20ab64fc

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1477640 - Make AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) crash in debug build if aEvent is not safe to be dispatched asynchronously

AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) just stores
aEvent with RefPtr.  So, this constructor assumes that aEvent->mEvent is always
allocated by dom::Event or its subclasses.  For avoiding some callers use
dom::Event instances which are created with Widget*Event or Internal*Event
in the stack and its DuplicatePrivateData() has never been called, this patch
add MOZ_ASSERT to check if aEvent->mEventIsInternal is true.

Review commit: https://reviewboard.mozilla.org/r/258978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258978/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b1d69fac25bc5a61eafc61875ca6bdfd0948943

Comment 6

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

Comment on attachment 8994423 [details]
Bug 1477640 - Make AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) crash in debug build if aEvent is not safe to be dispatched asynchronously

https://reviewboard.mozilla.org/r/258978/#review266020

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2a5ae30413b
Make AsyncEventDispatcher(dom::EventTarget* aTarget, dom::Event* aEvent) crash in debug build if aEvent is not safe to be dispatched asynchronously r=smaug

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f2a5ae30413b