iBooks sometimes crashes when closing a book.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jul 2017 20:42:21 +0000 (20:42 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jul 2017 20:42:21 +0000 (20:42 +0000)
<rdar://problem/31180331> and https://bugs.webkit.org/show_bug.cgi?id=174658

Reviewed by Oliver Hunt.

Source/WebKit:

- LegacyCustomProtocolManagerProxy should not reference a WebProcessPool directly.
- LegacyCustomProtocolManagerProxy should invalidate in its destructor.

* UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.cpp:
(WebKit::LegacyCustomProtocolManagerProxy::LegacyCustomProtocolManagerProxy):
(WebKit::LegacyCustomProtocolManagerProxy::~LegacyCustomProtocolManagerProxy):
(WebKit::LegacyCustomProtocolManagerProxy::startLoading):
(WebKit::LegacyCustomProtocolManagerProxy::stopLoading):
(WebKit::LegacyCustomProtocolManagerProxy::invalidate):
(WebKit::LegacyCustomProtocolManagerProxy::wasRedirectedToRequest):
(WebKit::LegacyCustomProtocolManagerProxy::didReceiveResponse):
(WebKit::LegacyCustomProtocolManagerProxy::didLoadData):
(WebKit::LegacyCustomProtocolManagerProxy::didFailWithError):
(WebKit::LegacyCustomProtocolManagerProxy::didFinishLoading):
(WebKit::LegacyCustomProtocolManagerProxy::processDidClose): Deleted.
* UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.h:

* UIProcess/Network/NetworkProcessProxy.cpp:
(WebKit::NetworkProcessProxy::NetworkProcessProxy):
(WebKit::NetworkProcessProxy::didClose):
* UIProcess/Network/NetworkProcessProxy.h:
(WebKit::NetworkProcessProxy::processPool):

Tools:

* TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:
(-[ProcessPoolDestroyedDuringLoadingProtocol startLoading]):
(-[ProcessPoolDestroyedDuringLoadingProtocol finishTheLoad]):
(-[ProcessPoolDestroyedDuringLoadingProtocol stopLoading]):
(TestWebKitAPI::TEST):

Add a "spin the runloop X number of times" utility:
* TestWebKitAPI/Utilities.h:
* TestWebKitAPI/cocoa/UtilitiesCocoa.mm:
(TestWebKitAPI::Util::spinRunLoop):

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Cocoa/LegacyCustomProtocolManagerClient.mm
Source/WebKit/UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.cpp
Source/WebKit/UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.h
Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm
Tools/TestWebKitAPI/Utilities.h
Tools/TestWebKitAPI/cocoa/UtilitiesCocoa.mm

index 70c7894..a9d0606 100644 (file)
@@ -1,3 +1,33 @@
+2017-07-19  Brady Eidson  <beidson@apple.com>
+
+        iBooks sometimes crashes when closing a book.
+        <rdar://problem/31180331> and https://bugs.webkit.org/show_bug.cgi?id=174658
+
+        Reviewed by Oliver Hunt.
+
+        - LegacyCustomProtocolManagerProxy should not reference a WebProcessPool directly.
+        - LegacyCustomProtocolManagerProxy should invalidate in its destructor.
+
+        * UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.cpp:
+        (WebKit::LegacyCustomProtocolManagerProxy::LegacyCustomProtocolManagerProxy):
+        (WebKit::LegacyCustomProtocolManagerProxy::~LegacyCustomProtocolManagerProxy):
+        (WebKit::LegacyCustomProtocolManagerProxy::startLoading):
+        (WebKit::LegacyCustomProtocolManagerProxy::stopLoading):
+        (WebKit::LegacyCustomProtocolManagerProxy::invalidate):
+        (WebKit::LegacyCustomProtocolManagerProxy::wasRedirectedToRequest):
+        (WebKit::LegacyCustomProtocolManagerProxy::didReceiveResponse):
+        (WebKit::LegacyCustomProtocolManagerProxy::didLoadData):
+        (WebKit::LegacyCustomProtocolManagerProxy::didFailWithError):
+        (WebKit::LegacyCustomProtocolManagerProxy::didFinishLoading):
+        (WebKit::LegacyCustomProtocolManagerProxy::processDidClose): Deleted.
+        * UIProcess/Network/CustomProtocols/LegacyCustomProtocolManagerProxy.h:
+
+        * UIProcess/Network/NetworkProcessProxy.cpp:
+        (WebKit::NetworkProcessProxy::NetworkProcessProxy):
+        (WebKit::NetworkProcessProxy::didClose):
+        * UIProcess/Network/NetworkProcessProxy.h:
+        (WebKit::NetworkProcessProxy::processPool):
+
 2017-07-19  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Implement WTF::ThreadGroup
index 1654e9f..49536c6 100644 (file)
@@ -148,8 +148,11 @@ void LegacyCustomProtocolManagerClient::stopLoading(LegacyCustomProtocolManagerP
 
 void LegacyCustomProtocolManagerClient::invalidate(LegacyCustomProtocolManagerProxy&)
 {
-    for (auto& loader : m_loaderMap)
-        [loader.value customProtocolManagerProxyDestroyed];
+    while (!m_loaderMap.isEmpty()) {
+        auto loader = m_loaderMap.take(m_loaderMap.begin()->key);
+        ASSERT(loader);
+        [loader customProtocolManagerProxyDestroyed];
+    }
 }
 
 } // namespace WebKit
index 40697f7..711273a 100644 (file)
 #include "LegacyCustomProtocolManagerProxy.h"
 
 #include "APICustomProtocolManagerClient.h"
-#include "ChildProcessProxy.h"
 #include "LegacyCustomProtocolManagerMessages.h"
 #include "LegacyCustomProtocolManagerProxyMessages.h"
+#include "NetworkProcessProxy.h"
 #include "WebProcessPool.h"
 #include <WebCore/ResourceRequest.h>
 
 namespace WebKit {
 
-LegacyCustomProtocolManagerProxy::LegacyCustomProtocolManagerProxy(ChildProcessProxy* childProcessProxy, WebProcessPool& processPool)
-    : m_childProcessProxy(childProcessProxy)
-    , m_processPool(processPool)
+LegacyCustomProtocolManagerProxy::LegacyCustomProtocolManagerProxy(NetworkProcessProxy& networkProcessProxy)
+    : m_networkProcessProxy(networkProcessProxy)
 {
-    ASSERT(m_childProcessProxy);
-    m_childProcessProxy->addMessageReceiver(Messages::LegacyCustomProtocolManagerProxy::messageReceiverName(), *this);
+    m_networkProcessProxy.addMessageReceiver(Messages::LegacyCustomProtocolManagerProxy::messageReceiverName(), *this);
 }
 
 LegacyCustomProtocolManagerProxy::~LegacyCustomProtocolManagerProxy()
 {
-    m_childProcessProxy->removeMessageReceiver(Messages::LegacyCustomProtocolManagerProxy::messageReceiverName());
+    m_networkProcessProxy.removeMessageReceiver(Messages::LegacyCustomProtocolManagerProxy::messageReceiverName());
+    invalidate();
 }
 
 void LegacyCustomProtocolManagerProxy::startLoading(uint64_t customProtocolID, const WebCore::ResourceRequest& request)
 {
-    m_processPool.customProtocolManagerClient().startLoading(*this, customProtocolID, request);
+    m_networkProcessProxy.processPool().customProtocolManagerClient().startLoading(*this, customProtocolID, request);
 }
 
 void LegacyCustomProtocolManagerProxy::stopLoading(uint64_t customProtocolID)
 {
-    m_processPool.customProtocolManagerClient().stopLoading(*this, customProtocolID);
+    m_networkProcessProxy.processPool().customProtocolManagerClient().stopLoading(*this, customProtocolID);
 }
 
-void LegacyCustomProtocolManagerProxy::processDidClose()
+void LegacyCustomProtocolManagerProxy::invalidate()
 {
-    m_processPool.customProtocolManagerClient().invalidate(*this);
+    m_networkProcessProxy.processPool().customProtocolManagerClient().invalidate(*this);
 }
 
 void LegacyCustomProtocolManagerProxy::wasRedirectedToRequest(uint64_t customProtocolID, const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& redirectResponse)
 {
-    m_childProcessProxy->send(Messages::LegacyCustomProtocolManager::WasRedirectedToRequest(customProtocolID, request, redirectResponse), 0);
+    m_networkProcessProxy.send(Messages::LegacyCustomProtocolManager::WasRedirectedToRequest(customProtocolID, request, redirectResponse), 0);
 }
 
 void LegacyCustomProtocolManagerProxy::didReceiveResponse(uint64_t customProtocolID, const WebCore::ResourceResponse& response, uint32_t cacheStoragePolicy)
 {
-    m_childProcessProxy->send(Messages::LegacyCustomProtocolManager::DidReceiveResponse(customProtocolID, response, cacheStoragePolicy), 0);
+    m_networkProcessProxy.send(Messages::LegacyCustomProtocolManager::DidReceiveResponse(customProtocolID, response, cacheStoragePolicy), 0);
 }
 
 void LegacyCustomProtocolManagerProxy::didLoadData(uint64_t customProtocolID, const IPC::DataReference& data)
 {
-    m_childProcessProxy->send(Messages::LegacyCustomProtocolManager::DidLoadData(customProtocolID, data), 0);
+    m_networkProcessProxy.send(Messages::LegacyCustomProtocolManager::DidLoadData(customProtocolID, data), 0);
 }
 
 void LegacyCustomProtocolManagerProxy::didFailWithError(uint64_t customProtocolID, const WebCore::ResourceError& error)
 {
-    m_childProcessProxy->send(Messages::LegacyCustomProtocolManager::DidFailWithError(customProtocolID, error), 0);
+    m_networkProcessProxy.send(Messages::LegacyCustomProtocolManager::DidFailWithError(customProtocolID, error), 0);
 }
 
 void LegacyCustomProtocolManagerProxy::didFinishLoading(uint64_t customProtocolID)
 {
-    m_childProcessProxy->send(Messages::LegacyCustomProtocolManager::DidFinishLoading(customProtocolID), 0);
+    m_networkProcessProxy.send(Messages::LegacyCustomProtocolManager::DidFinishLoading(customProtocolID), 0);
 }
 
 } // namespace WebKit
index 27d735f..b76f6ca 100644 (file)
@@ -45,18 +45,17 @@ class ResourceResponse;
 
 namespace WebKit {
 
-class ChildProcessProxy;
-class WebProcessPool;
+class NetworkProcessProxy;
 
 class LegacyCustomProtocolManagerProxy : public IPC::MessageReceiver {
 public:
-    LegacyCustomProtocolManagerProxy(ChildProcessProxy*, WebProcessPool&);
+    LegacyCustomProtocolManagerProxy(NetworkProcessProxy&);
     ~LegacyCustomProtocolManagerProxy();
 
     void startLoading(uint64_t customProtocolID, const WebCore::ResourceRequest&);
     void stopLoading(uint64_t customProtocolID);
 
-    void processDidClose();
+    void invalidate();
 
     void wasRedirectedToRequest(uint64_t customProtocolID, const WebCore::ResourceRequest&, const WebCore::ResourceResponse&);
     void didReceiveResponse(uint64_t customProtocolID, const WebCore::ResourceResponse&, uint32_t cacheStoragePolicy);
@@ -68,8 +67,7 @@ private:
     // IPC::MessageReceiver
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
 
-    ChildProcessProxy* m_childProcessProxy;
-    WebProcessPool& m_processPool;
+    NetworkProcessProxy& m_networkProcessProxy;
 
 #if PLATFORM(COCOA)
     typedef HashMap<uint64_t, RetainPtr<WKCustomProtocolLoader>> LoaderMap;
index 2cb8338..8c8f833 100644 (file)
@@ -69,7 +69,7 @@ NetworkProcessProxy::NetworkProcessProxy(WebProcessPool& processPool)
     : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
     , m_processPool(processPool)
     , m_numPendingConnectionRequests(0)
-    , m_customProtocolManagerProxy(this, processPool)
+    , m_customProtocolManagerProxy(*this)
     , m_throttler(*this, processPool.shouldTakeUIBackgroundAssertion())
 {
     connect();
@@ -234,7 +234,7 @@ void NetworkProcessProxy::didClose(IPC::Connection&)
 {
     if (m_downloadProxyMap)
         m_downloadProxyMap->processDidClose();
-    m_customProtocolManagerProxy.processDidClose();
+    m_customProtocolManagerProxy.invalidate();
 
     m_tokenForHoldingLockedFiles = nullptr;
 
index 55d58e8..e9bdc63 100644 (file)
@@ -77,6 +77,7 @@ public:
     void setIsHoldingLockedFiles(bool);
 
     ProcessThrottler& throttler() { return m_throttler; }
+    WebProcessPool& processPool() { return m_processPool; }
 
 private:
     NetworkProcessProxy(WebProcessPool&);
index 6e8801b..4fe8bdb 100644 (file)
@@ -1,3 +1,21 @@
+2017-07-19  Brady Eidson  <beidson@apple.com>
+
+        iBooks sometimes crashes when closing a book.
+        <rdar://problem/31180331> and https://bugs.webkit.org/show_bug.cgi?id=174658
+
+        Reviewed by Oliver Hunt.
+
+        * TestWebKitAPI/Tests/WebKit2ObjC/CustomProtocolsTest.mm:
+        (-[ProcessPoolDestroyedDuringLoadingProtocol startLoading]):
+        (-[ProcessPoolDestroyedDuringLoadingProtocol finishTheLoad]):
+        (-[ProcessPoolDestroyedDuringLoadingProtocol stopLoading]):
+        (TestWebKitAPI::TEST):
+
+        Add a "spin the runloop X number of times" utility:
+        * TestWebKitAPI/Utilities.h:
+        * TestWebKitAPI/cocoa/UtilitiesCocoa.mm:
+        (TestWebKitAPI::Util::spinRunLoop):
+
 2017-07-19  Jonathan Bedard  <jbedard@apple.com>
 
         lint-test-expectations should be run during style checking
index 5e58b2c..a34d592 100644 (file)
@@ -105,6 +105,40 @@ static WKProcessGroup *processGroup()
 
 @end
 
+@interface ProcessPoolDestroyedDuringLoadingProtocol : TestProtocol
+-(void)finishTheLoad;
+@end
+
+static bool isDone;
+static RetainPtr<ProcessPoolDestroyedDuringLoadingProtocol> processPoolProtocolInstance;
+
+@implementation ProcessPoolDestroyedDuringLoadingProtocol
+
+- (void)startLoading
+{
+    NSURL *requestURL = self.request.URL;
+    NSData *data = [@"PASS" dataUsingEncoding:NSASCIIStringEncoding];
+    RetainPtr<NSURLResponse> response = adoptNS([[NSURLResponse alloc] initWithURL:requestURL MIMEType:@"text/html" expectedContentLength:data.length textEncodingName:nil]);
+
+    [self.client URLProtocol:self didReceiveResponse:response.get() cacheStoragePolicy:NSURLCacheStorageNotAllowed];
+    [self.client URLProtocol:self didLoadData:data];
+    
+    processPoolProtocolInstance = self;
+    isDone = true;
+}
+
+- (void)finishTheLoad
+{
+    [self.client URLProtocolDidFinishLoading:self];
+}
+
+- (void)stopLoading
+{
+    isDone = true;
+}
+
+@end
+
 namespace TestWebKitAPI {
 
 static void runTest()
@@ -132,6 +166,38 @@ TEST(WebKit2CustomProtocolsTest, CloseDuringCustomProtocolLoad)
     [CloseWhileStartingProtocol unregister];
 }
 
+TEST(WebKit2CustomProtocolsTest, ProcessPoolDestroyedDuringLoading)
+{
+    [ProcessPoolDestroyedDuringLoadingProtocol registerWithScheme:@"custom"];
+
+    auto autoreleasePool = adoptNS([[NSAutoreleasePool alloc] init]);
+    auto browsingContextGroup = adoptNS([[WKBrowsingContextGroup alloc] initWithIdentifier:@"TestIdentifier"]);
+    auto processGroup = adoptNS([[WKProcessGroup alloc] init]);
+    auto wkView = adoptNS([[WKView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) processGroup:processGroup.get() browsingContextGroup:browsingContextGroup.get()]);
+    
+    [[wkView browsingContextController] loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"custom:///test"]]];
+
+    Util::run(&isDone);
+    isDone = false;
+
+    processGroup = nil;
+    wkView = nil;
+    browsingContextGroup = nil;
+    autoreleasePool = nil;
+
+    ASSERT(processPoolProtocolInstance);
+    [processPoolProtocolInstance finishTheLoad];
+    
+    // isDone might already be true if the protocol has already been told to stopLoading.
+    if (!isDone)
+        Util::run(&isDone);
+    
+    // To crash reliably we need to spin the runloop a few times after the custom protocol has completed.
+    Util::spinRunLoop(10);
+
+    [ProcessPoolDestroyedDuringLoadingProtocol unregister];
+}
+
 } // namespace TestWebKitAPI
 
 #endif // WK_API_ENABLED
index f77d25c..c29303c 100644 (file)
 namespace TestWebKitAPI {
 namespace Util {
 
-// Runs a platform runloop until the 'done' is true. 
+// Runs a platform runloop until the 'done' flag is true.
 void run(bool* done);
+
+// Runs a platform runloop `count` number of spins.
+void spinRunLoop(uint64_t count);
+
+// Runs a platform runloop until the amount of seconds has passed.
 void sleep(double seconds);
 
 } // namespace Util
index ba5a52f..2c8f952 100644 (file)
@@ -35,6 +35,12 @@ void run(bool* done)
         [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]];
 }
 
+void spinRunLoop(uint64_t count)
+{
+    for (uint64_t i = 0; i < count; ++i)
+        [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]];
+}
+
 void sleep(double seconds)
 {
     [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:seconds]];