1845215 - Make `ContentEventHandler` stop using strong pointers as far as possible

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

According to the profile reported in bug 1844470, ContentEventHandler spends on the content iterator which updates and references its nsCOMPtr<nsINode> members a lot. Additionally, ContentEventHandler::NodePosition inherits RangeBoundary and used for temporary use a lot. However, while they are being used, there is no call of MOZ_CAN_RUN_SCRIPT. Therefore, we can make them work without strong pointers.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1845215 - part 1: Make `ContentIteratorBase` a template class to work without strong pointers r=smaug!,#dom-core

ContentEventHandler works with PostContentIterator and PreContentIterator
when it scans DOM nodes in a range. While iterating the DOM nodes, script
never runs. Therefore, we can make ContentEventHandler work with new
content iterators which do not store nodes with strong pointers.

This patch makes ContentIteratorBase a template class and create new
UnsafePostContentIterator and UnsafePreContentIterator. They will
check whether DOM mutation occurs before destruction to detect dangerous
regressions.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1845215 - part 2: Make `ContentEventHandler::NodePosition` inherits `RawRangeBoundary` instead of `RangeBoundary` r=smaug!,#dom-core

It's used for tying a set of node and offset in it in short time and
ContentEventHandler does not flush pending things in most paths.
Therefore, it can avoid using RangeBoundary for solving its performance
issue.

Depends on D184441

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, in this profile, ContentEventHandler::RawRange appears too. I'll add a patch for it today.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Testing again with additional patches...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1845215 - part 3: Make `ContentEventHandler::RawRange` a template class r=smaug!,#dom-core

It's used as both a temporary object in the stack and storing first range
selection range while ContentEventHandler handles everything. In the
first case, ContentEventHandler won't call any methods marked as
MOZ_CAN_RUN_SCRIPT. Therefore, we can make it store only raw nsINode
pointer and RawRangeBoundary.

Depends on D184442

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1845215 - part 4: Make `ContentEventHandler` stop using strong pointers if safe r=smaug,#dom-core

Depends on D184466

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1845215 - part 5: Rename `ContentEventHandler::*RawRange*` to `*SimpleRange*` r=smaug!,#dom-core

RawRange sounds like related to RawRangeBoundary, but it now has
RangeBoundary members and UnsafeRawRange has RawRangeBoundarys.

They are helpers for ContentEventHandler to work faster than nsRange
and dom::StaticRange with avoiding to do unnecessary things for
ContentEventHandler. Therefore, this patch renames them to *SimplRange*
since they just store two range boundaries and their root node simply.

Depends on D184467

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3601308f553b
part 1: Make `ContentIteratorBase` a template class to work without strong pointers r=smaug
https://hg.mozilla.org/integration/autoland/rev/681e574c9ccb
part 2: Make `ContentEventHandler::NodePosition` inherits `RawRangeBoundary` instead of `RangeBoundary` r=smaug
https://hg.mozilla.org/integration/autoland/rev/d0a2d3fc1837
part 3: Make `ContentEventHandler::RawRange` a template class r=smaug
https://hg.mozilla.org/integration/autoland/rev/716372eb9576
part 4: Make `ContentEventHandler` stop using strong pointers if safe r=smaug
https://hg.mozilla.org/integration/autoland/rev/b2b54f91567d
part 5: Rename `ContentEventHandler::*RawRange*` to `*SimpleRange*` r=smaug

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3601308f553b
https://hg.mozilla.org/mozilla-central/rev/681e574c9ccb
https://hg.mozilla.org/mozilla-central/rev/d0a2d3fc1837
https://hg.mozilla.org/mozilla-central/rev/716372eb9576
https://hg.mozilla.org/mozilla-central/rev/b2b54f91567d