Cleanup and fix bugs in PresShell::HandleEvent

ASSIGNED
Assigned to

Status

()

Product:
Core
Component:
Event Handling
Importance:
P2
normal
Status:
ASSIGNED
Reported:
9 months ago
Modified:
6 days ago

People

(Reporter: mats, Assigned: masayuki)

Tracking

({leave-open})

Version:
Trunk
Keywords:
leave-open
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(22 attachments)

Bug 1466208 - part 1: Create stack class to handle events in PresShell
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 19: Group PresShell, nsIFrame and nsIContent with a struct
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame()
47 bytes, text/x-phabricator-request
Details | Review
(Reporter)

Description

•
9 months ago 
Follow-up from bug 1430813 comment #21:
> PresShell::HandleEvent is a horrible mess.
> Here we might reassign 'frame' to the root frame of some shell ancestor:
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#6911
> but the rest of HandleEvent quite happily continues to use 'this'
> to access members etc.
> 
> The IME block at the end for example makes the assumption that
> 'frame' belongs to 'this':
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#7268
> It even assigns mCurrentEventFrame = frame which is clearly
> a recipe for disaster (exploitable UAFs).

Although the code lives under layout/ it seems this code is mostly doing
event handling / IME, so I'm assuming DOM Events is appropriate.
Component: DOM: Events → Event Handling
 (Reporter)

Comment 1

•
6 months ago 
This bug is scary.  Can we find an owner for it please?
Flags: needinfo?(overholt)
 (Assignee)

Comment 2

•
6 months ago 
(In reply to Mats Palmgren (:mats) from comment #1)
> This bug is scary.  Can we find an owner for it please?

I'd like to work on this when I have much time. Do you think when should we fix this by?
(FYI: I'm working on redesigning editor for InputEvent.inputType and beforeinput event. Therefore, I don't have much time at least for a couple of months.)

Comment 3

•
6 months ago 
(For comment 2. I agree that Masayuki would be great for this, assuming timelines align.)
Flags: needinfo?(overholt) → needinfo?(mats)
 (Reporter)

Comment 4

•
6 months ago 
Sure, sounds good.
Flags: needinfo?(mats)
 (Assignee)

Comment 5

•
a month ago

I think that it is really risky method to change. So, we should fix this bug slowly for detecting regressions.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
status-firefox62: affected → ---
Keywords: leave-open
 (Assignee)

Comment 9

•
a month ago

Well, I was thinking that the best solution is to split each block in HandleEvent() and HandleEventInternal() to methods of PresShell. However, this approach makes too many small methods into PresShell. And badly, some of them may be designed only for specific situations and need to take caller's local variables.

PresShell is a class not only for event handling. So, for grouping event handling helpers, I think that we should create a nexted stack based class called EventHandler and HandleEvent() should use it. Then, methods can share some variables with members.

Comment 10

•
a month ago

I think I like that approach. It could lead to quite elegant code.

 (Assignee)

Comment 17

•
a month ago

Created attachment 9037501 [details]
Bug 1466208 - part 1: Create stack class to handle events in PresShell

PresShell::HandleEvent() and PresShell::HandleEventInternal() are too big.
Additionally, we have a lot of methods used only by them. So, if we'll
split those big methods, PresShell will have a lot of small methods which
are not grouped as a part of event handling. That's too bad because some
of them may depend on the calling order, etc.

So, for grouping them, PresShell should create a stack class instance to handle
each event. Then, we can store shared information in it only while we're
handling an event.

This patch creates PresShell::EventHandler and PresShell methods become
wrappers of the stack class, but this patch does not change any logic in the
code, i.e., just reorganizing existing methods.

Note that HandleEventWithTarget() and HandleEventInternal() need to take
WidgetEvent rather than WidgetGUIEvent. Additionally, some other methods
require WidgetGUIEvent to refer WidgetGUIEvent::mWidget. Therefore, this
patch does not make the new class store the event as a member.

 (Assignee)

Comment 18

•
a month ago

Created attachment 9037502 [details]
Bug 1466208 - part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub

PresShell::EventHandler::HandleEvent() is too big. That makes us difficult to
understand the flow of them. So, first of all, we should split the method to
smaller chunks. Then, we can understand what we're doing in HandleEvent() more.

This patch creates MaybeHandleEventWithAccessibleCaret() for first handling
block in HandleEvent(). Note that the following patch will clean it up.
I.e., this patch just moves the existing block into the new method.

 (Assignee)

Comment 19

•
a month ago

Created attachment 9037505 [details]
Bug 1466208 - part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style

Because of spinning out from PresShell::EventHandler::HandleEvent(), we can use
early-return style in MaybeHandleEventWithAccessibleCaret(). This patch
rewrites MaybeHandleEventWithAccessibleCaret() with the style.

 (Assignee)

Comment 20

•
a month ago

Created attachment 9037506 [details]
Bug 1466208 - part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event

PresShell::HandleEvent() treats capturing content only when received event is
related to pointing device. And it's used in 2 purposes. One is for computing
to target document of coming event. The other is for handling events using
coordinates. Therefore, if we create a helper method to retrieve it, we can
move the variable into smaller blocks.

 (Assignee)

Comment 21

•
a month ago

Created attachment 9037509 [details]
Bug 1466208 - part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event

In some cases, PresShell::EventHandler::HandleEvent() needs to call
HandleEvent() of another instance.

For retrieving the instance, we need to compute retarget document first.
This patch makes new method to retrieve it. The following patch will clean
up it.

 (Assignee)

Comment 22

•
a month ago

Created attachment 9037511 [details]
Bug 1466208 - part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument()

 (Assignee)

Comment 23

•
a month ago

Created attachment 9037512 [details]
Bug 1466208 - part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance

Next, we need to look for a frame for first parameter of calling
PresShell::HandleEvent() of another PresShell instance. This patch creates
PresShell::EventHandler::GetFrameForHandlingEventWith() to do it.

Unfortunately, the result is used in 3 patterns. One is, the caller should
stop handling the event. Another one is, the caller should keep handling
the event by itself. The other is, the caller should call
PresShell::HandleEvent() of different PresShell instance. Therefore, this
patch makes the method take aFrame of the caller. Then, the caller can check
the last 2 patterns with check the result is same as aFrame. This is not so
smart approach, but I have no better idea without adding a bool argument or
making the return type bool and adding out argument of nsIFrame.

 (Assignee)

Comment 24

•
a month ago

Created attachment 9037513 [details]
Bug 1466208 - part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith()

 (Assignee)

Comment 25

•
a month ago

Created attachment 9037515 [details]
Bug 1466208 - part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary

Let's move the redirection of coming event in
PresShell::EventHandler::HandleEvent() into a method. This makes the caller
easier to read.

 (Assignee)

Comment 26

•
a month ago

Created attachment 9037516 [details]
Bug 1466208 - part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event

It may not be safe to handle events even when
PresShell::EventHandler::HandleEvent(). In such case, we need to discard
received events with notifying somebody. This patch move this rare case
jobs into the new method, MaybeDiscardEvent(). Then, the caller, HandleEvnet(),
becomes easier to read.

Comment 30

•
26 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/66c3b5b15c8e
part 1: Create stack class to handle events in PresShell r=smaug

Comment 33

•
26 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8b7c7e0317ef
part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub r=smaug

Comment 36

•
25 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c650507f7fee
part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style r=smaug

Comment 39

•
25 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9d38de19e569
part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event r=smaug

Comment 43

•
24 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7e9c7b742cb9
part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event r=smaug

Comment 46

•
24 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/646991effcda
part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument() r=smaug

Comment 50

•
23 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e7930893f8f0
part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance r=smaug

Comment 52

•
23 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ea7d56b00394
part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith() r=smaug

Comment 56

•
21 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1d868390c454
part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary r=smaug

Comment 58

•
21 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8d612e0b0258
part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event r=smaug
 (Assignee)

Comment 61

•
20 days ago

Created attachment 9041099 [details]
Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()

PresShell::EventHandler::HandleEvent() discards or puts off to dispatch
the handling event if it's a keyboard event and event dispatching is
suppressed by the document.

This patch moves the block into the new method for making HandleEvent() simpler.

 (Assignee)

Comment 62

•
20 days ago

Created attachment 9041100 [details]
Bug 1466208 - part 12: Get rid of frame variable from PresShell::EventHandler::HandleEvent()

There is an unclear variable frame in PresShell::EventHandler::HandleEvent().
It's overwritten with different frame and its meanings is changed sometimes.
Finally, it's necessary only in the if (aGUIEvent->IsUsingCoordinates())
block. Therefore, we can move it into the block and rename it when them for
each purpose.

 (Assignee)

Comment 63

•
20 days ago

Created attachment 9041101 [details]
Bug 1466208 - part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles()

PresShell::EventHandler::HandleEvent() tries to flush pending animation first
when it decides frame to handle events using coordinates. This patch moves
the code into the new method.

 (Assignee)

Comment 64

•
20 days ago

Created attachment 9041103 [details]
Bug 1466208 - part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent()

In some reasons, handling event should be handled in specific frame even if
the coordinates are out of the frame. PresShell::EventHandler::HandleEvent()
computes it with popups, capturing content, etc. This patch moves the blocks
into new method for making HandleEvent() simpler.

Note that most of the code is just moved. The following patch will clean it
up.

 (Assignee)

Comment 65

•
20 days ago

Created attachment 9041105 [details]
Bug 1466208 - part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent()

PresShell::EventHandler::ComputeRootFrameToHandleEvent() computes root frame
to handle event with popup frame and/or capturing content. The former result
can be rewritten with the latter. So, for cleaning it up with early return
style, we need to split it to 2 methods.

 (Assignee)

Comment 66

•
20 days ago

Created attachment 9041106 [details]
Bug 1466208 - part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style

Attachment #9041099 - Attachment description: Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrPutOffToDispatchKeyboardEvent() → Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()

Comment 69

•
17 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7fd1b72f66c0
part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent() r=smaug

Comment 71

•
17 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d8420442ba89
part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent() r=smaug

Comment 75

•
16 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/da71b4d4ad40
part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles() r=smaug

Comment 77

•
16 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/546288d07c7c
part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug

Comment 81

•
15 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/41be96f0dbbb
part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug

Comment 83

•
14 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b79d7da6617b
part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style r=smaug
 (Assignee)

Comment 88

•
14 days ago

Created attachment 9042806 [details]
Bug 1466208 - part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame

This patch splits the part handling event when there is pointer capturing
content but it does not have frame.

 (Assignee)

Comment 89

•
14 days ago

Created attachment 9042807 [details]
Bug 1466208 - part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent()

This patch moves the block, which discard or put the event into the delayed event
queue if handling event is a mouse event, to new method.

 (Assignee)

Comment 90

•
13 days ago

Created attachment 9042851 [details]
Bug 1466208 - part 19: Group PresShell, nsIFrame and nsIContent with a struct

PresShell::EventHandler::HandleEvent() looks for PresShell, nsIFrame and
nsIContent a lot for aGUIEvent. Sometimes part of them are modified,
otherwise, all of them are modified by some reasons. Therefore, for
splitting each of the modifiers into separated methods, we need a struct
for making them as a group and usable for in/out parameter.

(If you have some ideas of better name, let me know.)

 (Assignee)

Comment 91

•
13 days ago

Created attachment 9042852 [details]
Bug 1466208 - part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument()

Now, we can create methods to update event target into EventTargetData().
This moves a block in PresShell::EventHandler::HandleEvent() to retarget
to active document into the new method.

 (Assignee)

Comment 92

•
13 days ago

Created attachment 9042853 [details]
Bug 1466208 - part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style

 (Assignee)

Comment 93

•
13 days ago

Created attachment 9042854 [details]
Bug 1466208 - part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame()

This patch moves the block to compute event target of the event using
coordinates into the new method of PresShell::EventHandler::EventTargetData.

Comment 94

•
12 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d8ad6016347f
part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame r=smaug

Comment 96

•
12 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d70e9638ff75
part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent() r=smaug

Comment 98

•
11 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/86b52991b24a
part 19: Group PresShell, nsIFrame and nsIContent with a struct r=smaug

Comment 100

•
9 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1bcbf6ebcd45
part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() r=smaug

Comment 101

•
9 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4cb13cbe5f05
part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style r=smaug

Comment 104

•
6 days ago 
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4a53016221d4
part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame() r=smaug
You need to log in before you can comment on or make changes to this bug.