587944 - need to clear target frame when we clear our target content

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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.

Comment 1

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

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).

Comment 2

[Timothy Nikkel (:tnikkel)]

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.

Comment 3

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

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

Comment 4

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

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.

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

[Timothy Nikkel (:tnikkel)]

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.

Comment 9

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

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.

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: alternate patch

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?

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: save and restore the target frame

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.

Comment 13

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

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.

Comment 14

[Timothy Nikkel (:tnikkel)]

(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?

Comment 15

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

previousTarget is not an nsWeakFrame.

Comment 16

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

... 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.

Comment 17

[Timothy Nikkel (:tnikkel)]

Oh yes, of course. Thanks.

Comment 18

[Timothy Nikkel (:tnikkel)]

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