Summary: | Release assert in ScriptController::canExecuteScripts via HTMLMediaElement::~HTMLMediaElement() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | Media | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | eric.carlson, jer.noble, koivisto, sabouhallawa, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=185284 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 203600 | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2018-05-03 20:09:13 PDT
Created attachment 339514 [details]
Speculative fix
Comment on attachment 339514 [details] Speculative fix View in context: https://bugs.webkit.org/attachment.cgi?id=339514&action=review > Source/WebCore/ChangeLog:12 > + In long term, ResourceLoader::cancel should never fire load event synchronously: webkit.org/b/185284. This is confusing. How does cancelling loading the resource fire the load event? Should not it fire the abort event? > Source/WebCore/html/HTMLMediaElement.cpp:584 > + return !!s_destructorCount; I think there is no need for !!. > Source/WebCore/html/HTMLMediaElement.cpp:589 > + HTMLMediaElementDestructorScope() { ++s_destructorCount; } Do we expect s_destructorCount to be more than 1? Should not s_destructorCount be a bool, e.g. s_inHTMLMediaElementDestructor? Will the destructor of HTMLMediaElement call the destructor of itself or other HTMLMediaElements? I think neither should ever happen. (In reply to Said Abou-Hallawa from comment #3) > Comment on attachment 339514 [details] > Speculative fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339514&action=review > > > Source/WebCore/ChangeLog:12 > > + In long term, ResourceLoader::cancel should never fire load event synchronously: webkit.org/b/185284. > > This is confusing. How does cancelling loading the resource fire the load > event? Should not it fire the abort event? When the last resource that had been blocking document's load event gets canceled, we fire load event on the document. > > Source/WebCore/html/HTMLMediaElement.cpp:589 > > + HTMLMediaElementDestructorScope() { ++s_destructorCount; } > > Do we expect s_destructorCount to be more than 1? Should not > s_destructorCount be a bool, e.g. s_inHTMLMediaElementDestructor? Will the > destructor of HTMLMediaElement call the destructor of itself or other > HTMLMediaElements? I think neither should ever happen. Hopefully that's the case but the media code is so complicated that such a guarantee is not self-evident. Because we're trying to create a very safe fix here, I'm opt'ing to use the safest approach, which is to use a counter. Comment on attachment 339514 [details] Speculative fix View in context: https://bugs.webkit.org/attachment.cgi?id=339514&action=review r=me with nit. > Source/WebCore/loader/FrameLoader.cpp:811 > + if (HTMLMediaElement::isRunningDestructor()) { > + scheduleCheckCompleted(); We could really use an ASSERT_NOT_REACHED() here so that it crashes in debug builds. (In reply to Jer Noble from comment #5) > Comment on attachment 339514 [details] > Speculative fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339514&action=review > > r=me with nit. > > > Source/WebCore/loader/FrameLoader.cpp:811 > > + if (HTMLMediaElement::isRunningDestructor()) { > > + scheduleCheckCompleted(); > > We could really use an ASSERT_NOT_REACHED() here so that it crashes in debug > builds. Sure, will do. Committed r231392: <https://trac.webkit.org/changeset/231392> |