Bug 194360

Summary: HTMLMediaElement registers wrong ScriptExecutionContext with its ActiveDOMObject parent class
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Jer Noble 2019-02-06 13:43:31 PST
HTMLMediaElement registers wrong ScriptExecutionContext with its ActiveDOMObject parent class
Comment 1 Jer Noble 2019-02-06 13:54:05 PST
<rdar://problem/47801392>
Comment 2 Jer Noble 2019-02-06 13:54:23 PST
Created attachment 361325 [details]
Patch
Comment 3 Geoffrey Garen 2019-02-06 14:10:20 PST
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?
Comment 4 Chris Dumez 2019-02-06 14:13:35 PST
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
Comment 5 Jer Noble 2019-02-06 14:30:27 PST
Created attachment 361329 [details]
Patch
Comment 6 Geoffrey Garen 2019-02-07 10:16:05 PST
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.
Comment 7 Chris Dumez 2019-02-07 10:17:32 PST
(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?
Comment 8 Jer Noble 2019-02-07 10:35:56 PST
(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.
Comment 9 WebKit Commit Bot 2019-02-07 10:59:55 PST
Comment on attachment 361329 [details]
Patch

Clearing flags on attachment: 361329

Committed r241130: <https://trac.webkit.org/changeset/241130>
Comment 10 WebKit Commit Bot 2019-02-07 10:59:57 PST
All reviewed patches have been landed.  Closing bug.