Bug 194360 – HTMLMediaElement registers wrong ScriptExecutionContext with its ActiveDOMObject parent class

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

HTMLMediaElement registers wrong ScriptExecutionContext with its ActiveDOMObject parent class

[Jer Noble]

<rdar://problem/47801392>

[Jer Noble]

Created attachment 361325 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361325&action=review

Can haz regression test?

> Source/WebCore/dom/Document.h:2144
> +    : ActiveDOMObject((ScriptExecutionContext*)&document.contextDocument())

static_cast?

[Chris Dumez]

Comment on attachment 361325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361325&action=review

>> Source/WebCore/dom/Document.h:2144
>> +    : ActiveDOMObject((ScriptExecutionContext*)&document.contextDocument())
> 
> static_cast?

I made the same review comment offline before Jer even uploaded the patch :P

[Jer Noble]

Created attachment 361329 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361329&action=review

r=me

> Source/WebCore/dom/ActiveDOMObject.h:118
>      explicit ActiveDOMObject(ScriptExecutionContext*);

Maybe we should, in a follow-up patch, remove this constructor entirely, since it's still an opportunity for our caller to forget to call contextDocument().

Also, as we discussed in person, it would be nice to refactor Document so that it has a ScriptExecutionContext accessor, rather than inheriting from ScriptExecutionContext. Inheriting from ScriptExecutionContext is wrong because DOMImplementation.createDocument() returns a document that is not a ScriptExecutionContext and must not be used as one, and also because the document is a property of the window/frame/ScriptExecutionContext, and is not the ScriptExecutionContext itself.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 361329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361329&action=review
> 
> r=me
> 
> > Source/WebCore/dom/ActiveDOMObject.h:118
> >      explicit ActiveDOMObject(ScriptExecutionContext*);
> 
> Maybe we should, in a follow-up patch, remove this constructor entirely,
> since it's still an opportunity for our caller to forget to call
> contextDocument().

But what if the ScriptExecutionContext is NOT a document? What about ActiveDOMObjects in workers?

[Jer Noble]

(In reply to Chris Dumez from comment #7)
> (In reply to Geoffrey Garen from comment #6)
> > Comment on attachment 361329 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=361329&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/dom/ActiveDOMObject.h:118
> > >      explicit ActiveDOMObject(ScriptExecutionContext*);
> > 
> > Maybe we should, in a follow-up patch, remove this constructor entirely,
> > since it's still an opportunity for our caller to forget to call
> > contextDocument().
> 
> But what if the ScriptExecutionContext is NOT a document? What about
> ActiveDOMObjects in workers?

I think the idea would be to have an explicit constructor for Workers.

[WebKit Commit Bot]

Comment on attachment 361329 [details]
Patch

Clearing flags on attachment: 361329

Committed r241130: <https://trac.webkit.org/changeset/241130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.