Add retry for hit tests with expired epoch in result #37085

longvatrong111 · 2025-05-22T08:54:11Z

Follow up to hit_test failed occasionally when the touch event is sent and #36676 (comment), this PR adds a retry for hit tests with expired epoch in result.

Hit tests with outdated epoch mean it is too early to perform the hit test.

Solution: retry hit test for the event on the next webrender frame.
The retry should guarantee that:

Keep the correct order of events
Retry time is not too long

Test cases: ./mach test-wpt --product servodriver -r tests\wpt\tests\webdriver\tests\classic\element_click\events.py

cc: @xiaochengh , @yezhizhen

longvatrong111

I wonder should we retry all failed hit tests (the epoch may be mismatch but there is no item in the result to check)?

components/compositing/compositor.rs

yezhizhen

Hit tests with expired epoch mean it is too early to perform the hit test.

I think it means "too late" instead "too early"? If we retry, we are essentially hit-test against the new display list. It may work well with WebDriver as it is fairly static, but may introduce weird experience for real browsing..
cc @jdm

components/compositing/webview_renderer.rs

longvatrong111 · 2025-05-23T01:24:14Z

Hit tests with expired epoch mean it is too early to perform the hit test.

I think it means "too late" instead "too early"? If we retry, we are essentially hit-test against the new display list. It may work well with WebDriver as it is fairly static, but may introduce weird experience for real browsing.. cc @jdm

If two events occur in sequence, their effects should be applied sequentially.
In this issue, the first event triggers a rebuild of the display list, and the second event arrives during that process so it get "outdated epoch". Servo should wait until the process is complete to handle the second event, rather than ignore it.

components/webdriver_server/lib.rs

components/shared/embedder/input_events.rs

components/compositing/compositor.rs

components/compositing/webview_renderer.rs

mrobinson · 2025-05-23T11:01:32Z

I'm not sure a "retry" approach is the right one here. It feels like putting a bandaid over the real problem which is that hit testing needs to be done synchronously with script. 🫤

longvatrong111 · 2025-05-23T11:27:34Z

I'm not sure a "retry" approach is the right one here. It feels like putting a bandaid over the real problem which is that hit testing needs to be done synchronously with script. 🫤

We can think retry and synchronization are similar terms here because we pick a specific point to retry.
What we do here is making some events to wait until hit testing is ready for them.
p/s: we even don't need the retry. As my understanding, all hit test will fail from when epoch is updated in compositor to when compositor receives next webrender frame. So if events come during that period, just make them wait immediately, dont need to try hit test.

longvatrong111 · 2025-05-23T11:52:19Z

I'm not sure a "retry" approach is the right one here. It feels like putting a bandaid over the real problem which is that hit testing needs to be done synchronously with script. 🫤

We can think retry and synchronization are similar terms here because we pick a specific point to retry. What we do here is making some events to wait until hit testing is ready for them. p/s: we even don't need the retry. As my understanding, all hit test will fail from when epoch is updated in compositor to when compositor receives next webrender frame. So if events come during that period, just make them wait immediately, dont need to try hit test.

Maybe it leads to the problem of notify new epoch to compositor.
However, I think if we only retry failing hit test with mismatch epoch once in the next frame, it is an improvement to ignoring it.

xiaochengh · 2025-05-28T05:12:12Z

@mrobinson

I'm not sure a "retry" approach is the right one here. It feels like putting a bandaid over the real problem which is that hit testing needs to be done synchronously with script. 🫤

CMIIW but I think the situation is inevitable as long as we have compositor and script running on different threads, and possible solutions are:

Let compositor retry failed hit test attempts
Block compositor/script until synchronized
Allow compositor to return out-of-date hit test results.

Here 1 seems to be the easiest fix. 2 is much more complicated and may cause a bigger performance hit, and I don't see much benefits in there.

However, my favorite is 3, and I would like to try it later in the year https://github.com/servo/servo/wiki/Roadmap#architecture later

components/compositing/compositor.rs

components/compositing/webview_renderer.rs

components/compositing/compositor.rs

components/compositing/webview_renderer.rs

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

components/compositing/compositor.rs

components/compositing/webview_renderer.rs

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

components/compositing/webview_renderer.rs

kongbai1996 · 2025-06-23T03:02:43Z

components/compositing/webview_renderer.rs

+                return false;
+            },
+            _ => {
+                return false;


In this case, may also need to retry.

This just keeps the default logic, you can add a retry here with a proper reason.
I'm just curious, if so, basically we retry all failed hit test?

kongbai1996 · 2025-06-23T03:08:10Z

components/compositing/webview_renderer.rs

+                .borrow()
+                .hit_test_at_point(point, get_pipeline_details)
+            else {
+                // Don't need to process pending input events in this frame any more.


On some complex pages, such as https://m.huaweimossel.com, a retry failure may occur once.
For touchmove, failure only affects one swipe, which has a minor impact.
However, for touchdown and touchup, a retry failure has a significant impact on the application.
A touchdown failure will prevent the subsequent touchmove or click events from being completed.
A touchup failure will prevent the click event from being triggered. Additionally, the number of active_points saved in the script thread is incorrect. When the next touchdown is triggered, the application will receive two Touch messages.

a retry failure has a significant impact on the application

If we don't retry touch events, there is no impact?

The touchdown and touchup events exist in pairs. If touchdown is successful and touchup fails, the application's JavaScript will consider there to be a touchpoint present. When touchdown is triggered again, the number of touchpoints becomes 2.
When this situation occurs on the https://m.huaweimossel.com/ website, the carousel will not be able to slide. This is because the developer has checked the size of e.touches.length in the JavaScript code. This is the issue I have discovered so far.

Thanks for your details. As my understanding, the problem occurs because of hit test failings. Since the retry keeps the order of events, it doesn't play a role here.
Anyway, retry is just an improvement, not a final fix.
We can wait for this: #37085 (comment).

For the problem found now, should we first solve it by increasing the number of retries?

Increase amount of retry may lead to a scenario in which, 1 event takes several frames to be processed.
I'm thinking of retrying touch events as pairs of down and up, how do you think?

Do you mean that if either down or up fails, everything will be retried?

Yes both must succeed or fail together.

But there could be two touchdowns, and then two touchups. Or more.

longvatrong111 force-pushed the hit-testing-fix branch from cbc82a5 to ce8f9c6 Compare May 22, 2025 09:35

longvatrong111 changed the title ~~[Just a draft to share draft solution] - Add retry for hit tests with expired epoch in result~~ Add retry for hit tests with expired epoch in result May 22, 2025

longvatrong111 marked this pull request as ready for review May 22, 2025 09:42

longvatrong111 requested a review from mrobinson as a code owner May 22, 2025 09:42

longvatrong111 force-pushed the hit-testing-fix branch 2 times, most recently from afcd497 to 747726a Compare May 22, 2025 10:18

longvatrong111 commented May 22, 2025

View reviewed changes

components/compositing/compositor.rs Show resolved Hide resolved

yezhizhen reviewed May 22, 2025

View reviewed changes

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

xiaochengh reviewed May 23, 2025

View reviewed changes

longvatrong111 force-pushed the hit-testing-fix branch 2 times, most recently from 759c376 to 9ced3cd Compare May 25, 2025 17:18

xiaochengh reviewed May 28, 2025

View reviewed changes

longvatrong111 force-pushed the hit-testing-fix branch from dce9c69 to aa68ffd Compare May 28, 2025 10:04

longvatrong111 requested a review from xiaochengh May 28, 2025 10:04

xiaochengh reviewed May 29, 2025

View reviewed changes

longvatrong111 requested a review from gterzian as a code owner May 30, 2025 06:40

longvatrong111 force-pushed the hit-testing-fix branch from 2d09697 to 6da22fe Compare May 30, 2025 06:43

longvatrong111 requested a review from xiaochengh May 30, 2025 06:45

longvatrong111 force-pushed the hit-testing-fix branch 2 times, most recently from 5c762c6 to b68f5b1 Compare May 30, 2025 07:24

xiaochengh reviewed May 30, 2025

View reviewed changes

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

longvatrong111 added 4 commits June 3, 2025 16:16

Add retry for hit testing with expired epoch

b5a3c97

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Update hit test result return type

ddad26f

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Add flag for epoch mismatch

54d3763

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Apply delay when epoch is mismatch for touch events

afa3f4b

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

Add comment and rename epoch_not_synchronized

3d7ced5

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

xiaochengh reviewed Jun 3, 2025

View reviewed changes

components/compositing/compositor.rs Outdated Show resolved Hide resolved

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

components/compositing/webview_renderer.rs Outdated Show resolved Hide resolved

Refactor send_touch_event

0d02b61

Signed-off-by: batu_hoang <longvatrong111@gmail.com>

longvatrong111 force-pushed the hit-testing-fix branch from 8c69528 to 0d02b61 Compare June 3, 2025 09:57

xiaochengh approved these changes Jun 4, 2025

View reviewed changes

xiaochengh added this pull request to the merge queue Jun 4, 2025

Merged via the queue into servo:main with commit b422a65 Jun 4, 2025
21 checks passed

kongbai1996 reviewed Jun 6, 2025

View reviewed changes

components/compositing/webview_renderer.rs Show resolved Hide resolved

kongbai1996 reviewed Jun 6, 2025

View reviewed changes

components/compositing/webview_renderer.rs Show resolved Hide resolved

longvatrong111 deleted the hit-testing-fix branch June 10, 2025 18:10

kongbai1996 reviewed Jun 23, 2025

View reviewed changes

kongbai1996 mentioned this pull request Jun 28, 2025

When touchup hit test fails, script's event.touches.length is incorrect. #37763

Closed

kongbai1996 mentioned this pull request Jul 8, 2025

Move hittest from webrender to layout. #37932

Closed

Uh oh!

Add retry for hit tests with expired epoch in result #37085

Add retry for hit tests with expired epoch in result #37085

Uh oh!

Conversation

longvatrong111 commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longvatrong111 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yezhizhen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

longvatrong111 commented May 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrobinson commented May 23, 2025

Uh oh!

longvatrong111 commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longvatrong111 commented May 23, 2025

Uh oh!

xiaochengh commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kongbai1996 Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

longvatrong111 commented May 22, 2025 •

edited

Loading

longvatrong111 left a comment •

edited

Loading

longvatrong111 commented May 23, 2025 •

edited

Loading

kongbai1996 Jun 23, 2025 •

edited

Loading