Title: [257618] trunk
- Revision
- 257618
- Author
- timothy_hor...@apple.com
- Date
- 2020-02-27 22:21:20 -0800 (Thu, 27 Feb 2020)
Log Message
UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window
https://bugs.webkit.org/show_bug.cgi?id=208365
Reviewed by Alex Christensen.
Source/WebKit:
New test: WKWebView.PrepareForMoveToWindowCrashAfterNotMovingToWindow
* UIProcess/Cocoa/WebViewImpl.mm:
(-[WKWindowVisibilityObserver dealloc]):
(-[WKWindowVisibilityObserver setWindowToObserve:]):
(WebKit::WebViewImpl::viewWillMoveToWindow):
Two small changes to make WKWindowVisibilityObserver safer to use, which
fix the aforementioned bug:
- Instead of exposing startObserving/stopObserving and making clients
be careful about pairing them, and remembering which window to stopObserving,
just add "setWindowToObserve", and keep track (weakly) of the current
NSWindow being observed. This avoids double-adding observers.
- Always stopObserving when WKWindowVisibilityObserver is deallocated.
In the "normal" case, WKWebView will always be removed from the view
hierarchy before it is deallocated (and thus before the
WKWindowVisibilityObserver is deallocated), because otherwise its superview
holds a reference to it. But in the _prepareForMoveToWindow case, we do
not have this guarantee, and can end up deallocating the WKWebView without
getting a willMoveToWindow:nil. Make sure to clean up the observers in
that case, if the window is still around, otherwise when NSWindow sends
notifications, it will try to message a deallocated WKWebView.
Tools:
* TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:
(TEST):
Add a test! It was a 100% repro crash before this change.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (257617 => 257618)
--- trunk/Source/WebKit/ChangeLog 2020-02-28 05:53:20 UTC (rev 257617)
+++ trunk/Source/WebKit/ChangeLog 2020-02-28 06:21:20 UTC (rev 257618)
@@ -1,3 +1,34 @@
+2020-02-27 Tim Horton <timothy_hor...@apple.com>
+
+ UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window
+ https://bugs.webkit.org/show_bug.cgi?id=208365
+
+ Reviewed by Alex Christensen.
+
+ New test: WKWebView.PrepareForMoveToWindowCrashAfterNotMovingToWindow
+
+ * UIProcess/Cocoa/WebViewImpl.mm:
+ (-[WKWindowVisibilityObserver dealloc]):
+ (-[WKWindowVisibilityObserver setWindowToObserve:]):
+ (WebKit::WebViewImpl::viewWillMoveToWindow):
+ Two small changes to make WKWindowVisibilityObserver safer to use, which
+ fix the aforementioned bug:
+
+ - Instead of exposing startObserving/stopObserving and making clients
+ be careful about pairing them, and remembering which window to stopObserving,
+ just add "setWindowToObserve", and keep track (weakly) of the current
+ NSWindow being observed. This avoids double-adding observers.
+
+ - Always stopObserving when WKWindowVisibilityObserver is deallocated.
+ In the "normal" case, WKWebView will always be removed from the view
+ hierarchy before it is deallocated (and thus before the
+ WKWindowVisibilityObserver is deallocated), because otherwise its superview
+ holds a reference to it. But in the _prepareForMoveToWindow case, we do
+ not have this guarantee, and can end up deallocating the WKWebView without
+ getting a willMoveToWindow:nil. Make sure to clean up the observers in
+ that case, if the window is still around, otherwise when NSWindow sends
+ notifications, it will try to message a deallocated WKWebView.
+
2020-02-27 Brent Fulgham <bfulg...@apple.com>
[iOS] Remove logging for sysctl access to properties used by NSURLSession
Modified: trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm (257617 => 257618)
--- trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2020-02-28 05:53:20 UTC (rev 257617)
+++ trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2020-02-28 06:21:20 UTC (rev 257618)
@@ -201,6 +201,7 @@
WebKit::WebViewImpl *_impl;
BOOL _didRegisterForLookupPopoverCloseNotifications;
+ __weak NSWindow *_observingWindow;
}
- (instancetype)initWithView:(NSView *)view impl:(WebKit::WebViewImpl&)impl;
@@ -237,6 +238,8 @@
NSNotificationCenter *workspaceNotificationCenter = [[NSWorkspace sharedWorkspace] notificationCenter];
[workspaceNotificationCenter removeObserver:self name:NSWorkspaceActiveSpaceDidChangeNotification object:nil];
+ [self setWindowToObserve:nil];
+
[super dealloc];
}
@@ -299,6 +302,14 @@
[window removeObserver:self forKeyPath:@"titlebarAppearsTransparent" context:keyValueObservingContext];
}
+- (void)setWindowToObserve:(NSWindow *)window
+{
+ if (_observingWindow)
+ [self stopObserving:_observingWindow];
+ _observingWindow = window;
+ [self startObserving:window];
+}
+
- (void)startObservingFontPanel
{
[[NSFontPanel sharedFontPanel] addObserver:self forKeyPath:@"visible" options:0 context:keyValueObservingContext];
@@ -2209,9 +2220,7 @@
clearAllEditCommands();
- NSWindow *stopObservingWindow = m_targetWindowForMovePreparation ? m_targetWindowForMovePreparation : [m_view window];
- [m_windowVisibilityObserver stopObserving:stopObservingWindow];
- [m_windowVisibilityObserver startObserving:window];
+ [m_windowVisibilityObserver setWindowToObserve:window];
}
void WebViewImpl::viewDidMoveToWindow()
Modified: trunk/Tools/ChangeLog (257617 => 257618)
--- trunk/Tools/ChangeLog 2020-02-28 05:53:20 UTC (rev 257617)
+++ trunk/Tools/ChangeLog 2020-02-28 06:21:20 UTC (rev 257618)
@@ -1,3 +1,14 @@
+2020-02-27 Tim Horton <timothy_hor...@apple.com>
+
+ UIProcess crash after using _prepareForMoveToWindow, then deallocating the WKWebView before moving to the window
+ https://bugs.webkit.org/show_bug.cgi?id=208365
+
+ Reviewed by Alex Christensen.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm:
+ (TEST):
+ Add a test! It was a 100% repro crash before this change.
+
2020-02-27 Kate Cheney <katherine_che...@apple.com>
TestWebKitAPI and WebKitTestRunner should have bundle identifiers
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm (257617 => 257618)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm 2020-02-28 05:53:20 UTC (rev 257617)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/PrepareForMoveToWindow.mm 2020-02-28 06:21:20 UTC (rev 257618)
@@ -76,4 +76,25 @@
TestWebKitAPI::Util::run(&isDone);
}
+TEST(WKWebView, PrepareForMoveToWindowCrashAfterNotMovingToWindow)
+{
+ auto webView = adoptNS([[WKWebView alloc] init]);
+ RetainPtr<NSWindow> window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:NO]);
+
+ [webView _prepareForMoveToWindow:window.get() completionHandler:^{ }];
+
+ dispatch_async(dispatch_get_main_queue(), ^{
+ [webView _prepareForMoveToWindow:window.get() completionHandler:^{ }];
+
+ dispatch_async(dispatch_get_main_queue(), ^{
+ [window setFrame:NSMakeRect(0, 0, 10, 10) display:YES];
+ isDone = true;
+ });
+ });
+
+ webView = nil;
+
+ TestWebKitAPI::Util::run(&isDone);
+}
+
#endif
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
