[iOS] Break a reference cycle between PreviewLoader and ResourceLoader
authoraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Mar 2019 23:30:47 +0000 (23:30 +0000)
committeraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Mar 2019 23:30:47 +0000 (23:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194964
<rdar://problem/48279441>

Reviewed by Alex Christensen.

Source/WebCore:

When a document's QuickLook preview is loaded, a reference cycle is created between
WebPreviewLoader and ResourceLoader. Break the cycle by changing WebPreviewLoader to hold a
WeakPtr to its ResourceLoader ResourceLoader::releaseResources().

Fixes leaks detected by `run-webkit-tests --leaks LayoutTests/quicklook`. Also covered by
existing API tests.

* loader/ResourceLoader.h:
* loader/ios/PreviewLoader.mm:
(-[WebPreviewLoader initWithResourceLoader:resourceResponse:]):
(-[WebPreviewLoader _sendDidReceiveResponseIfNecessary]):
(-[WebPreviewLoader connection:didReceiveData:lengthReceived:]):
(-[WebPreviewLoader connectionDidFinishLoading:]):
(-[WebPreviewLoader connection:didFailWithError:]):

Source/WebKitLegacy/mac:

The WebDataSource._quickLookContent SPI accidentally relied on PreviewLoaders being leaked
to keep the temporary file referenced by WebQuickLookFileNameKey in existence. Since
PreviewLoaders are now being deleted properly, we teach WebDataSource to keep the
PreviewLoaderClient alive for its lifetime. This ensures that as long as
WebDataSource._quickLookContent can be called, the associated temp file will not be deleted
by WebKit.

* WebCoreSupport/WebFrameLoaderClient.mm:
(WebFrameLoaderClient::createPreviewLoaderClient):
* WebView/WebDataSource.mm:
(-[WebDataSource _quickLookPreviewLoaderClient]):
(-[WebDataSource _setQuickLookPreviewLoaderClient:]):
* WebView/WebDataSourceInternal.h:

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

Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceLoader.h
Source/WebCore/loader/ios/PreviewLoader.mm
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
Source/WebKitLegacy/mac/WebView/WebDataSource.mm
Source/WebKitLegacy/mac/WebView/WebDataSourceInternal.h

index fbe8963..457bea6 100644 (file)
@@ -1,3 +1,26 @@
+2019-03-25  Andy Estes  <aestes@apple.com>
+
+        [iOS] Break a reference cycle between PreviewLoader and ResourceLoader
+        https://bugs.webkit.org/show_bug.cgi?id=194964
+        <rdar://problem/48279441>
+
+        Reviewed by Alex Christensen.
+
+        When a document's QuickLook preview is loaded, a reference cycle is created between
+        WebPreviewLoader and ResourceLoader. Break the cycle by changing WebPreviewLoader to hold a
+        WeakPtr to its ResourceLoader ResourceLoader::releaseResources().
+
+        Fixes leaks detected by `run-webkit-tests --leaks LayoutTests/quicklook`. Also covered by
+        existing API tests.
+
+        * loader/ResourceLoader.h:
+        * loader/ios/PreviewLoader.mm:
+        (-[WebPreviewLoader initWithResourceLoader:resourceResponse:]):
+        (-[WebPreviewLoader _sendDidReceiveResponseIfNecessary]):
+        (-[WebPreviewLoader connection:didReceiveData:lengthReceived:]):
+        (-[WebPreviewLoader connectionDidFinishLoading:]):
+        (-[WebPreviewLoader connection:didFailWithError:]):
+
 2019-03-25  Alex Christensen  <achristensen@webkit.org>
 
         Enable IPC sending and receiving non-default-constructible types
index 4c33a50..a8ac773 100644 (file)
@@ -53,7 +53,7 @@ class FrameLoader;
 class NetworkLoadMetrics;
 class PreviewLoader;
 
-class ResourceLoader : public RefCounted<ResourceLoader>, protected ResourceHandleClient {
+class ResourceLoader : public CanMakeWeakPtr<ResourceLoader>, public RefCounted<ResourceLoader>, protected ResourceHandleClient {
 public:
     virtual ~ResourceLoader() = 0;
 
index 66c0c9e..7251d70 100644 (file)
@@ -42,7 +42,7 @@
 using namespace WebCore;
 
 @interface WebPreviewLoader : NSObject {
-    RefPtr<ResourceLoader> _resourceLoader;
+    WeakPtr<ResourceLoader> _resourceLoader;
     ResourceResponse _response;
     RefPtr<PreviewLoaderClient> _client;
     std::unique_ptr<PreviewConverter> _converter;
@@ -81,7 +81,7 @@ static PreviewLoaderClient& emptyClient()
     if (!self)
         return nil;
 
-    _resourceLoader = &resourceLoader;
+    _resourceLoader = makeWeakPtr(resourceLoader);
     _response = resourceResponse;
     _converter = std::make_unique<PreviewConverter>(self, _response);
     _bufferedDataArray = adoptNS([[NSMutableArray alloc] init]);
@@ -121,6 +121,9 @@ static PreviewLoaderClient& emptyClient()
 
 - (void)_sendDidReceiveResponseIfNecessary
 {
+    if (!_resourceLoader)
+        return;
+
     ASSERT(!_resourceLoader->reachedTerminalState());
     if (_hasSentDidReceiveResponse)
         return;
@@ -138,6 +141,9 @@ static PreviewLoaderClient& emptyClient()
     _resourceLoader->didReceiveResponse(response, [self, retainedSelf = retainPtr(self)] {
         _hasProcessedResponse = YES;
 
+        if (!_resourceLoader)
+            return;
+
         if (_resourceLoader->reachedTerminalState())
             return;
 
@@ -158,6 +164,9 @@ static PreviewLoaderClient& emptyClient()
 
 - (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data lengthReceived:(long long)lengthReceived
 {
+    if (!_resourceLoader)
+        return;
+
     ASSERT_UNUSED(connection, !connection);
     if (_resourceLoader->reachedTerminalState())
         return;
@@ -185,6 +194,9 @@ static PreviewLoaderClient& emptyClient()
 
 - (void)connectionDidFinishLoading:(NSURLConnection *)connection
 {
+    if (!_resourceLoader)
+        return;
+
     ASSERT_UNUSED(connection, !connection);
     if (_resourceLoader->reachedTerminalState())
         return;
@@ -206,6 +218,9 @@ static inline bool isQuickLookPasswordError(NSError *error)
 
 - (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error
 {
+    if (!_resourceLoader)
+        return;
+
     ASSERT_UNUSED(connection, !connection);
     if (_resourceLoader->reachedTerminalState())
         return;
index 24c554d..98b02c8 100644 (file)
@@ -1,3 +1,25 @@
+2019-03-25  Andy Estes  <aestes@apple.com>
+
+        [iOS] Break a reference cycle between PreviewLoader and ResourceLoader
+        https://bugs.webkit.org/show_bug.cgi?id=194964
+        <rdar://problem/48279441>
+
+        Reviewed by Alex Christensen.
+
+        The WebDataSource._quickLookContent SPI accidentally relied on PreviewLoaders being leaked
+        to keep the temporary file referenced by WebQuickLookFileNameKey in existence. Since
+        PreviewLoaders are now being deleted properly, we teach WebDataSource to keep the
+        PreviewLoaderClient alive for its lifetime. This ensures that as long as
+        WebDataSource._quickLookContent can be called, the associated temp file will not be deleted
+        by WebKit.
+
+        * WebCoreSupport/WebFrameLoaderClient.mm:
+        (WebFrameLoaderClient::createPreviewLoaderClient):
+        * WebView/WebDataSource.mm:
+        (-[WebDataSource _quickLookPreviewLoaderClient]):
+        (-[WebDataSource _setQuickLookPreviewLoaderClient:]):
+        * WebView/WebDataSourceInternal.h:
+
 2019-03-25  Gyuyoung Kim  <gyuyoung.kim@webkit.org>
 
         Remove NavigatorContentUtils in WebCore/Modules
index b1fb7b9..35ed759 100644 (file)
@@ -2243,8 +2243,11 @@ RefPtr<PreviewLoaderClient> WebFrameLoaderClient::createPreviewLoaderClient(cons
     if (!filePath)
         return nullptr;
 
+    auto documentWriter = adoptRef(*new QuickLookDocumentWriter(filePath));
+
     [m_webFrame provisionalDataSource]._quickLookContent = @{ WebQuickLookFileNameKey : filePath, WebQuickLookUTIKey : uti };
-    return adoptRef(*new QuickLookDocumentWriter(filePath));
+    [m_webFrame provisionalDataSource]._quickLookPreviewLoaderClient = documentWriter.ptr();
+    return documentWriter;
 }
 #endif
 
index 51802cc..529dc63 100644 (file)
@@ -53,6 +53,7 @@
 #import <WebCore/FrameLoader.h>
 #import <WebCore/LegacyWebArchive.h>
 #import <WebCore/MIMETypeRegistry.h>
+#import <WebCore/PreviewLoaderClient.h>
 #import <WebCore/ResourceRequest.h>
 #import <WebCore/SharedBuffer.h>
 #import <WebCore/WebCoreObjCExtras.h>
@@ -108,6 +109,7 @@ public:
 #endif
 #if USE(QUICK_LOOK)
     RetainPtr<NSDictionary> _quickLookContent;
+    RefPtr<WebCore::PreviewLoaderClient> _quickLookPreviewLoaderClient;
 #endif
 };
 
@@ -417,10 +419,20 @@ static inline void addTypesFromClass(NSMutableDictionary *allTypes, Class objCCl
 }
 
 #if USE(QUICK_LOOK)
+- (WebCore::PreviewLoaderClient*)_quickLookPreviewLoaderClient
+{
+    return toPrivate(_private)->_quickLookPreviewLoaderClient.get();
+}
+
 - (void)_setQuickLookContent:(NSDictionary *)quickLookContent
 {
     toPrivate(_private)->_quickLookContent = adoptNS([quickLookContent copy]);
 }
+
+- (void)_setQuickLookPreviewLoaderClient:(WebCore::PreviewLoaderClient*)quickLookPreviewLoaderClient
+{
+    toPrivate(_private)->_quickLookPreviewLoaderClient = quickLookPreviewLoaderClient;
+}
 #endif
 
 @end
index b101b13..ed0153a 100644 (file)
@@ -30,7 +30,8 @@
 #import <wtf/Forward.h>
 
 namespace WebCore {
-    class DocumentLoader;
+class DocumentLoader;
+class PreviewLoaderClient;
 }
 
 class WebDocumentLoaderMac;
@@ -59,5 +60,6 @@ class WebDocumentLoaderMac;
 - (WebCore::DocumentLoader*)_documentLoader;
 #if USE(QUICK_LOOK)
 @property (nonatomic, copy, setter=_setQuickLookContent:) NSDictionary *_quickLookContent;
+@property (nonatomic, setter=_setQuickLookPreviewLoaderClient:) WebCore::PreviewLoaderClient* _quickLookPreviewLoaderClient;
 #endif
 @end