Timothy Nikkel (:tnikkel) Assignee
	Description • 14 years ago

When running /toolkit/content/tests/widgets/test_popup_coords.xul with 130078 nsEventStateManager::DispatchMouseEvent gets called and sets mCurrentTargetContent to something. nsEventStateManager::GetEventTarget gets called and fills in mCurrentTarget with the frame for mCurrentTargetContent. At the end of nsEventStateManager::DispatchMouseEvent we null out mCurrentTargetContent but its frame is still in mCurrentTarget. Then the test checks a popup shown event's rangeParent and rangeOffset -- nsDOMUIEvent::GetRangeParent/Offset gets the current event target and returns info based on that when the current target should be null for this event.

The other two changes come from just looking at the assignments to mCurrentTarget/Content where it looks like we could have a similar problem.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 14 years ago

Comment on attachment 466559 [details] [diff] [review]
patch

Could this cause some problems with :hover
I wonder if SetContentState(targetContent, NS_EVENT_STATE_HOVER);
could be moved to happen under PreHandleEvent, not under PostHandleEvent
(in that one case it is still in PostHandleEvent).

	Timothy Nikkel (:tnikkel) Assignee
	Comment 2 • 14 years ago

I don't think I know this code well enough to answer your questions with any sort of confidence or expedience. Would it be easier if you produced a patch doing what you are suggesting? I can test that it fixes the specific problem I'm encountering of course.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 14 years ago

OK, I'll try to look at this tomorrow.
Remind me if I don't.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 14 years ago

Comment on attachment 466559 [details] [diff] [review]
patch

Actually, I think this should be ok.

Only NS_MOUSE_ENTER is handled in PostHandleEvent (and only there), not
NS_MOUSE_ENTER_SYNTH.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 14 years ago

Uh, wait, how does uievent.getRangeParent work with mouse move events if this
change is done.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 14 years ago

Especially the mouse move event which happens right after mouseover event.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 14 years ago

Would it be enough to clear both event target at the end of PostHandleEvent?
That would be a lot safer.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 8 • 14 years ago

I actually landed this last night, but test results took forever, so I didn't get a chance to mark that here before going to sleep. Should I back it out or just leave it in until we figure out the correct fix here?

Just clearing both event targets at the end of PostHandleEvent is not enough to fix the problem with test_popup_coords.xul.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 14 years ago

Ouh. Sorry! I didn't figure out the possible problem early enough.
I think you should back out, unless you have tested that
nothing has changed in mousemove+rangeparent handling.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 10 • 14 years ago

Landed
http://hg.mozilla.org/mozilla-central/rev/66dcf25705f9

and back out
http://hg.mozilla.org/mozilla-central/rev/958a30df30dd
http://hg.mozilla.org/mozilla-central/rev/2a0e5811ece9

	Timothy Nikkel (:tnikkel) Assignee
	Comment 11 • 14 years ago

The problematic DispatchMouseEvent call happens when NotifyMouseOut calls NotifyMouseOut on a child ESM. The child ESM isn't handling an event so there should be no danger in clearing the event target in that case.

This is an uglier patch; is there a better way of limiting this to only happen when the ESM isn't currently handling an event?

	Timothy Nikkel (:tnikkel) Assignee
	Comment 12 • 14 years ago

I think this is better.

Clear mCurrentTarget and mCurrentTargetContent at the end of PostHandleEvent. In DispatchMouseEvent save the mCurrentTarget before dispatching the event, and restore it after.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 14 years ago

Comment on attachment 468091 [details] [diff] [review]
save and restore the target frame

>+  nsIFrame* previousTarget = mCurrentTarget;
>+
>   mCurrentTargetContent = aTargetContent;
> 
>   nsIFrame* targetFrame = nsnull;
>   if (aTargetContent) {
>     nsESMEventCB callback(aTargetContent);
>     nsEventDispatcher::Dispatch(aTargetContent, mPresContext, &event, nsnull,
>                                 &status, &callback);
> 
>@@ -3624,16 +3627,17 @@ nsEventStateManager::DispatchMouseEvent(
>     // it may not be the same object after event dispatching and handling.
>     // So we need to refetch it.
>     if (mPresContext) {
>       targetFrame = mPresContext->GetPrimaryFrameFor(aTargetContent);
>     }
>   }
> 
>   mCurrentTargetContent = nsnull;
>+  mCurrentTarget = previousTarget;
previousTarget may point to a deleted object here.
If the type was nsWeakFrame, this could work.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 14 • 14 years ago

(In reply to comment #13)
> previousTarget may point to a deleted object here.
> If the type was nsWeakFrame, this could work.

Hmm, I thought that a weak frame was cleared out when its frame was deleted. The code seems to indicate this. Am I missing something?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 14 years ago

previousTarget is not an nsWeakFrame.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 14 years ago

... but nsIFrame*. And if you call anything which might delete it, like
you do here by dispatching an event, it may point to a deleted object.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 17 • 14 years ago

Oh yes, of course. Thanks.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 18 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/8ac6b17eab3e