Changeset 271479 in webkit

Timestamp:

Jan 13, 2021, 10:31:21 PM (6 years ago)

Author:

timothy_horton@apple.com

Message:

REGRESSION (r266634): Messages crashes sometimes while scrolling around and playing YouTube videos
​https://bugs.webkit.org/show_bug.cgi?id=220602
<rdar://problem/70402593>

Reviewed by Wenson Hsieh.

Source/WebKit:

No new tests; we are unable to API test video full-screen because of the lack of UIApp;
I have written a stand-alone test app that can reliably reproduce before this patch
and not afterwards.

• UIProcess/Cocoa/WebPageProxyCocoa.mm:

(WebKit::WebPageProxy::scheduleActivityStateUpdate):
We can't call dispatchActivityStateChange directly underneath a post-commit callback,
because it has side-effects (like un-parenting the full-screen window) that may result
in other frameworks (e.g. UIKit) trying to install commit handlers for the same phase,
which is not allowed.

To fix this, add a dispatch_async; we _only_ care that the activity state change
doesn't apply until after the active commit is complete.

Tools:

• TestWebKitAPI/PlatformWebView.h:

Mark PlatformWebView noncopyable, since it is effectively noncopyable
(at least, the macOS implementation will overrelease the view if you
copy it, whoops).

• TestWebKitAPI/Tests/WebKit/DeferredViewInWindowStateChange.mm:

(TestWebKitAPI::TEST):
Add a matching dispatch_async, or this test fails.

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/PlatformWebView.h (modified) (2 diffs)
• Tools/TestWebKitAPI/Tests/WebKit/DeferredViewInWindowStateChange.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-01-13  Tim Horton  <timothy_horton@apple.com>
+
+        REGRESSION (r266634): Messages crashes sometimes while scrolling around and playing YouTube videos
+        https://bugs.webkit.org/show_bug.cgi?id=220602
+        <rdar://problem/70402593>
+
+        Reviewed by Wenson Hsieh.
+
+        No new tests; we are unable to API test video full-screen because of the lack of UIApp;
+        I have written a stand-alone test app that can reliably reproduce before this patch
+        and not afterwards.
+
+        * UIProcess/Cocoa/WebPageProxyCocoa.mm:
+        (WebKit::WebPageProxy::scheduleActivityStateUpdate):
+        We can't call dispatchActivityStateChange directly underneath a post-commit callback,
+        because it has side-effects (like un-parenting the full-screen window) that may result
+        in other frameworks (e.g. UIKit) trying to install commit handlers for the same phase,
+        which is not allowed.
+
+        To fix this, add a dispatch_async; we _only_ care that the activity state change
+        doesn't apply until after the active commit is complete.
+
+
 2021-01-13  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm

@@ -506 +506 @@
     if (hasActiveCATransaction) {
         [CATransaction addCommitHandler:[weakThis = makeWeakPtr(*this)] {
-            auto protectedThis = makeRefPtr(weakThis.get());
-            if (!protectedThis)
-                return;
-
-            protectedThis->dispatchActivityStateChange();
+            // We can't call dispatchActivityStateChange directly underneath this commit handler, because it has side-effects
+            // that may result in other frameworks trying to install commit handlers for the same phase, which is not allowed.
+            // So, dispatch_async here; we only care that the activity state change doesn't apply until after the active commit is complete.
+            dispatch_async(dispatch_get_main_queue(), [weakThis] {
+                auto protectedThis = makeRefPtr(weakThis.get());
+                if (!protectedThis)
+                    return;
+
+                protectedThis->dispatchActivityStateChange();
+            });
         } forPhase:kCATransactionPhasePostCommit];
         return;

trunk/Tools/ChangeLog

@@ +1 @@
+2021-01-13  Tim Horton  <timothy_horton@apple.com>
+
+        REGRESSION (r266634): Messages crashes sometimes while scrolling around and playing YouTube videos
+        https://bugs.webkit.org/show_bug.cgi?id=220602
+        <rdar://problem/70402593>
+
+        Reviewed by Wenson Hsieh.
+
+        * TestWebKitAPI/PlatformWebView.h:
+        Mark PlatformWebView noncopyable, since it is effectively noncopyable
+        (at least, the macOS implementation will overrelease the view if you
+        copy it, whoops).
+
+        * TestWebKitAPI/Tests/WebKit/DeferredViewInWindowStateChange.mm:
+        (TestWebKitAPI::TEST):
+        Add a matching dispatch_async, or this test fails.
+
 2021-01-13  Alex Christensen  <achristensen@webkit.org>
 

trunk/Tools/TestWebKitAPI/PlatformWebView.h

@@ -28 +28 @@
 
 #include <wtf/FastMalloc.h>
+#include <wtf/Noncopyable.h>
 
 #if USE(CG)
@@ -70 +71 @@
 class PlatformWebView {
     WTF_MAKE_FAST_ALLOCATED;
+    WTF_MAKE_NONCOPYABLE(PlatformWebView);
 public:
     explicit PlatformWebView(WKPageConfigurationRef);

trunk/Tools/TestWebKitAPI/Tests/WebKit/DeferredViewInWindowStateChange.mm

@@ -76 +76 @@
     [wkView endDeferringViewInWindowChanges];
 
-    EXPECT_JS_TRUE(webView.page(), "document.hidden");
+    __block bool done = false;
+    WKPageRef page = webView.page();
+    dispatch_async(dispatch_get_main_queue(), ^{
+        EXPECT_JS_TRUE(page, "document.hidden");
+        done = true;
+    });
+
+    Util::run(&done);
 }