Skip to content
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

Add a fully active check for EventTarget in event listener inner invoke #1085

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shaseley
Copy link
Contributor

@shaseley shaseley commented Jun 8, 2022

This adds a fully active check in https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke that skips running event listeners if the EventTarget's realm's global's document is not fully active. Blink, Gecko, and WebKit were all skipping these event listeners, but the spec does not currently reflect this behavior.

I put the check where Blink and WebKit have it — just after the event listener was removed if it is a one-off. My understanding of Gecko's implementation is that the listeners are cleared on frame detach, so this should be compatable. The position of the check could be web observable if the document becomes fully active again, i.e. with bfcache, but Chromium at least doesn't cache pages with opener references.

Closes #1084.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2022
This adds tests for dispatching events to an EventTarget created in an
iframe that gets detached --- both prior to and during event dispatch.
The behavior of engines for this case is to not run event listeners
after the EventTarget's associated global is detached, which is what we
check for in the tests.

Spec: https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke
PR: whatwg/dom#1085

Bug: 1323391
Change-Id: I57aac7d444d3532ad0940a228452d206b5c1be07
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2022
This adds tests for dispatching events to an EventTarget created in an
iframe that gets detached --- both prior to and during event dispatch.
The behavior of engines for this case is to not run event listeners
after the EventTarget's associated global is detached, which is what we
check for in the tests.

Spec: https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke
PR: whatwg/dom#1085

Bug: 1323391
Change-Id: I57aac7d444d3532ad0940a228452d206b5c1be07
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

@smaug---- what do you think of this?

Was this tested for various kind of events, including synthetic events?

dom.bs Outdated Show resolved Hide resolved
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@shaseley
Copy link
Contributor Author

I manually tested with browser-initiated events via AbortController/AbortSignal and with synthetic events, for both detaching a frame prior to and during dispatch. I observed the behavior to be consistent across browsers. I have WPT tests for synthetic events here (not yet landed), and happy to add/run more tests if it makes sense.

@smaug----
Copy link
Collaborator

Firefox has checks in the webidl callbacks layer to prevent calling listener in the globals which are "gone".

@annevk
Copy link
Member

annevk commented Jul 22, 2022

That would suggest it needs to be added to https://webidl.spec.whatwg.org/#call-a-user-objects-operation although that would also affect other callers of that algorithm. (None come to mind at the moment.)

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Jul 22, 2022
@shaseley
Copy link
Contributor Author

Firefox has checks in the webidl callbacks layer to prevent calling listener in the globals which are "gone".

The global of the EventTarget or the listener? In this case, the checks are about the global of the EventTarget, i.e. the listener's global can still be attached, but the target's is not.

When I filed the issue, I did some debugging in FF code to see if I could figure out where the check was happening. I observed the following:

  1. If the EventTarget's global is detached when dispatching events, we hit an early out in EventTargetChainItem::HandleEvent() because the EventListenerManager is null.
  2. If the EventTarget's global is detached during dispatch, I found iteration during HandleEventInternal() abruptly ends because the listeners got cleared. This happens, for example, if there are two event listeners and the first detaches the event target's global. I found code that "disconnects" the event targets and clears listeners which I think caused this, but regardless I definitely observed iteration ending after the first event listener detached the event target's global.

I don't know this code nearly as well as y'all, but this is what I observed with some hacky printf debugging; hopefully that's helpful.

@smaug----
Copy link
Collaborator

In Gecko if the global of the WebIDL callback itself isn't the current one anymore, the bindings layer prevents the call.
Certain EventTarget objects do indeed disconnect themselves too when closing the relevant global (the idea is roughly that things like XHR isn't really usable one the global is gone).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: events
Development

Successfully merging this pull request may close these issues.

Should event dispatch check that the event target is in a fully active realm?
3 participants