GetExecutionContext() == nullptr in event_target.cc

[Fergal Daly]

I just ran into a clusterfuzz bug where this happens.

My fix is here

https://crrev.com/c/3858841

but I wonder if we should just always refuse to add a listener when there is no ExecutionContext?

F

[Fergal Daly]

On Tue, 30 Aug 2022 at 04:26, Ian Clelland <icle...@google.com> wrote:

On Mon, Aug 29, 2022 at 5:53 AM Fergal Daly <fer...@google.com> wrote:

I just ran into a clusterfuzz bYes, I think ug where this happens.

My fix is here

https://crrev.com/c/3858841

but I wonder if we should just always refuse to add a listener when there is no ExecutionContext?

We should definitely fix the code so that we don't do a nullptr deref, but I'm wondering in general what it would mean to add an event listener to an object with no execution context. Clearly script is running, in order for addEventListener to be called in the first place. But, I suppose if the target really has no association of its own with an execution context, as seems to be the case with the TextTrack, and apparently RemoteDOMWindows and possibly some AccessibilityNodes, then I would suggest that they're probably not valid event targets, and we shouldn't attempt to add listeners. (So Yes, I think that what you're proposing makes sense)

The TextTrack case specifically though is a bit confusing (maybe because I don't understand the use case for it yet) -- I'm not sure why it can't delegate GetExecutionContext to the <track> element that owns it?

Also, it appears that TextTrack::GetExecutionContext can return nullptr or not, depending on whether it currently has a MediaElement associated with it, and that can change over time. It seems like the behaviour of addEventListener might be inconsistent, depending on what order things are attached to the document in.

I don't know enough about ExeutionContexts but

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/execution_context/execution_context.h?q=executioncontext&ss=chromium

// Typically, the ExecutionContext is an instance of LocalDOMWindow or of

// WorkerOrWorkletGlobalScope.

So for an HTML element (or object coming from one), when would the ExecutionContext not just be the LocalDOMWindow? Is there ever a reason to return nullptr instead of just going and finding that?

F

 

F

[Ian Clelland]

On Mon, Aug 29, 2022 at 5:53 AM Fergal Daly <fer...@google.com> wrote:

I just ran into a clusterfuzz bYes, I think ug where this happens.

My fix is here

https://crrev.com/c/3858841

but I wonder if we should just always refuse to add a listener when there is no ExecutionContext?

We should definitely fix the code so that we don't do a nullptr deref, but I'm wondering in general what it would mean to add an event listener to an object with no execution context. Clearly script is running, in order for addEventListener to be called in the first place. But, I suppose if the target really has no association of its own with an execution context, as seems to be the case with the TextTrack, and apparently RemoteDOMWindows and possibly some AccessibilityNodes, then I would suggest that they're probably not valid event targets, and we shouldn't attempt to add listeners. (So Yes, I think that what you're proposing makes sense)

The TextTrack case specifically though is a bit confusing (maybe because I don't understand the use case for it yet) -- I'm not sure why it can't delegate GetExecutionContext to the <track> element that owns it?

Also, it appears that TextTrack::GetExecutionContext can return nullptr or not, depending on whether it currently has a MediaElement associated with it, and that can change over time. It seems like the behaviour of addEventListener might be inconsistent, depending on what order things are attached to the document in.

F

[Mason Freed]

I put a comment on the bug to this effect, but I also think it's weird that the TextTrack doesn't get its ExecutionContext from it's <track>. It seems like perhaps the GetExecutionContext() override was just added in an "easy" way, grabbing it from the MediaElement, which might or might not exist. But at the point of construction, I'd think we could get an ExecutionContext and keep it.

I still think the CL is the right way to go: AddEventListenerInternal should null-check the execution context before using it.

Thanks for following up on this!

Thanks,

Mason

[Mason Freed]

Yep, thanks. I'll see how much work it'll take to fix up TextTrack.

Thanks,

Mason

On Mon, Aug 29, 2022 at 6:21 PM Fergal Daly <fer...@google.com> wrote:

That has been submitted. I'll leave it up to DOM folks to decide whether to follow up and try to eliminate cases where there is no execution context,

F

[Fergal Daly]

That has been submitted. I'll leave it up to DOM folks to decide whether to follow up and try to eliminate cases where there is no execution context,

F

On Tue, 30 Aug 2022 at 06:25, Mason Freed <mas...@chromium.org> wrote: