Crash in -[WKFullScreenWindowController windowDidExitFullScreen:]

jyavenard · jyavenard · commit a3569b03fbab · 2021-08-02T23:19:18.000Z
https://bugs.webkit.org/show_bug.cgi?id=228694 rdar://79569881 Reviewed by Jer Noble. It appears that [WKFullScreenWindowController finishedEnterFullScreenAnimation] can cause the retain count to be decreased (this is the only explanation that makes any sense considering the crash and when the regression started following bug 227243). No test, can't reproduce. * UIProcess/mac/WKFullScreenWindowController.mm: (-[WKFullScreenWindowController setVideoFullscreenManagerClient:]): Add utility method to remove duplicated code. (-[WKFullScreenWindowController didExitPictureInPicture]): (-[WKFullScreenWindowController windowDidEnterFullScreen:]): (-[WKFullScreenWindowController windowDidFailToExitFullScreen:]): (-[WKFullScreenWindowController windowDidExitFullScreen:]): (-[WKFullScreenWindowController _manager]): fly-by make style consistent with other similar methods. Canonical link: https://commits.webkit.org/240192@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280568 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,26 @@
+2021-08-02  Jean-Yves Avenard  <jya@apple.com>
+
+        Crash in -[WKFullScreenWindowController windowDidExitFullScreen:]
+        https://bugs.webkit.org/show_bug.cgi?id=228694
+        rdar://79569881
+
+        Reviewed by Jer Noble.
+
+        It appears that [WKFullScreenWindowController finishedEnterFullScreenAnimation] can
+        cause the retain count to be decreased (this is the only explanation that makes any
+        sense considering the crash and when the regression started following bug 227243).
+        No test, can't reproduce.
+
+        * UIProcess/mac/WKFullScreenWindowController.mm:
+        (-[WKFullScreenWindowController setVideoFullscreenManagerClient:]): Add utility method
+        to remove duplicated code.
+        (-[WKFullScreenWindowController didExitPictureInPicture]):
+        (-[WKFullScreenWindowController windowDidEnterFullScreen:]):
+        (-[WKFullScreenWindowController windowDidFailToExitFullScreen:]):
+        (-[WKFullScreenWindowController windowDidExitFullScreen:]):
+        (-[WKFullScreenWindowController _manager]): fly-by make style consistent with other
+        similar methods.
+
 2021-08-02  Alex Christensen  <achristensen@webkit.org>
 
         Add log when experimental network loader is used
diff --git a/Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm b/Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm
@@ -645,14 +645,19 @@ - (void)didEnterPictureInPicture
         [self requestExitFullScreen];
 }
 
-- (void)didExitPictureInPicture
+- (void)setVideoFullscreenManagerClient:(WebKit::VideoFullscreenManagerProxyClient *)client
 {
     if (auto* videoFullscreenManager = self._videoFullscreenManager) {
         ASSERT(videoFullscreenManager->client() == &_videoFullscreenManagerProxyClient);
-        videoFullscreenManager->setClient(nullptr);
+        videoFullscreenManager->setClient(client);
     }
 }
 
+- (void)didExitPictureInPicture
+{
+    [self setVideoFullscreenManagerClient:nullptr];
+}
+
 #pragma mark -
 #pragma mark Custom NSWindow Full Screen Animation
 
@@ -683,27 +688,23 @@ - (void)windowDidFailToEnterFullScreen:(NSWindow *)window
 
 - (void)windowDidEnterFullScreen:(NSNotification *)notification
 {
+    RetainPtr<WKFullScreenWindowController> retain = self;
     [self finishedEnterFullScreenAnimation:YES];
-
-    if (auto* videoFullscreenManager = self._videoFullscreenManager) {
-        ASSERT(videoFullscreenManager->client() == nullptr);
-        videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient);
-    }
+    [self setVideoFullscreenManagerClient:&_videoFullscreenManagerProxyClient];
 }
 
 - (void)windowDidFailToExitFullScreen:(NSWindow *)window
 {
+    RetainPtr<WKFullScreenWindowController> retain = self;
     [self finishedExitFullScreenAnimation:NO];
+    [self setVideoFullscreenManagerClient:nullptr];
 }
 
 - (void)windowDidExitFullScreen:(NSNotification *)notification
 {
+    RetainPtr<WKFullScreenWindowController> retain = self;
     [self finishedExitFullScreenAnimation:YES];
-
-    if (auto* videoFullscreenManager = self._videoFullscreenManager) {
-        ASSERT(videoFullscreenManager->client() == &_videoFullscreenManagerProxyClient);
-        videoFullscreenManager->setClient(nullptr);
-    }
+    [self setVideoFullscreenManagerClient:nullptr];
 }
 
 - (NSWindow *)destinationWindowToExitFullScreenForWindow:(NSWindow *)window
@@ -718,6 +719,7 @@ - (NSWindow *)destinationWindowToExitFullScreenForWindow:(NSWindow *)window
 {
     if (!_page)
         return nullptr;
+
     return _page->fullScreenManager();
 }
 

Original file line number	Diff line number	Diff line change
`@@ -645,14 +645,19 @@ - (void)didEnterPictureInPicture`
`645`	`645`	`[self requestExitFullScreen];`
`646`	`646`	`}`
`647`	`647`
`648`		`-- (void)didExitPictureInPicture`
	`648`	`+- (void)setVideoFullscreenManagerClient:(WebKit::VideoFullscreenManagerProxyClient *)client`
`649`	`649`	`{`
`650`	`650`	`if (auto* videoFullscreenManager = self._videoFullscreenManager) {`
`651`	`651`	`ASSERT(videoFullscreenManager->client() == &_videoFullscreenManagerProxyClient);`
`652`		`- videoFullscreenManager->setClient(nullptr);`
	`652`	`+ videoFullscreenManager->setClient(client);`
`653`	`653`	`}`
`654`	`654`	`}`
`655`	`655`
	`656`	`+- (void)didExitPictureInPicture`
	`657`	`+{`
	`658`	`+ [self setVideoFullscreenManagerClient:nullptr];`
	`659`	`+}`
	`660`	`+`
`656`	`661`	`#pragma mark -`
`657`	`662`	`#pragma mark Custom NSWindow Full Screen Animation`
`658`	`663`
`@@ -683,27 +688,23 @@ - (void)windowDidFailToEnterFullScreen:(NSWindow *)window`
`683`	`688`
`684`	`689`	`- (void)windowDidEnterFullScreen:(NSNotification *)notification`
`685`	`690`	`{`
	`691`	`+ RetainPtr<WKFullScreenWindowController> retain = self;`
`686`	`692`	`[self finishedEnterFullScreenAnimation:YES];`
`687`		`-`
`688`		`- if (auto* videoFullscreenManager = self._videoFullscreenManager) {`
`689`		`- ASSERT(videoFullscreenManager->client() == nullptr);`
`690`		`- videoFullscreenManager->setClient(&_videoFullscreenManagerProxyClient);`
`691`		`- }`
	`693`	`+ [self setVideoFullscreenManagerClient:&_videoFullscreenManagerProxyClient];`
`692`	`694`	`}`
`693`	`695`
`694`	`696`	`- (void)windowDidFailToExitFullScreen:(NSWindow *)window`
`695`	`697`	`{`
	`698`	`+ RetainPtr<WKFullScreenWindowController> retain = self;`
`696`	`699`	`[self finishedExitFullScreenAnimation:NO];`
	`700`	`+ [self setVideoFullscreenManagerClient:nullptr];`
`697`	`701`	`}`
`698`	`702`
`699`	`703`	`- (void)windowDidExitFullScreen:(NSNotification *)notification`
`700`	`704`	`{`
	`705`	`+ RetainPtr<WKFullScreenWindowController> retain = self;`
`701`	`706`	`[self finishedExitFullScreenAnimation:YES];`
`702`		`-`
`703`		`- if (auto* videoFullscreenManager = self._videoFullscreenManager) {`
`704`		`- ASSERT(videoFullscreenManager->client() == &_videoFullscreenManagerProxyClient);`
`705`		`- videoFullscreenManager->setClient(nullptr);`
`706`		`- }`
	`707`	`+ [self setVideoFullscreenManagerClient:nullptr];`
`707`	`708`	`}`
`708`	`709`
`709`	`710`	`- (NSWindow )destinationWindowToExitFullScreenForWindow:(NSWindow )window`
`@@ -718,6 +719,7 @@ - (NSWindow )destinationWindowToExitFullScreenForWindow:(NSWindow )window`
`718`	`719`	`{`
`719`	`720`	`if (!_page)`
`720`	`721`	`return nullptr;`
	`722`	`+`
`721`	`723`	`return _page->fullScreenManager();`
`722`	`724`	`}`
`723`	`725`