- Revision
- 285086
- Author
- cdu...@apple.com
- Date
- 2021-10-30 18:24:38 -0700 (Sat, 30 Oct 2021)
Log Message
Improve error handling in sendWithAsyncReply()
https://bugs.webkit.org/show_bug.cgi?id=232508
Reviewed by Darin Adler.
Source/WebKit:
* Platform/IPC/Connection.cpp:
(IPC::Connection::invalidate):
We stop delivering messages as soon as |m_valid| becomes false. It is
thus a good idea to cancel any pending async reply handlers as soon
as we set m_isValid to false. We were doing it correctly in connectionDidClose()
but not in invalidate(). The Connection destructor would eventually call
the async reply handlers. However, I have noticed when investigating a leak
that this may delay the calling of the reply handler for a long time if
something keeps the Connection alive (or forever in case of a leak).
* Platform/IPC/Connection.h:
(IPC::Connection::sendWithAsyncReply):
Return early if the connection is no longer valid and call the
completion handler right away (but asynchronously). Previously,
we'd register the async reply handler and then the call to
sendMessage() would fail.
* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView _getContentsAsStringWithCompletionHandlerKeepIPCConnectionAliveForTesting:]):
* UIProcess/API/Cocoa/WKWebViewPrivate.h:
Add helper SPI for API test.
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm:
(TEST):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (285085 => 285086)
--- trunk/Source/WebKit/ChangeLog 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Source/WebKit/ChangeLog 2021-10-31 01:24:38 UTC (rev 285086)
@@ -1,3 +1,32 @@
+2021-10-30 Chris Dumez <cdu...@apple.com>
+
+ Improve error handling in sendWithAsyncReply()
+ https://bugs.webkit.org/show_bug.cgi?id=232508
+
+ Reviewed by Darin Adler.
+
+ * Platform/IPC/Connection.cpp:
+ (IPC::Connection::invalidate):
+ We stop delivering messages as soon as |m_valid| becomes false. It is
+ thus a good idea to cancel any pending async reply handlers as soon
+ as we set m_isValid to false. We were doing it correctly in connectionDidClose()
+ but not in invalidate(). The Connection destructor would eventually call
+ the async reply handlers. However, I have noticed when investigating a leak
+ that this may delay the calling of the reply handler for a long time if
+ something keeps the Connection alive (or forever in case of a leak).
+
+ * Platform/IPC/Connection.h:
+ (IPC::Connection::sendWithAsyncReply):
+ Return early if the connection is no longer valid and call the
+ completion handler right away (but asynchronously). Previously,
+ we'd register the async reply handler and then the call to
+ sendMessage() would fail.
+
+ * UIProcess/API/Cocoa/WKWebView.mm:
+ (-[WKWebView _getContentsAsStringWithCompletionHandlerKeepIPCConnectionAliveForTesting:]):
+ * UIProcess/API/Cocoa/WKWebViewPrivate.h:
+ Add helper SPI for API test.
+
2021-10-30 Kate Cheney <katherine_che...@apple.com>
[iOS 15] Loads after WKWebView session restore are marked as app-initiated
Modified: trunk/Source/WebKit/Platform/IPC/Connection.cpp (285085 => 285086)
--- trunk/Source/WebKit/Platform/IPC/Connection.cpp 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Source/WebKit/Platform/IPC/Connection.cpp 2021-10-31 01:24:38 UTC (rev 285086)
@@ -428,6 +428,7 @@
}
m_isValid = false;
+ clearAsyncReplyHandlers(*this);
m_connectionQueue->dispatch([protectedThis = Ref { *this }]() mutable {
protectedThis->platformInvalidate();
Modified: trunk/Source/WebKit/Platform/IPC/Connection.h (285085 => 285086)
--- trunk/Source/WebKit/Platform/IPC/Connection.h 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Source/WebKit/Platform/IPC/Connection.h 2021-10-31 01:24:38 UTC (rev 285086)
@@ -546,6 +546,13 @@
{
COMPILE_ASSERT(!T::isSync, AsyncMessageExpected);
+ if (!isValid()) {
+ RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
+ T::cancelReply(WTFMove(completionHandler));
+ });
+ return 0;
+ }
+
auto encoder = makeUniqueRef<Encoder>(T::name(), destinationID);
uint64_t listenerID = nextAsyncReplyHandlerID();
addAsyncReplyHandler(*this, listenerID, CompletionHandler<void(Decoder*)>([completionHandler = WTFMove(completionHandler)] (Decoder* decoder) mutable {
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm (285085 => 285086)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm 2021-10-31 01:24:38 UTC (rev 285086)
@@ -2962,6 +2962,14 @@
});
}
+- (void)_getContentsAsStringWithCompletionHandlerKeepIPCConnectionAliveForTesting:(void (^)(NSString *, NSError *))completionHandler
+{
+ THROW_IF_SUSPENDED;
+ _page->getContentsAsString(WebKit::ContentAsStringIncludesChildFrames::No, [handler = makeBlockPtr(completionHandler), connection = RefPtr { _page->process().connection() }](String string) {
+ handler(string, nil);
+ });
+}
+
- (void)_getContentsOfAllFramesAsStringWithCompletionHandler:(void (^)(NSString *))completionHandler
{
THROW_IF_SUSPENDED;
Modified: trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h (285085 => 285086)
--- trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h 2021-10-31 01:24:38 UTC (rev 285086)
@@ -291,6 +291,7 @@
- (void)_getMainResourceDataWithCompletionHandler:(void (^)(NSData *, NSError *))completionHandler;
- (void)_getWebArchiveDataWithCompletionHandler:(void (^)(NSData *, NSError *))completionHandler;
- (void)_getContentsAsStringWithCompletionHandler:(void (^)(NSString *, NSError *))completionHandler WK_API_AVAILABLE(macos(10.13), ios(11.0));
+- (void)_getContentsAsStringWithCompletionHandlerKeepIPCConnectionAliveForTesting:(void (^)(NSString *, NSError *))completionHandler;
- (void)_getContentsOfAllFramesAsStringWithCompletionHandler:(void (^)(NSString *))completionHandler WK_API_AVAILABLE(macos(11.0), ios(14.0));
- (void)_getContentsAsAttributedStringWithCompletionHandler:(void (^)(NSAttributedString *, NSDictionary<NSAttributedStringDocumentAttributeKey, id> *, NSError *))completionHandler WK_API_AVAILABLE(macos(10.15), ios(13.0));
Modified: trunk/Tools/ChangeLog (285085 => 285086)
--- trunk/Tools/ChangeLog 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Tools/ChangeLog 2021-10-31 01:24:38 UTC (rev 285086)
@@ -1,3 +1,15 @@
+2021-10-30 Chris Dumez <cdu...@apple.com>
+
+ Improve error handling in sendWithAsyncReply()
+ https://bugs.webkit.org/show_bug.cgi?id=232508
+
+ Reviewed by Darin Adler.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm:
+ (TEST):
+
2021-10-30 Sam Sneddon <gsnedd...@apple.com>
Ensure we stop LayoutTest servers without them becoming zombies
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm (285085 => 285086)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm 2021-10-31 01:18:06 UTC (rev 285085)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewGetContents.mm 2021-10-31 01:24:38 UTC (rev 285086)
@@ -49,6 +49,23 @@
TestWebKitAPI::Util::run(&finished);
}
+TEST(WKWebView, GetContentsShouldFailWhenClosingPage)
+{
+ auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+
+ [webView synchronouslyLoadTestPageNamed:@"simple"];
+
+ __block bool finished = false;
+
+ [webView _getContentsAsStringWithCompletionHandlerKeepIPCConnectionAliveForTesting:^(NSString *string, NSError *error) {
+ finished = true;
+ }];
+
+ [webView _close];
+
+ TestWebKitAPI::Util::run(&finished);
+}
+
TEST(WKWebView, GetContentsOfAllFramesShouldReturnString)
{
RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
