WKURLSchemeTaskImpl instances are being abandoned (except if the task is stopped).
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jul 2017 20:03:42 +0000 (20:03 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Jul 2017 20:03:42 +0000 (20:03 +0000)
<rdar://problem/33568276> and https://bugs.webkit.org/show_bug.cgi?id=174895

Reviewed by Alex Christensen.

There was a lot going on here:
- There was an unbroken cycle between URLSchemeHandlers and URLSchemeTasks
- WKURLSchemeTasks were not destroying their API::URLSchemeTask implementation object
- When a WKWebView was dealloc'ed, it was not cleaning up the related UIProcess tasks
- If the NetworkingProcess crashed, WebProcesses were not cleaning up their tasks
  (and therefore the UIProcess tasks were also not getting cleaned up)
- If a WebProcess crashed, the UIProcess was not cleaning up the related tasks

* UIProcess/API/Cocoa/WKURLSchemeTask.mm:
(-[WKURLSchemeTaskImpl dealloc]): Call API::~URLSchemeTask

* UIProcess/Cocoa/WebURLSchemeHandlerCocoa.h:
* UIProcess/Cocoa/WebURLSchemeHandlerCocoa.mm:
(WebKit::WebURLSchemeHandlerCocoa::platformTaskCompleted): Clear out the API wrapper map
  so the wrapper and implementation object can both be destroyed.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::close): Call stopAllURLSchemeTasks.
(WebKit::WebPageProxy::processDidTerminate): Call stopAllURLSchemeTasks.
(WebKit::WebPageProxy::stopAllURLSchemeTasks):
* UIProcess/WebPageProxy.h:

* UIProcess/WebURLSchemeHandler.cpp:
(WebKit::WebURLSchemeHandler::startTask):
(WebKit::WebURLSchemeHandler::stopAllTasksForPage):
(WebKit::WebURLSchemeHandler::stopTask):
(WebKit::WebURLSchemeHandler::taskCompleted):
* UIProcess/WebURLSchemeHandler.h:

* UIProcess/WebURLSchemeTask.cpp:
(WebKit::WebURLSchemeTask::WebURLSchemeTask):
* UIProcess/WebURLSchemeTask.h:
(WebKit::WebURLSchemeTask::pageID): Store separate from the WebPage so messaging
  is possible even after the WebPage* has been cleared.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::stopAllURLSchemeTasks):
* WebProcess/WebPage/WebPage.h:

* WebProcess/WebPage/WebURLSchemeHandlerProxy.cpp:
(WebKit::WebURLSchemeHandlerProxy::stopAllTasks):
(WebKit::WebURLSchemeHandlerProxy::taskDidComplete):
(WebKit::WebURLSchemeHandlerProxy::taskDidStopLoading):
(WebKit::WebURLSchemeHandlerProxy::removeTask):
* WebProcess/WebPage/WebURLSchemeHandlerProxy.h:

* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::networkProcessConnectionClosed): Call stopAllURLSchemeTasks
  for each WebPage.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@220011 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm
Source/WebKit/UIProcess/Cocoa/WebURLSchemeHandlerCocoa.h
Source/WebKit/UIProcess/Cocoa/WebURLSchemeHandlerCocoa.mm
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebURLSchemeHandler.cpp
Source/WebKit/UIProcess/WebURLSchemeHandler.h
Source/WebKit/UIProcess/WebURLSchemeTask.cpp
Source/WebKit/UIProcess/WebURLSchemeTask.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/WebURLSchemeHandlerProxy.cpp
Source/WebKit/WebProcess/WebPage/WebURLSchemeHandlerProxy.h
Source/WebKit/WebProcess/WebProcess.cpp

index 68d9c63..dd6a4b8 100644 (file)
@@ -1,3 +1,60 @@
+2017-07-28  Brady Eidson  <beidson@apple.com>
+
+        WKURLSchemeTaskImpl instances are being abandoned (except if the task is stopped).
+        <rdar://problem/33568276> and https://bugs.webkit.org/show_bug.cgi?id=174895
+
+        Reviewed by Alex Christensen.
+
+        There was a lot going on here:
+        - There was an unbroken cycle between URLSchemeHandlers and URLSchemeTasks 
+        - WKURLSchemeTasks were not destroying their API::URLSchemeTask implementation object
+        - When a WKWebView was dealloc'ed, it was not cleaning up the related UIProcess tasks
+        - If the NetworkingProcess crashed, WebProcesses were not cleaning up their tasks
+          (and therefore the UIProcess tasks were also not getting cleaned up)
+        - If a WebProcess crashed, the UIProcess was not cleaning up the related tasks
+        
+        * UIProcess/API/Cocoa/WKURLSchemeTask.mm:
+        (-[WKURLSchemeTaskImpl dealloc]): Call API::~URLSchemeTask
+        
+        * UIProcess/Cocoa/WebURLSchemeHandlerCocoa.h:
+        * UIProcess/Cocoa/WebURLSchemeHandlerCocoa.mm:
+        (WebKit::WebURLSchemeHandlerCocoa::platformTaskCompleted): Clear out the API wrapper map
+          so the wrapper and implementation object can both be destroyed.
+        
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::close): Call stopAllURLSchemeTasks.
+        (WebKit::WebPageProxy::processDidTerminate): Call stopAllURLSchemeTasks.
+        (WebKit::WebPageProxy::stopAllURLSchemeTasks):
+        * UIProcess/WebPageProxy.h:
+        
+        * UIProcess/WebURLSchemeHandler.cpp:
+        (WebKit::WebURLSchemeHandler::startTask):
+        (WebKit::WebURLSchemeHandler::stopAllTasksForPage):
+        (WebKit::WebURLSchemeHandler::stopTask):
+        (WebKit::WebURLSchemeHandler::taskCompleted):
+        * UIProcess/WebURLSchemeHandler.h:
+        
+        * UIProcess/WebURLSchemeTask.cpp:
+        (WebKit::WebURLSchemeTask::WebURLSchemeTask):
+        * UIProcess/WebURLSchemeTask.h:
+        (WebKit::WebURLSchemeTask::pageID): Store separate from the WebPage so messaging
+          is possible even after the WebPage* has been cleared.
+        
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::stopAllURLSchemeTasks):
+        * WebProcess/WebPage/WebPage.h:
+        
+        * WebProcess/WebPage/WebURLSchemeHandlerProxy.cpp:
+        (WebKit::WebURLSchemeHandlerProxy::stopAllTasks):
+        (WebKit::WebURLSchemeHandlerProxy::taskDidComplete):
+        (WebKit::WebURLSchemeHandlerProxy::taskDidStopLoading):
+        (WebKit::WebURLSchemeHandlerProxy::removeTask):
+        * WebProcess/WebPage/WebURLSchemeHandlerProxy.h:
+        
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::networkProcessConnectionClosed): Call stopAllURLSchemeTasks 
+          for each WebPage.
+
 2017-07-28  Frederic Wang  <fwang@igalia.cpm>
 
         Fix typo in scrollPositionChangedViaDelegatedScrolling
index f0deea5..51f8179 100644 (file)
@@ -28,6 +28,7 @@
 
 #if WK_API_ENABLED
 
+#import "WebURLSchemeHandler.h"
 #import "WebURLSchemeTask.h"
 #import <WebCore/ResourceError.h>
 #import <WebCore/ResourceResponse.h>
@@ -61,6 +62,13 @@ static void raiseExceptionIfNecessary(WebKit::WebURLSchemeTask::ExceptionType ex
 
 @implementation WKURLSchemeTaskImpl
 
+- (void)dealloc
+{
+    _urlSchemeTask->API::URLSchemeTask::~URLSchemeTask();
+
+    [super dealloc];
+}
+
 - (NSURLRequest *)request
 {
     return _urlSchemeTask->task().request().nsURLRequest(DoNotUpdateHTTPBody);
index 2590af3..ca8f5b7 100644 (file)
@@ -47,6 +47,7 @@ private:
 
     void platformStartTask(WebPageProxy&, WebURLSchemeTask&) final;
     void platformStopTask(WebPageProxy&, WebURLSchemeTask&) final;
+    void platformTaskCompleted(WebURLSchemeTask&) final;
 
     RetainPtr<id <WKURLSchemeHandler>> m_apiHandler;
     HashMap<uint64_t, Ref<API::URLSchemeTask>> m_apiTasks;
index b84cc64..1439c3e 100644 (file)
@@ -76,4 +76,13 @@ void WebURLSchemeHandlerCocoa::platformStopTask(WebPageProxy& page, WebURLScheme
 #endif
 }
 
+void WebURLSchemeHandlerCocoa::platformTaskCompleted(WebURLSchemeTask& task)
+{
+#if WK_API_ENABLED
+    m_apiTasks.remove(task.identifier());
+#else
+    UNUSED_PARAM(task);
+#endif
+}
+
 } // namespace WebKit
index 8b06edc..22afa23 100644 (file)
@@ -781,6 +781,8 @@ void WebPageProxy::close()
     // Make sure we don't hold a process assertion after getting closed.
     m_activityToken = nullptr;
 #endif
+
+    stopAllURLSchemeTasks();
 }
 
 bool WebPageProxy::tryClose()
@@ -5355,6 +5357,18 @@ void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
         if (auto* automationSession = process().processPool().automationSession())
             automationSession->terminate();
     }
+
+    stopAllURLSchemeTasks();
+}
+
+void WebPageProxy::stopAllURLSchemeTasks()
+{
+    HashSet<WebURLSchemeHandler*> handlers;
+    for (auto& handler : m_urlSchemeHandlersByScheme.values())
+        handlers.add(handler.ptr());
+
+    for (auto* handler : handlers)
+        handler->stopAllTasksForPage(*this);
 }
 
 #if PLATFORM(IOS)
index 3619e43..7cc6693 100644 (file)
@@ -1623,6 +1623,8 @@ private:
 
     void viewIsBecomingVisible();
 
+    void stopAllURLSchemeTasks();
+
     PageClient& m_pageClient;
     Ref<API::PageConfiguration> m_configuration;
 
index 490855b..4a8233e 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "WebURLSchemeHandler.h"
 
+#include "WebPageProxy.h"
 #include "WebURLSchemeTask.h"
 
 using namespace WebCore;
@@ -53,20 +54,54 @@ void WebURLSchemeHandler::startTask(WebPageProxy& page, uint64_t taskIdentifier,
     auto result = m_tasks.add(taskIdentifier, WebURLSchemeTask::create(*this, page, taskIdentifier, request));
     ASSERT(result.isNewEntry);
 
+    auto pageEntry = m_tasksByPageIdentifier.add(page.pageID(), HashSet<uint64_t>());
+    ASSERT(!pageEntry.iterator->value.contains(taskIdentifier));
+    pageEntry.iterator->value.add(taskIdentifier);
+
     platformStartTask(page, result.iterator->value);
 }
 
+void WebURLSchemeHandler::stopAllTasksForPage(WebPageProxy& page)
+{
+    auto iterator = m_tasksByPageIdentifier.find(page.pageID());
+    if (iterator == m_tasksByPageIdentifier.end())
+        return;
+
+    auto& tasksByPage = iterator->value;
+    while (!tasksByPage.isEmpty())
+        stopTask(page, *tasksByPage.begin());
+
+    ASSERT(m_tasksByPageIdentifier.find(page.pageID()) == m_tasksByPageIdentifier.end());
+}
+
 void WebURLSchemeHandler::stopTask(WebPageProxy& page, uint64_t taskIdentifier)
 {
     auto iterator = m_tasks.find(taskIdentifier);
     if (iterator == m_tasks.end())
         return;
 
-    iterator->value->stop();
+    auto pageIterator = m_tasksByPageIdentifier.find(page.pageID());
+    ASSERT(pageIterator != m_tasksByPageIdentifier.end());
+    ASSERT(pageIterator->value.contains(taskIdentifier));
+    pageIterator->value.remove(taskIdentifier);
 
+    iterator->value->stop();
     platformStopTask(page, iterator->value);
 
     m_tasks.remove(iterator);
+    if (pageIterator->value.isEmpty())
+        m_tasksByPageIdentifier.remove(pageIterator);
+}
+
+void WebURLSchemeHandler::taskCompleted(WebURLSchemeTask& task)
+{
+    auto takenTask = m_tasks.take(task.identifier());
+    ASSERT_UNUSED(takenTask, takenTask->ptr() == &task);
+
+    ASSERT(m_tasksByPageIdentifier.get(task.pageID()).contains(task.identifier()));
+    m_tasksByPageIdentifier.get(task.pageID()).remove(task.identifier());
+
+    platformTaskCompleted(task);
 }
 
 } // namespace WebKit
index 7179dcb..6b76405 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "WebURLSchemeTask.h"
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 
@@ -47,6 +48,8 @@ public:
 
     void startTask(WebPageProxy&, uint64_t taskIdentifier, const WebCore::ResourceRequest&);
     void stopTask(WebPageProxy&, uint64_t taskIdentifier);
+    void stopAllTasksForPage(WebPageProxy&);
+    void taskCompleted(WebURLSchemeTask&);
 
 protected:
     WebURLSchemeHandler();
@@ -54,10 +57,12 @@ protected:
 private:
     virtual void platformStartTask(WebPageProxy&, WebURLSchemeTask&) = 0;
     virtual void platformStopTask(WebPageProxy&, WebURLSchemeTask&) = 0;
+    virtual void platformTaskCompleted(WebURLSchemeTask&) = 0;
 
     uint64_t m_identifier;
 
     HashMap<uint64_t, Ref<WebURLSchemeTask>> m_tasks;
+    HashMap<uint64_t, HashSet<uint64_t>> m_tasksByPageIdentifier;
 
 }; // class WebURLSchemeHandler
 
index 3739485..304eef8 100644 (file)
@@ -44,6 +44,7 @@ WebURLSchemeTask::WebURLSchemeTask(WebURLSchemeHandler& handler, WebPageProxy& p
     : m_urlSchemeHandler(handler)
     , m_page(&page)
     , m_identifier(resourceIdentifier)
+    , m_pageIdentifier(page.pageID())
     , m_request(request)
 {
 }
@@ -115,6 +116,8 @@ auto WebURLSchemeTask::didComplete(const ResourceError& error) -> ExceptionType
 
     m_completed = true;
     m_page->send(Messages::WebPage::URLSchemeTaskDidComplete(m_urlSchemeHandler->identifier(), m_identifier, error));
+    m_urlSchemeHandler->taskCompleted(*this);
+
     return ExceptionType::None;
 }
 
index c6f92c8..691b439 100644 (file)
@@ -46,6 +46,7 @@ public:
     static Ref<WebURLSchemeTask> create(WebURLSchemeHandler&, WebPageProxy&, uint64_t identifier, const WebCore::ResourceRequest&);
 
     uint64_t identifier() const { return m_identifier; }
+    uint64_t pageID() const { return m_pageIdentifier; }
 
     const WebCore::ResourceRequest& request() const { return m_request; }
 
@@ -71,6 +72,7 @@ private:
     Ref<WebURLSchemeHandler> m_urlSchemeHandler;
     WebPageProxy* m_page;
     uint64_t m_identifier;
+    uint64_t m_pageIdentifier;
     WebCore::ResourceRequest m_request;
     bool m_stopped { false };
     bool m_responseSent { false };
index 1c7a8fe..e13d2b9 100644 (file)
@@ -5908,6 +5908,16 @@ WebURLSchemeHandlerProxy* WebPage::urlSchemeHandlerForScheme(const String& schem
     return m_schemeToURLSchemeHandlerProxyMap.get(scheme);
 }
 
+void WebPage::stopAllURLSchemeTasks()
+{
+    HashSet<WebURLSchemeHandlerProxy*> handlers;
+    for (auto& handler : m_schemeToURLSchemeHandlerProxyMap.values())
+        handlers.add(handler.get());
+
+    for (auto* handler : handlers)
+        handler->stopAllTasks();
+}
+
 void WebPage::registerURLSchemeHandler(uint64_t handlerIdentifier, const String& scheme)
 {
     auto schemeResult = m_schemeToURLSchemeHandlerProxyMap.add(scheme, WebURLSchemeHandlerProxy::create(*this, handlerIdentifier));
index ad1bec4..6bc371e 100644 (file)
@@ -979,6 +979,8 @@ public:
 #endif
 
     WebURLSchemeHandlerProxy* urlSchemeHandlerForScheme(const String&);
+    void stopAllURLSchemeTasks();
+
     std::optional<double> cpuLimit() const { return m_cpuLimit; }
 
     static PluginView* pluginViewForFrame(WebCore::Frame*);
index 5afb754..cdb43fb 100644 (file)
@@ -55,6 +55,12 @@ void WebURLSchemeHandlerProxy::startNewTask(ResourceLoader& loader)
     result.iterator->value->startLoading();
 }
 
+void WebURLSchemeHandlerProxy::stopAllTasks()
+{
+    while (!m_tasks.isEmpty())
+        m_tasks.begin()->value->stopLoading();
+}
+
 void WebURLSchemeHandlerProxy::taskDidPerformRedirection(uint64_t taskIdentifier, WebCore::ResourceResponse&& redirectResponse, WebCore::ResourceRequest&& newRequest)
 {
     auto* task = m_tasks.get(taskIdentifier);
@@ -84,18 +90,25 @@ void WebURLSchemeHandlerProxy::taskDidReceiveData(uint64_t taskIdentifier, size_
 
 void WebURLSchemeHandlerProxy::taskDidComplete(uint64_t taskIdentifier, const ResourceError& error)
 {
-    auto task = m_tasks.take(taskIdentifier);
-    if (!task)
-        return;
-
-    WebProcess::singleton().webLoaderStrategy().removeURLSchemeTaskProxy(*task);
-    task->didComplete(error);
+    if (auto task = removeTask(taskIdentifier))
+        task->didComplete(error);
 }
 
 void WebURLSchemeHandlerProxy::taskDidStopLoading(WebURLSchemeTaskProxy& task)
 {
     ASSERT(m_tasks.get(task.identifier()) == &task);
-    m_tasks.remove(task.identifier());
+    removeTask(task.identifier());
+}
+
+RefPtr<WebURLSchemeTaskProxy> WebURLSchemeHandlerProxy::removeTask(uint64_t identifier)
+{
+    auto task = m_tasks.take(identifier);
+    if (!task)
+        return nullptr;
+
+    WebProcess::singleton().webLoaderStrategy().removeURLSchemeTaskProxy(*task);
+
+    return task;
 }
 
 } // namespace WebKit
index 3138676..742415c 100644 (file)
@@ -49,6 +49,7 @@ public:
     ~WebURLSchemeHandlerProxy();
 
     void startNewTask(WebCore::ResourceLoader&);
+    void stopAllTasks();
 
     uint64_t identifier() const { return m_identifier; }
     WebPage& page() { return m_webPage; }
@@ -61,6 +62,9 @@ public:
 
 private:
     WebURLSchemeHandlerProxy(WebPage&, uint64_t identifier);
+
+    RefPtr<WebURLSchemeTaskProxy> removeTask(uint64_t identifier);
+
     WebPage& m_webPage;
     uint64_t m_identifier { 0 };
 
index d400a71..6bab545 100644 (file)
@@ -1129,6 +1129,9 @@ void WebProcess::networkProcessConnectionClosed(NetworkProcessConnection* connec
 
     m_webLoaderStrategy.networkProcessCrashed();
     WebSocketStream::networkProcessCrashed();
+
+    for (auto& page : m_pageMap.values())
+        page->stopAllURLSchemeTasks();
 }
 
 WebLoaderStrategy& WebProcess::webLoaderStrategy()