412567 - Once dispatched, events cannot be redispatched

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P3)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

This causes acid3 test 30 to fail.

This was supposed to have been fixed in bug 296704, but the fix was wrong as far as I can tell (init*Event should have no effect after an event has been dispatched, and the event if thrown multiple times should be "frozen"), and it seems to have missed landing on trunk as well.


We currently hit this warning on trunk:

WARNING: NS_ENSURE_TRUE(!(aDOMEvent && (aEvent->flags & NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY))) failed: file /Users/jwalden/moz/trunk/mozilla/content/events/src/nsEventDispatcher.cpp, line 379

First, that name doesn't quite look right.  Instances of the flag on trunk:

http://mxr.mozilla.org/mozilla/search?string=stop_dispatch_immediately

The one in nsGUIEvent.h says it's for a DOM3 method on DOMEvent which we don't support, going by the following:

http://mxr.mozilla.org/mozilla/search?string=stopimmediate

Second, the flag's set in one location inside the NS_MARK_EVENT_DISPATCH_DONE macro:

http://mxr.mozilla.org/mozilla/search?string=NS_MARK_EVENT_DISPATCH_DONE

That macro's only used once, at the end of event dispatch, so it's going to prevent any event from ever being thrown a second time as I read things.


The patch here basically removes the NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY flag.  From the references above it's not actually being meaningfully used right now to prevent nested dispatch or anything, and it's preventing rethrowing events.  We pass all the dom/tests/mochitest tests with this patch.


Requesting blocking because it's acid3 and it's a trivial fix...

Comment 1

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

(In reply to comment #0)
> This was supposed to have been fixed in bug 296704, 
As far as I can see, bug 296704 forced reinitialization.

> and it
> seems to have missed landing on trunk as well.
Er, what? Note, DOM event handling in 1.9 has been rewritten.

> WARNING: NS_ENSURE_TRUE(!(aDOMEvent && (aEvent->flags &
> NS_EVENT_FLAG_STOP_DISPATCH_IMMEDIATELY))) failed: file
> /Users/jwalden/moz/trunk/mozilla/content/events/src/nsEventDispatcher.cpp, 
On purpose, to not to change the behavior from 1.8.
 
> That macro's only used once, at the end of event dispatch, so it's going to
> prevent any event from ever being thrown a second time as I read things.
Unless one reinitializes event.

> From the references above it's not actually being meaningfully used
> right now to prevent nested dispatch or anything, and it's preventing
> rethrowing events.
That is exactly what it what used for. Preventing dispatching events
without reinitialization.

The patch "nicely" regress a variation of bug 296704 ;), as far as I see 
(didn't test though).
Try first dispatching a trusted event (for example click a mouse 
button) in a content page and then while the event is being dispatched, take 
reference to it. After a timeout re-dispatch the event again. After the first
dispatch but before the second dispatch the event should be trusted, but
during the second dispatch it *must* not be trusted, because untrusted
scripts must not be able to dispatch trusted events.
I think we don't have mochitests for this must-have-functionality - we
didn't have mochitests when bug 296704 was fixed and the testcase there doesn't
even handle the timeout case, I think.

Comment 2

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

And when re-dispatching trusted event in chrome, it should stay trusted.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #1)
> (In reply to comment #0)
> > and it seems to have missed landing on trunk as well.
> Er, what? Note, DOM event handling in 1.9 has been rewritten.

I was looking at the specific changes at the end of the init*Event events to remove the bit (one of the later patches in the bug), although come to think of it, those probably just got invisibly consolidated into the one in nsDOMEvent::InitEvent.

> > That macro's only used once, at the end of event dispatch, so it's going to
> > prevent any event from ever being thrown a second time as I read things.
> Unless one reinitializes event.

Except that the DOM Events spec is pretty clear about this in the description of initEvent:

  This method may only be called before the Event has been dispatched
  via the EventTarget.dispatchEvent() method. [...]  If the method is
  called several times before invoking EventTarget.dispatchEvent, only
  the final invocation takes precedence. This method has no effect if
  called after the event has been dispatched.

So after the first dispatch, initEvent no longer has any effect; it shouldn't be necessary to reinit in order to rethrow, and the reinitialization must either happen at the beginning or end of dispatchEvent (also in createEvent).

> That is exactly what it what used for. Preventing dispatching events
> without reinitialization.

Except that reinitialization is counter to the spec.

> The patch "nicely" regress a variation of bug 296704 ;), as far as I see 
> (didn't test though).

Sounds like I need to factor out resetting logic so that an event is reset after being dispatched, then, or something.

More to do than I thought, and I'm not really surprised; that felt too easy.  I'll drop some time on this and get some idea of scale rather than let a possibly-unuseful nomination stand.

Comment 4

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

(In reply to comment #3)
> Except that the DOM Events spec is pretty clear about this in the description
> of initEvent:
Sure, I wasn't saying that we follow the spec here. The spec doesn't know
anything about trusted events, so it doesn't need to handle the issues 
related to them.

Comment 5

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

Attachment: WIP

Jeff, I could take this bug.

Have to write mochitests for bug 296704 and this bug.

Comment 6

[Jeff Walden [:Waldo]]

Yes, please do, I don't have time.

Comment 7

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

(In reply to comment #3)
> Except that the DOM Events spec is pretty clear about this in the description
> of initEvent:
> ....
> This method has no effect if called after the event has been dispatched.
That is from DOM 3 Events, which is just a draft, so I'll ignore it for now -
and that is anyway a different bug.

Comment 8

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

Attachment: with test

When reviewing, keep in mind bug 296704.

This gives one point in ACID3.

Comment 9

[Johnny Stenback (:jst)]

Smaug, for future reference, please add -p to your diff arguments to make the diff easier to use. Try adding something like this to your ~/.hgrc file:

[defaults]
diff = -U 8 -p

Comment 10

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

(In reply to comment #9)
> Smaug, for future reference, please add -p to your diff arguments to make the
> diff easier to use. Try adding something like this to your ~/.hgrc file:
> 
> [defaults]
> diff = -U 8 -p
> 
Thanks for the info. That was first patch I created using Hg :)

Comment 11

[Damon Sicore (:damons)]

marking wanted1.9.1? to get this in the triage queue.