Mitigate crashes when removing KVO from NSWindow in -[WKWindowVisibil…

…ityObserver stopObserving:]

https://bugs.webkit.org/show_bug.cgi?id=249103
rdar://102360839

Reviewed by Patrick Angle.

After the fix in 256334@main, Music sometimes crashes when destroying `NSWindow`, when
`WKWindowVisibilityObserver` attempts to remove key-value observers for "contentLayoutRect" and
"titlebarAppearsTransparent" from the window that were not added in the first place.

While I haven't been able to reproduce the crash locally or come up with a test case that (exactly)
replicates the crashing stack during `NSWindow` destruction, it should be possible to avoid it
altogether by guarding KVO registration and unregistration by using an associated object on the
`NSWindow` to indicate when `WKWindowVisibilityObserver` has key-value observers to the window. If
this flag is not set, then we avoid attempting to unregister KVO; similarly, if this flag is set,
then we avoid attempting to re-register KVO.

Test: WKWebView.PrepareForMoveToWindowShouldNotCrashWhenRemovingWindowObservers

* Source/WebKit/UIProcess/mac/WebViewImpl.mm:
(-[WKWindowVisibilityObserver startObserving:]):
(-[WKWindowVisibilityObserver stopObserving:]):

Use the `_impl` pointer as the context key.

(WebKit::WebViewImpl::viewWillMoveToWindowImpl):
(WebKit::WebViewImpl::viewWillMoveToWindow):

Move a debug assertion from `viewWillMoveToWindowImpl` into the call site in `viewWillMoveToWindow`,
such that the assertion doesn't fire due to the SPI being misused by a client (which the new test
case exercises).

* Tools/TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:

Add a (somewhat contrived) API test that exercises the mitigation.

Canonical link: https://commits.webkit.org/257860@main

Source/WebKit/UIProcess/mac/WebViewImpl.mm

@@ -332,6 +332,10 @@ - (void)startObserving:(NSWindow *)window
     if (_shouldObserveFontPanel)
         [self startObservingFontPanel];
 
+    if (objc_getAssociatedObject(window, _impl))
+        return;
+
+    objc_setAssociatedObject(window, _impl, @YES, OBJC_ASSOCIATION_COPY_NONATOMIC);
     [window addObserver:self forKeyPath:@"contentLayoutRect" options:NSKeyValueObservingOptionInitial context:keyValueObservingContext];
     [window addObserver:self forKeyPath:@"titlebarAppearsTransparent" options:NSKeyValueObservingOptionInitial context:keyValueObservingContext];
 }
@@ -362,6 +366,10 @@ - (void)stopObserving:(NSWindow *)window
     if (_shouldObserveFontPanel)
         [[NSFontPanel sharedFontPanel] removeObserver:self forKeyPath:@"visible" context:keyValueObservingContext];
 
+    if (!objc_getAssociatedObject(window, _impl))
+        return;
+
+    objc_setAssociatedObject(window, _impl, nil, OBJC_ASSOCIATION_COPY_NONATOMIC);
     [window removeObserver:self forKeyPath:@"contentLayoutRect" context:keyValueObservingContext];
     [window removeObserver:self forKeyPath:@"titlebarAppearsTransparent" context:keyValueObservingContext];
 }
@@ -2108,9 +2116,6 @@ static NSTrackingAreaOptions trackingAreaOptions()
 
 void WebViewImpl::viewWillMoveToWindowImpl(NSWindow *window)
 {
-    // If we're in the middle of preparing to move to a window, we should only be moved to that window.
-    ASSERT_IMPLIES(m_targetWindowForMovePreparation, m_targetWindowForMovePreparation == window);
-
     NSWindow *currentWindow = [m_view window];
     if (window == currentWindow)
         return;
@@ -2133,6 +2138,8 @@ static NSTrackingAreaOptions trackingAreaOptions()
 
 void WebViewImpl::viewWillMoveToWindow(NSWindow *window)
 {
+    // If we're in the middle of preparing to move to a window, we should only be moved to that window.
+    ASSERT_IMPLIES(m_targetWindowForMovePreparation, m_targetWindowForMovePreparation == window);
     viewWillMoveToWindowImpl(window);
     m_targetWindowForMovePreparation = nil;
     m_isPreparingToUnparentView = false;

Tools/TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm

@@ -70,6 +70,23 @@
     TestWebKitAPI::Util::run(&done);
 }
 
+TEST(WKWebView, PrepareForMoveToWindowShouldNotCrashWhenRemovingWindowObservers)
+{
+    auto window = adoptNS([NSWindow new]);
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+    [webView synchronouslyLoadTestPageNamed:@"simple"];
+
+    [[window contentView] addSubview:webView.get()];
+    [webView _prepareForMoveToWindow:nil completionHandler:^{ }];
+    [webView _prepareForMoveToWindow:window.get() completionHandler:^{ }];
+
+    __block bool done = false;
+    [webView _prepareForMoveToWindow:nil completionHandler:^{
+        done = true;
+    }];
+    TestWebKitAPI::Util::run(&done);
+}
+
 TEST(WKWebView, PrepareForMoveToWindowThenClose)
 {
     auto webView = adoptNS([[WKWebView alloc] init]);