Summary: | HTMLMediaElement registers wrong ScriptExecutionContext with its ActiveDOMObject parent class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||
Component: | New Bugs | Assignee: | 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
Jer Noble
2019-02-06 13:43:31 PST
Created attachment 361325 [details]
Patch
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 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 Created attachment 361329 [details]
Patch
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. (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? (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 on attachment 361329 [details] Patch Clearing flags on attachment: 361329 Committed r241130: <https://trac.webkit.org/changeset/241130> All reviewed patches have been landed. Closing bug. |