Fix <rdar://problem/5432686> 333MB RPRVT seems to leak @ www.43folders.com (1hr plug...
authormrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Nov 2007 23:48:52 +0000 (23:48 +0000)
committermrowe@apple.com <mrowe@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Nov 2007 23:48:52 +0000 (23:48 +0000)
http://bugs.webkit.org/show_bug.cgi?id=13705

Reviewed by Tim Hatcher.

Have NP_ASFILE and NP_ASFILEONLY streams write the data to disk as they receive it rather than
dumping the data to disk in a single go when the stream has completed loading.  On a test case
involving a 150MB Flash movie being streamed from a local web server this reduces memory consumption
on page load from around 400MB to 22MB.

The only plugin I have found that uses NP_ASFILE or NP_ASFILEONLY on the Mac is our NetscapeMoviePlugin
example code so the NP_ASFILE portion of this change has not had any testing with a real-world plugin.

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

WebCore/ChangeLog
WebCore/loader/mac/NetscapePlugInStreamLoaderMac.mm
WebCore/loader/mac/WebPlugInStreamLoaderDelegate.h
WebKit/mac/ChangeLog
WebKit/mac/Plugins/WebBaseNetscapePluginStream.h
WebKit/mac/Plugins/WebBaseNetscapePluginStream.mm
WebKit/mac/Plugins/WebBaseNetscapePluginView.mm

index 1920c4009a53e2a36aec4b9e8818ae359b0684ba..b047bd2f66670296f9f3b18499a373c068a01bc3 100644 (file)
@@ -1,3 +1,17 @@
+2007-11-24  Mark Rowe  <mrowe@apple.com>
+
+        Reviewed by Tim Hatcher.
+
+        Fix <rdar://problem/5432686> 333MB RPRVT seems to leak @ www.43folders.com (1hr plug-in stream).
+        http://bugs.webkit.org/show_bug.cgi?id=13705
+
+        Don't buffer the entire stream contents in memory in the ResourceLoader.
+
+        * loader/mac/NetscapePlugInStreamLoaderMac.mm:
+        (WebCore::NetscapePlugInStreamLoader::NetscapePlugInStreamLoader):
+        (WebCore::NetscapePlugInStreamLoader::didFinishLoading):
+        * loader/mac/WebPlugInStreamLoaderDelegate.h:
+
 2007-11-23  Adam Roben  <aroben@apple.com>
 
         Get rid of WebCoreSystemInterface on Windows
index 143c4204a860bd41aa289e29e5904bce1ea34dde..9aac2d4c4d744fef8de9ecbfc73a390bdce16219 100644 (file)
@@ -42,6 +42,7 @@ NetscapePlugInStreamLoader::NetscapePlugInStreamLoader(Frame* frame, id <WebPlug
     : ResourceLoader(frame, true, true)
     , m_stream(stream)
 {
+    setShouldBufferData(false);
 }
 
 NetscapePlugInStreamLoader::~NetscapePlugInStreamLoader()
@@ -106,9 +107,7 @@ void NetscapePlugInStreamLoader::didFinishLoading()
     RefPtr<NetscapePlugInStreamLoader> protect(this);
 
     m_documentLoader->removePlugInStreamLoader(this);
-    NSData *data = resourceData()->createNSData();
-    [m_stream.get() finishedLoadingWithData:data];
-    [data release];
+    [m_stream.get() finishedLoading];
     ResourceLoader::didFinishLoading();
 }
 
index f7fa9db43c019817b3e9dbd9935e07b8ab55ebe2..b4d38b52bbd811a014063667add8e38522c20d79 100644 (file)
@@ -41,6 +41,6 @@
 - (void)cancelLoadAndDestroyStreamWithError:(NSError *)error;
 
 - (void)receivedData:(NSData *)data;
-- (void)finishedLoadingWithData:(NSData *)data;
+- (void)finishedLoading;
 
 @end
index 5ca9b6f999acaad9448eff33ae966f6fb7a47ac7..c7775adcc22e24ffee74c677108cf68bed03bc8f 100644 (file)
@@ -1,3 +1,37 @@
+2007-11-24  Mark Rowe  <mrowe@apple.com>
+
+        Reviewed by Tim Hatcher.
+
+        Fix <rdar://problem/5432686> 333MB RPRVT seems to leak @ www.43folders.com (1hr plug-in stream).
+        http://bugs.webkit.org/show_bug.cgi?id=13705
+
+        Have NP_ASFILE and NP_ASFILEONLY streams write the data to disk as they receive it rather than
+        dumping the data to disk in a single go when the stream has completed loading.  On a test case
+        involving a 150MB Flash movie being streamed from a local web server this reduces memory consumption
+        on page load from around 400MB to 22MB.
+
+        The only plugin I have found that uses NP_ASFILE or NP_ASFILEONLY on the Mac is our NetscapeMoviePlugin
+        example code so the NP_ASFILE portion of this change has not had any testing with a real-world plugin.
+
+        * Plugins/WebBaseNetscapePluginStream.h:
+        * Plugins/WebBaseNetscapePluginStream.mm:
+        (-[WebBaseNetscapePluginStream initWithRequestURL:plugin:notifyData:sendNotification:]):
+        (-[WebBaseNetscapePluginStream dealloc]):
+        (-[WebBaseNetscapePluginStream finalize]):
+        (-[WebBaseNetscapePluginStream startStreamResponseURL:expectedContentLength:lastModifiedDate:MIMEType:headers:]):
+        (-[WebBaseNetscapePluginStream _destroyStream]): Update to work with paths as NSStrings.
+        (-[WebBaseNetscapePluginStream _deliverDataToFile:]): Open the file if it is not already open, and write any data
+        to disk.
+        (-[WebBaseNetscapePluginStream finishedLoading]): If the stream is NP_ASFILE or NP_ASFILEONLY we need to ensure
+        that the file exists before _destroyStream passes it to the plugin.  Simulating the arrival of an empty data block
+        ensure that the file will be created if it has not already.
+        (-[WebBaseNetscapePluginStream receivedData:]):
+        (CarbonPathFromPOSIXPath):
+        * Plugins/WebBaseNetscapePluginView.mm:
+        (-[WebBaseNetscapePluginView pluginViewFinishedLoading:]): Data is dealt with incrementally so there's no need to pass
+        it to finishedLoading.
+        (-[WebBaseNetscapePluginView evaluateJavaScriptPluginRequest:]): Ditto.
+
 2007-11-23  Oliver Hunt  <oliver@apple.com>
 
         Reviewed by Mark Rowe.
index 7cfaeb7e10fa18927a0121a51a85583e8fc64499..023604748ba9efbde768fbbd2adc33d32a87bf30 100644 (file)
@@ -45,7 +45,8 @@
     uint16 transferMode;
     int32 offset;
     NPStream stream;
-    char *path;
+    NSString *path;
+    int fileDescriptor;
     BOOL sendNotification;
     void *notifyData;
     char *headers;
index 21b11f5898588c1aff1e5253d1c0b94b6434f080..f07b0cce801c670e76666ead480d51657ece4034 100644 (file)
@@ -42,7 +42,7 @@
 
 #define WEB_REASON_NONE -1
 
-static char *CarbonPathFromPOSIXPath(const char *posixPath);
+static NSString *CarbonPathFromPOSIXPath(NSString *posixPath);
 
 typedef HashMap<NPStream*, NPP> StreamMap;
 static StreamMap& streams()
@@ -117,6 +117,7 @@ static StreamMap& streams()
     [self setPlugin:thePlugin];
     notifyData = theNotifyData;
     sendNotification = flag;
+    fileDescriptor = -1;
 
     streams().add(&stream, thePlugin);
     
@@ -133,6 +134,7 @@ static StreamMap& streams()
 
     // The stream file should have been deleted, and the path freed, in -_destroyStream
     ASSERT(!path);
+    ASSERT(fileDescriptor == -1);
 
     [requestURL release];
     [responseURL release];
@@ -157,6 +159,7 @@ static StreamMap& streams()
 
     // The stream file should have been deleted, and the path freed, in -_destroyStream
     ASSERT(!path);
+    ASSERT(fileDescriptor == -1);
 
     free((void *)stream.url);
     free(path);
@@ -257,6 +260,8 @@ static StreamMap& streams()
     transferMode = NP_NORMAL;
     offset = 0;
     reason = WEB_REASON_NONE;
+    // FIXME: If WebNetscapePluginStream called our initializer we wouldn't have to do this here.
+    fileDescriptor = -1;
 
     // FIXME: Need a way to check if stream is seekable
 
@@ -364,23 +369,23 @@ static StreamMap& streams()
 
     if (stream.ndata != nil) {
         if (reason == NPRES_DONE && (transferMode == NP_ASFILE || transferMode == NP_ASFILEONLY)) {
+            ASSERT(fileDescriptor == -1);
             ASSERT(path != NULL);
-            char *carbonPath = CarbonPathFromPOSIXPath(path);
+            NSString *carbonPath = CarbonPathFromPOSIXPath(path);
             ASSERT(carbonPath != NULL);
             WebBaseNetscapePluginView *pv = pluginView;
             [pv willCallPlugInFunction];
-            NPP_StreamAsFile(plugin, &stream, carbonPath);
+            NPP_StreamAsFile(plugin, &stream, [carbonPath fileSystemRepresentation]);
             [pv didCallPlugInFunction];
 
             // Delete the file after calling NPP_StreamAsFile(), instead of in -dealloc/-finalize.  It should be OK
             // to delete the file here -- NPP_StreamAsFile() is always called immediately before NPP_DestroyStream()
             // (the stream destruction function), so there can be no expectation that a plugin will read the stream
             // file asynchronously after NPP_StreamAsFile() is called.
-            unlink(path);
-            free(path);
-            path = NULL;
+            unlink([path fileSystemRepresentation]);
+            [path release];
+            path = nil;
             LOG(Plugins, "NPP_StreamAsFile responseURL=%@ path=%s", responseURL, carbonPath);
-            free(carbonPath);
 
             if (isTerminated)
                 goto exit;
@@ -454,43 +459,6 @@ exit:
     [self release];
 }
 
-- (void)finishedLoadingWithData:(NSData *)data
-{
-    if (!stream.ndata)
-        return;
-    
-    if ((transferMode == NP_ASFILE || transferMode == NP_ASFILEONLY) && !path) {
-        path = strdup("/tmp/WebKitPlugInStreamXXXXXX");
-        int fd = mkstemp(path);
-        if (fd == -1) {
-            // This should almost never happen.
-            LOG_ERROR("can't make temporary file, almost certainly a problem with /tmp");
-            // This is not a network error, but the only error codes are "network error" and "user break".
-            [self _destroyStreamWithReason:NPRES_NETWORK_ERR];
-            free(path);
-            path = NULL;
-            return;
-        }
-        int dataLength = [data length];
-        if (dataLength > 0) {
-            int byteCount = write(fd, [data bytes], dataLength);
-            if (byteCount != dataLength) {
-                // This happens only rarely, when we are out of disk space or have a disk I/O error.
-                LOG_ERROR("error writing to temporary file, errno %d", errno);
-                close(fd);
-                // This is not a network error, but the only error codes are "network error" and "user break".
-                [self _destroyStreamWithReason:NPRES_NETWORK_ERR];
-                free(path);
-                path = NULL;
-                return;
-            }
-        }
-        close(fd);
-    }
-
-    [self _destroyStreamWithReason:NPRES_DONE];
-}
-
 - (void)_deliverData
 {
     if (!stream.ndata || [deliveryData length] == 0)
@@ -552,6 +520,58 @@ exit:
     [self release];
 }
 
+- (void)_deliverDataToFile:(NSData *)data
+{
+    if (fileDescriptor == -1 && !path) {
+        NSString *temporaryFileMask = [NSTemporaryDirectory() stringByAppendingPathComponent:@"WebKitPlugInStreamXXXXXX"];
+        char *temporaryFileName = strdup([temporaryFileMask fileSystemRepresentation]);
+        fileDescriptor = mkstemp(temporaryFileName);
+        if (fileDescriptor == -1) {
+            LOG_ERROR("Can't create a temporary file.");
+            // This is not a network error, but the only error codes are "network error" and "user break".
+            [self _destroyStreamWithReason:NPRES_NETWORK_ERR];
+            free(temporaryFileName);
+            return;
+        }
+
+        path = [[NSString stringWithUTF8String:temporaryFileName] retain];
+        free(temporaryFileName);
+    }
+
+    int dataLength = [data length];
+    if (!dataLength)
+        return;
+
+    int byteCount = write(fileDescriptor, [data bytes], dataLength);
+    if (byteCount != dataLength) {
+        // This happens only rarely, when we are out of disk space or have a disk I/O error.
+        LOG_ERROR("error writing to temporary file, errno %d", errno);
+        close(fileDescriptor);
+        fileDescriptor = -1;
+
+        // This is not a network error, but the only error codes are "network error" and "user break".
+        [self _destroyStreamWithReason:NPRES_NETWORK_ERR];
+        [path release];
+        path = nil;
+    }
+}
+
+- (void)finishedLoading
+{
+    if (!stream.ndata)
+        return;
+
+    if (transferMode == NP_ASFILE || transferMode == NP_ASFILEONLY) {
+        // Fake the delivery of an empty data to ensure that the file has been created
+        [self _deliverDataToFile:[NSData data]];
+        if (fileDescriptor != -1)
+            close(fileDescriptor);
+        fileDescriptor = -1;
+    }
+
+    [self _destroyStreamWithReason:NPRES_DONE];
+}
+
 - (void)receivedData:(NSData *)data
 {
     ASSERT([data length] > 0);
@@ -563,29 +583,24 @@ exit:
         [deliveryData appendData:data];
         [self _deliverData];
     }
+    if (transferMode == NP_ASFILE || transferMode == NP_ASFILEONLY)
+        [self _deliverDataToFile:data];
+
 }
 
 @end
 
-static char *CarbonPathFromPOSIXPath(const char *posixPath)
+static NSString *CarbonPathFromPOSIXPath(NSString *posixPath)
 {
     // Doesn't add a trailing colon for directories; this is a problem for paths to a volume,
     // so this function would need to be revised if we ever wanted to call it with that.
 
-    CFURLRef url = CFURLCreateFromFileSystemRepresentation(NULL, (const UInt8 *)posixPath, strlen(posixPath), false);
-    if (url) {
-        CFStringRef hfsPath = CFURLCopyFileSystemPath(url, kCFURLHFSPathStyle);
-        CFRelease(url);
-        if (hfsPath) {
-            CFIndex bufSize = CFStringGetMaximumSizeOfFileSystemRepresentation(hfsPath);
-            char* filename = static_cast<char*>(malloc(bufSize));
-            CFStringGetFileSystemRepresentation(hfsPath, filename, bufSize);
-            CFRelease(hfsPath);
-            return filename;
-        }
-    }
+    CFURLRef url = (CFURLRef)[NSURL fileURLWithPath:posixPath];
+    if (!url)
+        return nil;
 
-    return NULL;
+    NSString *hfsPath = NSMakeCollectable(CFURLCopyFileSystemPath(url, kCFURLHFSPathStyle));
+    return [hfsPath autorelease];
 }
 
 #endif
index c61133250d7e4c1a0d1f2f5a8e2a7c85fe3889a4..cc8037b0accc251c3f35a98df5e9601a693ade3c 100644 (file)
@@ -2009,7 +2009,7 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     ASSERT(_manualStream);
     
     if ([self isStarted])
-        [_manualStream finishedLoadingWithData:[[self dataSource] data]];    
+        [_manualStream finishedLoading];
 }
 
 @end
@@ -2081,7 +2081,7 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
                               MIMEType:@"text/plain"
                                headers:nil];
         [stream receivedData:JSData];
-        [stream finishedLoadingWithData:JSData];
+        [stream finishedLoading];
         [stream release];
     }
 }