Attachment 339514 Details for Bug 185288 – Speculative fix

[patch] Speculative fix

bug-185288-20180503202938.patch (text/plain), 3.91 KB, created by Ryosuke Niwa on 2018-05-03 20:29:38 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-05-03 20:29:38 PDT

Size: 3.91 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 231334)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2018-05-03  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Release assert in ScriptController::canExecuteScripts via HTMLMediaElement::~HTMLMediaElement()
>+        https://bugs.webkit.org/show_bug.cgi?id=185288
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The crash is caused by HTMLMediaElement::~HTMLMediaElement canceling the resource load via CachedResource
>+        which ends up calling FrameLoader::checkCompleted() and fire load event synchronously. Speculatively fix
>+        the crash by scheduling the check instead.
>+
>+        In long term, ResourceLoader::cancel should never fire load event synchronously: webkit.org/b/185284.
>+
>+        Unfortunately, no new tests. I can't get MediaResource to get destructed at the right time :(
>+
>+        * html/HTMLMediaElement.cpp:
>+        (WebCore::HTMLMediaElement::isRunningDestructor): Added to detect this specific case.
>+        (WebCore::HTMLMediaElementDestructorScope): Added.
>+        (WebCore::HTMLMediaElementDestructorScope::HTMLMediaElementDestructorScope): Added.
>+        (WebCore::HTMLMediaElementDestructorScope::~HTMLMediaElementDestructorScope): Added.
>+        (WebCore::HTMLMediaElement::~HTMLMediaElement): Instantiate HTMLMediaElement.
>+        * html/HTMLMediaElement.h:
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::checkCompleted): Call scheduleCheckCompleted instead of synchronously calling
>+        checkCompleted if we're in the middle of destructing a HTMLMediaElement.
>+
> 2018-05-03  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         Use pointer instead of std::optional<T&>
>Index: Source/WebCore/html/HTMLMediaElement.cpp
>===================================================================
>--- Source/WebCore/html/HTMLMediaElement.cpp	(revision 231334)
>+++ Source/WebCore/html/HTMLMediaElement.cpp	(working copy)
>@@ -576,8 +576,23 @@ void HTMLMediaElement::finishInitializat
>     mediaSession().clientWillBeginAutoplaying();
> }
> 
>+// FIXME: Remove this code once https://webkit.org/b/185284 is fixed.
>+static unsigned s_destructorCount = 0;
>+
>+bool HTMLMediaElement::isRunningDestructor()
>+{
>+    return !!s_destructorCount;
>+}
>+
>+class HTMLMediaElementDestructorScope {
>+public:
>+    HTMLMediaElementDestructorScope() { ++s_destructorCount; }
>+    ~HTMLMediaElementDestructorScope() { --s_destructorCount; }
>+};
>+
> HTMLMediaElement::~HTMLMediaElement()
> {
>+    HTMLMediaElementDestructorScope destructorScope;
>     ALWAYS_LOG(LOGIDENTIFIER);
> 
>     beginIgnoringTrackDisplayUpdateRequests();
>Index: Source/WebCore/html/HTMLMediaElement.h
>===================================================================
>--- Source/WebCore/html/HTMLMediaElement.h	(revision 231334)
>+++ Source/WebCore/html/HTMLMediaElement.h	(working copy)
>@@ -157,6 +157,8 @@ public:
> 
>     static HTMLMediaElement* bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose);
> 
>+    static bool isRunningDestructor();
>+
>     WEBCORE_EXPORT void rewind(double timeDelta);
>     WEBCORE_EXPORT void returnToRealtime() override;
> 
>Index: Source/WebCore/loader/FrameLoader.cpp
>===================================================================
>--- Source/WebCore/loader/FrameLoader.cpp	(revision 231334)
>+++ Source/WebCore/loader/FrameLoader.cpp	(working copy)
>@@ -805,6 +805,12 @@ void FrameLoader::checkCompleted()
>     // Have we completed before?
>     if (m_isComplete)
>         return;
>+    
>+    // FIXME: Remove this code once https://webkit.org/b/185284 is fixed.
>+    if (HTMLMediaElement::isRunningDestructor()) {
>+        scheduleCheckCompleted();
>+        return;
>+    }
> 
>     // FIXME: It would be better if resource loads were kicked off after render tree update (or didn't complete synchronously).
>     //        https://bugs.webkit.org/show_bug.cgi?id=171729

Flags:

jer.noble: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185288: 339514