Skip to content

DOM: Fix gmail event handler registry crash #53510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 1, 2025

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

EventHandlerRegistry is used indirectly by the compositor to determine
if a local frame root has a node with an event listener for certain
types of events that the compositor could otherwise fast-path around
to avoid the main thread, if there are no relevant event listeners.

This class is 1:1 with local frame roots, which means there must be
logic to register/unregister nodes that move across same-origin
local roots. This ensures a cross-document adopted node's old local
frame root no longer tracks nodes that move out of it, and the node's
new local root starts tracking adopted nodes that have the relevant
event listeners that EHR cares about.

This logic exists in Node::WillMoveToNewDocument() and
Node::MoveEventListenersToNewDocument(), however before this CL it
was broken. It would only trigger a transfer across
EventHandlerRegistries if the document was moving across blink::Page
objects. However, there can be multiple same-origin local frame roots
in the same blink::Page, so the transfer logic was kicking in under
fewer circumstances that it should have.

This made it possible to:

  1. A trigger a DCHECK() in EventHandlerRegistry::CheckConsistency()
    that ensures every tracked event target shares the local frame
    root that's associated with the given EHR. The most notable site
    triggering this DCHECK() was gmail, which is how this issue was
    discovered.
  2. Transfer a node from one local frame root to another and break
    the event listeners on that node that the compositor cares about
    (see EventHandlerRegistry::EventHandlerClass) because the
    compositor would never know about these listener after the node
    transferred to its new local frame root's registry.

This CL fixes the update logic to transfer event targets across
registries whenever event targets move across local frame roots, and
adds two tests, one for each scenario above.

R=dbaron

Bug: 40277823
Change-Id: Icde11417e80850fe7c512d90895c90ddae9ac85b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6674259
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1480989}

`EventHandlerRegistry` is used indirectly by the compositor to determine
if a local frame root has a node with an event listener for certain
types of events that the compositor could otherwise fast-path around
to avoid the main thread, if there are no relevant event listeners.

This class is 1:1 with local frame roots, which means there must be
logic to register/unregister nodes that move across same-origin
local roots. This ensures a cross-document adopted node's *old* local
frame root no longer tracks nodes that move out of it, and the node's
*new* local root starts tracking adopted nodes that have the relevant
event listeners that EHR cares about.

This logic exists in `Node::WillMoveToNewDocument()` and
`Node::MoveEventListenersToNewDocument()`, however before this CL it
was broken. It would only trigger a transfer across
EventHandlerRegistries if the document was moving across blink::Page
objects. However, there can be multiple same-origin local frame roots
in the same blink::Page, so the transfer logic was kicking in under
fewer circumstances that it should have.

This made it possible to:
  1. A trigger a DCHECK() in `EventHandlerRegistry::CheckConsistency()`
     that ensures every tracked event target shares the local frame
     root that's associated with the given EHR. The most notable site
     triggering this DCHECK() was gmail, which is how this issue was
     discovered.
  2. Transfer a node from one local frame root to another and break
     the event listeners on that node that the compositor cares about
     (see `EventHandlerRegistry::EventHandlerClass`) because the
     compositor would never know about these listener after the node
     transferred to its *new* local frame root's registry.

This CL fixes the update logic to transfer event targets across
registries whenever event targets move across local frame roots, and
adds two tests, one for each scenario above.

R=dbaron

Bug: 40277823
Change-Id: Icde11417e80850fe7c512d90895c90ddae9ac85b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6674259
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1480989}
@chromium-wpt-export-bot chromium-wpt-export-bot marked this pull request as ready for review July 1, 2025 13:06
@wpt-pr-bot wpt-pr-bot added the html label Jul 1, 2025
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 41b0579 into master Jul 1, 2025
25 checks passed
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-69da8efc2d branch July 1, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants