Once dispatched, events cannot be redispatched

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Waldo, Assigned: smaug)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

12 years ago
Posted patch PatchSplinter Review
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...
Flags: blocking1.9?
Attachment #297313 - Flags: review?(Olli.Pettay)
Assignee

Comment 1

12 years ago
(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.
Assignee

Updated

12 years ago
Attachment #297313 - Flags: review?(Olli.Pettay) → review-
Assignee

Comment 2

12 years ago
And when re-dispatching trusted event in chrome, it should stay trusted.
Reporter

Comment 3

12 years ago
(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.
Flags: blocking1.9?
Assignee

Comment 4

12 years ago
(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.
Assignee

Comment 5

11 years ago
Posted patch WIP (obsolete) — Splinter Review
Jeff, I could take this bug.

Have to write mochitests for bug 296704 and this bug.
Assignee: jwalden+bmo → Olli.Pettay
Reporter

Comment 6

11 years ago
Yes, please do, I don't have time.
Assignee

Comment 7

11 years ago
(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.
Assignee

Comment 8

11 years ago
Posted patch with testSplinter Review
When reviewing, keep in mind bug 296704.

This gives one point in ACID3.
Attachment #321079 - Attachment is obsolete: true
Attachment #321131 - Flags: superreview?(jst)
Attachment #321131 - Flags: review?(jst)
Attachment #321131 - Flags: superreview?(jst)
Attachment #321131 - Flags: superreview+
Attachment #321131 - Flags: review?(jst)
Attachment #321131 - Flags: review+
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
Assignee

Comment 10

11 years ago
(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 :)
Assignee

Updated

11 years ago
Keywords: checkin-needed
Assignee

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
marking wanted1.9.1? to get this in the triage queue.  
Flags: wanted1.9.1?
Priority: -- → P3
Assignee

Updated

11 years ago
Depends on: 447736
You need to log in before you can comment on or make changes to this bug.