Attachment 368646 Details for Bug 197426 – Patch

[patch] Patch

bug-197426-20190430190438.patch (text/plain), 13.64 KB, created by Chris Dumez on 2019-04-30 19:04:40 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-04-30 19:04:40 PDT

Size: 13.64 KB

patch

obsolete

>Subversion Revision: 244813
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 8a95139dea2760c7d45676fdfadc6da4bb82e93e..a0aac73f2e6667392c292d156dc4e24ecb2fc797 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-04-30  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) URL scheme handlers can no longer respond asynchronously
>+        https://bugs.webkit.org/show_bug.cgi?id=197426
>+        <rdar://problem/50256169>
>+
>+        Reviewed by Brady Eidson.
>+
>+        The issue was that when committing the provisional process, we would call WebPageProxy::processDidTerminate()
>+        which would call WebPageProxy::stopAllURLSchemeTasks(). This would terminate all URL scheme tasks associated
>+        with the page, including the one associated with the provisisional page / process.
>+
>+        To address the issue, pass m_process to stopAllURLSchemeTasks() in processDidTerminate() and only stop the
>+        tasks associated with the m_process (which is the process we're about to swap away from).
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::processDidTerminate):
>+        (WebKit::WebPageProxy::stopAllURLSchemeTasks):
>+        * UIProcess/WebPageProxy.h:
>+        * UIProcess/WebURLSchemeHandler.cpp:
>+        (WebKit::WebURLSchemeHandler::processForTaskIdentifier):
>+        (WebKit::WebURLSchemeHandler::stopAllTasksForPage):
>+        * UIProcess/WebURLSchemeHandler.h:
>+        * UIProcess/WebURLSchemeTask.h:
>+        (WebKit::WebURLSchemeTask::process const):
>+
> 2019-04-30  Chris Dumez  <cdumez@apple.com>
> 
>         Unreviewed, rolling out r244802.
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 323db28d6e9ba134444e8a02c7046bf05ae66438..5324051b1040e543b6e6da62c38dd9ea7397b714 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -6725,6 +6725,7 @@ void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
>     PageLoadState::Transaction transaction = m_pageLoadState.transaction();
> 
>     resetStateAfterProcessExited(reason);
>+    stopAllURLSchemeTasks(m_process.ptr());
> 
>     // For bringup of process swapping, NavigationSwap termination will not go out to clients.
>     // If it does *during* process swapping, and the client triggers a reload, that causes bizarre WebKit re-entry.
>@@ -6740,8 +6741,6 @@ void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
>         if (auto* automationSession = process().processPool().automationSession())
>             automationSession->terminate();
>     }
>-
>-    stopAllURLSchemeTasks();
> }
> 
> void WebPageProxy::provisionalProcessDidTerminate()
>@@ -6801,14 +6800,14 @@ void WebPageProxy::resetRecentCrashCount()
>     m_recentCrashCount = 0;
> }
> 
>-void WebPageProxy::stopAllURLSchemeTasks()
>+void WebPageProxy::stopAllURLSchemeTasks(WebProcessProxy* process)
> {
>     HashSet<WebURLSchemeHandler*> handlers;
>     for (auto& handler : m_urlSchemeHandlersByScheme.values())
>         handlers.add(handler.ptr());
> 
>     for (auto* handler : handlers)
>-        handler->stopAllTasksForPage(*this);
>+        handler->stopAllTasksForPage(*this, process);
> }
> 
> #if PLATFORM(IOS_FAMILY)
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index 91611eed525327557a2336465174996ef8720811..37658d8c0707e2a34cfdedf05088f4320c9a9655 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -2001,7 +2001,7 @@ private:
> 
>     void viewIsBecomingVisible();
> 
>-    void stopAllURLSchemeTasks();
>+    void stopAllURLSchemeTasks(WebProcessProxy* = nullptr);
> 
>     void clearInspectorTargets();
>     void createInspectorTargets();
>diff --git a/Source/WebKit/UIProcess/WebURLSchemeHandler.cpp b/Source/WebKit/UIProcess/WebURLSchemeHandler.cpp
>index 5ba7fcef8e438bd2d77bf2ee7597fc1b9a18751a..e355319ea4c4ee651c6bedf7c86fbdd21f29dc23 100644
>--- a/Source/WebKit/UIProcess/WebURLSchemeHandler.cpp
>+++ b/Source/WebKit/UIProcess/WebURLSchemeHandler.cpp
>@@ -60,17 +60,31 @@ void WebURLSchemeHandler::startTask(WebPageProxy& page, WebProcessProxy& process
>     platformStartTask(page, result.iterator->value);
> }
> 
>-void WebURLSchemeHandler::stopAllTasksForPage(WebPageProxy& page)
>+WebProcessProxy* WebURLSchemeHandler::processForTaskIdentifier(uint64_t taskIdentifier) const
>+{
>+    auto iterator = m_tasks.find(taskIdentifier);
>+    if (iterator == m_tasks.end())
>+        return nullptr;
>+    return iterator->value->process();
>+}
>+
>+void WebURLSchemeHandler::stopAllTasksForPage(WebPageProxy& page, WebProcessProxy* process)
> {
>     auto iterator = m_tasksByPageIdentifier.find(page.pageID());
>     if (iterator == m_tasksByPageIdentifier.end())
>         return;
> 
>     auto& tasksByPage = iterator->value;
>-    while (!tasksByPage.isEmpty())
>-        stopTask(page, *tasksByPage.begin());
>+    Vector<uint64_t> taskIdentifiersToStop;
>+    taskIdentifiersToStop.reserveInitialCapacity(tasksByPage.size());
>+    for (auto taskIdentifier : tasksByPage) {
>+        if (!process || processForTaskIdentifier(taskIdentifier) == process)
>+            taskIdentifiersToStop.uncheckedAppend(taskIdentifier);
>+    }
>+
>+    for (auto& taskIdentifier : taskIdentifiersToStop)
>+        stopTask(page, taskIdentifier);
> 
>-    ASSERT(m_tasksByPageIdentifier.find(page.pageID()) == m_tasksByPageIdentifier.end());
> }
> 
> void WebURLSchemeHandler::stopTask(WebPageProxy& page, uint64_t taskIdentifier)
>diff --git a/Source/WebKit/UIProcess/WebURLSchemeHandler.h b/Source/WebKit/UIProcess/WebURLSchemeHandler.h
>index ac812f25c16e1997c9e342d88b5f26b8b9a5fd54..89976c61f83d52b81f2c4407c2c5bbd73bf2ae49 100644
>--- a/Source/WebKit/UIProcess/WebURLSchemeHandler.h
>+++ b/Source/WebKit/UIProcess/WebURLSchemeHandler.h
>@@ -55,7 +55,7 @@ public:
> 
>     void startTask(WebPageProxy&, WebProcessProxy&, uint64_t taskIdentifier, WebCore::ResourceRequest&&, SyncLoadCompletionHandler&&);
>     void stopTask(WebPageProxy&, uint64_t taskIdentifier);
>-    void stopAllTasksForPage(WebPageProxy&);
>+    void stopAllTasksForPage(WebPageProxy&, WebProcessProxy*);
>     void taskCompleted(WebURLSchemeTask&);
> 
> protected:
>@@ -67,6 +67,7 @@ private:
>     virtual void platformTaskCompleted(WebURLSchemeTask&) = 0;
> 
>     void removeTaskFromPageMap(uint64_t pageID, uint64_t taskID);
>+    WebProcessProxy* processForTaskIdentifier(uint64_t) const;
> 
>     uint64_t m_identifier;
> 
>diff --git a/Source/WebKit/UIProcess/WebURLSchemeTask.h b/Source/WebKit/UIProcess/WebURLSchemeTask.h
>index d48a408e4902426850b15688d3662b72c8679dad..b872073bd2781ff09b9bf2533162c05aa69c03eb 100644
>--- a/Source/WebKit/UIProcess/WebURLSchemeTask.h
>+++ b/Source/WebKit/UIProcess/WebURLSchemeTask.h
>@@ -58,6 +58,7 @@ public:
> 
>     uint64_t identifier() const { return m_identifier; }
>     uint64_t pageID() const { return m_pageIdentifier; }
>+    WebProcessProxy* process() const { return m_process.get(); }
> 
>     const WebCore::ResourceRequest& request() const { return m_request; }
> 
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index b3003d0285a60c5a67101e719c398a3d4d6f746c..d068d7d9536fb2140feb17db8c8941d2eaa4f983 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-04-30  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) URL scheme handlers can no longer respond asynchronously
>+        https://bugs.webkit.org/show_bug.cgi?id=197426
>+        <rdar://problem/50256169>
>+
>+        Reviewed by Brady Eidson.
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+        (-[PSONScheme setShouldRespondAsynchronously:]):
>+        (-[PSONScheme webView:startURLSchemeTask:]):
>+        (-[PSONScheme webView:stopURLSchemeTask:]):
>+
> 2019-04-30  Chris Dumez  <cdumez@apple.com>
> 
>         Unreviewed, rolling out r244802.
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 7f5ddea5abca477763d277948fd1e2c7776d06a6..436ac525bc5b6346b2bb0172f04d5298cd9bf013 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -48,6 +48,7 @@
> #import <WebKit/_WKProcessPoolConfiguration.h>
> #import <WebKit/_WKWebsiteDataStoreConfiguration.h>
> #import <WebKit/_WKWebsitePolicies.h>
>+#import <wtf/BlockPtr.h>
> #import <wtf/Deque.h>
> #import <wtf/HashMap.h>
> #import <wtf/HashSet.h>
>@@ -238,6 +239,8 @@ @interface PSONScheme : NSObject <WKURLSchemeHandler> {
>     const char* _bytes;
>     HashMap<String, String> _redirects;
>     HashMap<String, RetainPtr<NSData>> _dataMappings;
>+    HashSet<id <WKURLSchemeTask>> _runningTasks;
>+    bool _shouldRespondAsynchronously;
> }
> - (instancetype)initWithBytes:(const char*)bytes;
> - (void)addRedirectFromURLString:(NSString *)sourceURLString toURLString:(NSString *)destinationURLString;
>@@ -263,8 +266,29 @@ - (void)addMappingFromURLString:(NSString *)urlString toData:(const char*)data
>     _dataMappings.set(urlString, [NSData dataWithBytesNoCopy:(void*)data length:strlen(data) freeWhenDone:NO]);
> }
> 
>+- (void)setShouldRespondAsynchronously:(BOOL)value
>+{
>+    _shouldRespondAsynchronously = value;
>+}
>+
> - (void)webView:(WKWebView *)webView startURLSchemeTask:(id <WKURLSchemeTask>)task
> {
>+    if ([(id<WKURLSchemeTaskPrivate>)task _requestOnlyIfCached]) {
>+        [task didFailWithError:[NSError errorWithDomain:@"TestWebKitAPI" code:1 userInfo:nil]];
>+        return;
>+    }
>+
>+    _runningTasks.add(task);
>+
>+    auto doAsynchronouslyIfNecessary = [self, strongSelf = retainPtr(self), task = retainPtr(task)](Function<void(id <WKURLSchemeTask>)>&& f, double delay) {
>+        if (!_shouldRespondAsynchronously)
>+            return f(task.get());
>+        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, delay * NSEC_PER_SEC), dispatch_get_main_queue(), makeBlockPtr([self, strongSelf, task, f = WTFMove(f)] {
>+            if (_runningTasks.contains(task.get()))
>+                f(task.get());
>+        }).get());
>+    };
>+
>     NSURL *finalURL = task.request.URL;
>     auto target = _redirects.get(task.request.URL.absoluteString);
>     if (!target.isEmpty()) {
>@@ -276,27 +300,30 @@ - (void)webView:(WKWebView *)webView startURLSchemeTask:(id <WKURLSchemeTask>)ta
>         [(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()];
>     }
> 
>-    if ([(id<WKURLSchemeTaskPrivate>)task _requestOnlyIfCached]) {
>-        [task didFailWithError:[NSError errorWithDomain:@"TestWebKitAPI" code:1 userInfo:nil]];
>-        return;
>-    }
>-
>-    RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:finalURL MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]);
>-    [task didReceiveResponse:response.get()];
>+    doAsynchronouslyIfNecessary([finalURL = retainPtr(finalURL)](id <WKURLSchemeTask> task) {
>+        RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:finalURL.get() MIMEType:@"text/html" expectedContentLength:1 textEncodingName:nil]);
>+        [task didReceiveResponse:response.get()];
>+    }, 0.1);
> 
>-    if (auto data = _dataMappings.get([finalURL absoluteString]))
>-        [task didReceiveData:data.get()];
>-    else if (_bytes) {
>-        RetainPtr<NSData> data = adoptNS([[NSData alloc] initWithBytesNoCopy:(void *)_bytes length:strlen(_bytes) freeWhenDone:NO]);
>-        [task didReceiveData:data.get()];
>-    } else
>-        [task didReceiveData:[@"Hello" dataUsingEncoding:NSUTF8StringEncoding]];
>+    doAsynchronouslyIfNecessary([self, finalURL = retainPtr(finalURL)](id <WKURLSchemeTask> task) {
>+        if (auto data = _dataMappings.get([finalURL absoluteString]))
>+            [task didReceiveData:data.get()];
>+        else if (_bytes) {
>+            RetainPtr<NSData> data = adoptNS([[NSData alloc] initWithBytesNoCopy:(void *)_bytes length:strlen(_bytes) freeWhenDone:NO]);
>+            [task didReceiveData:data.get()];
>+        } else
>+            [task didReceiveData:[@"Hello" dataUsingEncoding:NSUTF8StringEncoding]];
>+    }, 0.2);
> 
>-    [task didFinish];
>+    doAsynchronouslyIfNecessary([self](id <WKURLSchemeTask> task) {
>+        [task didFinish];
>+        _runningTasks.remove(task);
>+    }, 0.3);
> }
> 
> - (void)webView:(WKWebView *)webView stopURLSchemeTask:(id <WKURLSchemeTask>)task
> {
>+    _runningTasks.remove(task);
> }
> 
> @end
>@@ -471,7 +498,8 @@ static RetainPtr<_WKProcessPoolConfiguration> psonProcessPoolConfiguration()
>     return processPoolConfiguration;
> }
> 
>-TEST(ProcessSwap, Basic)
>+enum class SchemeHandlerShouldBeAsync { No, Yes };
>+static void runBasicTest(SchemeHandlerShouldBeAsync schemeHandlerShouldBeAsync)
> {
>     auto processPoolConfiguration = psonProcessPoolConfiguration();
>     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
>@@ -479,6 +507,7 @@ TEST(ProcessSwap, Basic)
>     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
>     [webViewConfiguration setProcessPool:processPool.get()];
>     auto handler = adoptNS([[PSONScheme alloc] init]);
>+    [handler setShouldRespondAsynchronously:(schemeHandlerShouldBeAsync == SchemeHandlerShouldBeAsync::Yes)];
>     [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
> 
>     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
>@@ -516,6 +545,16 @@ TEST(ProcessSwap, Basic)
>     EXPECT_EQ(numberOfDecidePolicyCalls, 3);
> }
> 
>+TEST(ProcessSwap, Basic)
>+{
>+    runBasicTest(SchemeHandlerShouldBeAsync::No);
>+}
>+
>+TEST(ProcessSwap, BasicWithAsyncSchemeHandler)
>+{
>+    runBasicTest(SchemeHandlerShouldBeAsync::Yes);
>+}
>+
> TEST(ProcessSwap, LoadAfterPolicyDecision)
> {
>     auto processPoolConfiguration = psonProcessPoolConfiguration();

Actions: View | Formatted Diff | Diff

Attachments on bug 197426: 368587 | 368646