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

Should event dispatch check that the event target is in a fully active realm? #1084

Open
shaseley opened this issue Jun 3, 2022 · 4 comments · May be fixed by #1085
Open

Should event dispatch check that the event target is in a fully active realm? #1084

shaseley opened this issue Jun 3, 2022 · 4 comments · May be fixed by #1085

Comments

@shaseley
Copy link
Contributor

shaseley commented Jun 3, 2022

I can't find in the DOM or WebIDL specs where such a check exists; apologies if I missed it.

The scenario is (jsfiddle):

  • A page has an iframe
  • The iframe creates a new AbortController (controller) in its realm and hangs it off of its parent
  • The parent page adds an event listener (added and defined in its realm) to controller.signal
  • The iframe gets removed
  • controller.abort() is called in the parent page's context

Should the event handler run?

This is similar to whatwg/webidl#993, but it's the event target that is in a "detached realm", not the event listener or call to controller.abort().

All the engines are consistent though — no one runs the event listener:

  • Blink and WebKit both early out in ~2.10 of inner invoke.
    • For Blink specifically, we can't create the JS wrapper for the event because the context is gone.
  • Gecko skips handling the event during EventTargetChainItem::HandleEvent() because the EventListenerManager is null (IIUC it got cleared during detach). I think this is during targeting, just prior to starting inner invoke.

I was a bit surprised by the behavior, but given that no engine is running these, would it make sense to add an explicit check (assuming I didn't miss it) so the spec is in sync, and if so, thoughts on where?

@domenic
Copy link
Member

domenic commented Jun 3, 2022

Note that this appears to be enforced at the EventTarget level, not at the AbortController/AbortSignal level or at the Event level: https://jsfiddle.net/wL5evuh3/3/.

Given that this is about the EventTarget itself, and thus isn't covered by the Web IDL issue you list, probably the check would have to be at https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke . You might think we could factor it out to surround all of https://dom.spec.whatwg.org/#concept-event-dispatch but that would probably not do the right thing if the first listener removed the frame. (In that case I assume subsequent listeners should not run... but someone should test.)

@shaseley
Copy link
Contributor Author

shaseley commented Jun 3, 2022

Note that this appears to be enforced at the EventTarget level, not at the AbortController/AbortSignal level or at the Event level: https://jsfiddle.net/wL5evuh3/3/.

Nice, right, this isn't specific to AbortSignal --- just using that as an example because that's just how I discovered this.

Given that this is about the EventTarget itself, and thus isn't covered by the Web IDL issue you list, probably the check would have to be at https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke . You might think we could factor it out to surround all of https://dom.spec.whatwg.org/#concept-event-dispatch but that would probably not do the right thing if the first listener removed the frame. (In that case I assume subsequent listeners should not run... but someone should test.)

Re: WebIDL, I was curious what should happen if constructing an object in a realm whose global's document is detached. This happens earlier (step 2 of fire an event), but Blink doesn't create the wrapper until much later.

I'll test the "detached during an event with subsequent handlers" case. I was thinking about that as well and was planning to write a WPT for that assuming we make a spec change. I agree that the check can't happen too early because if this case.

@shaseley
Copy link
Contributor Author

shaseley commented Jun 4, 2022

It looks like all the engines don't run subsequent listeners after the event target's context gets detached. For Blink and WebKit, this follows from where we're checking this; for Gecko, I think the listeners get cleared on detach, so iteration over the listeners stops after detach.

And yeah, there would need to be a check in https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke to account for detaching from a listener. I'll try to send a PR next week along with some tests.

@annevk
Copy link
Member

annevk commented Jun 7, 2022

cc @smaug----

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants