Changeset 28006 in webkit

Timestamp:

Nov 24, 2007, 3:48:52 PM (18 years ago)

Author:

mrowe@apple.com

Message:

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

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.

Location:

trunk

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/loader/mac/NetscapePlugInStreamLoaderMac.mm (modified) (2 diffs)
• WebCore/loader/mac/WebPlugInStreamLoaderDelegate.h (modified) (1 diff)
• WebKit/mac/ChangeLog (modified) (1 diff)
• WebKit/mac/Plugins/WebBaseNetscapePluginStream.h (modified) (1 diff)
• WebKit/mac/Plugins/WebBaseNetscapePluginStream.mm (modified) (10 diffs)
• WebKit/mac/Plugins/WebBaseNetscapePluginView.mm (modified) (2 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/loader/mac/NetscapePlugInStreamLoaderMac.mm

@@ -43 +43 @@
     , m_stream(stream)
 {
+    setShouldBufferData(false);
 }
 
@@ -107 +108 @@
 
     m_documentLoader->removePlugInStreamLoader(this);
-    NSData *data = resourceData()->createNSData();
-    [m_stream.get() finishedLoadingWithData:data];
-    [data release];
+    [m_stream.get() finishedLoading];
     ResourceLoader::didFinishLoading();
 }

trunk/WebCore/loader/mac/WebPlugInStreamLoaderDelegate.h

@@ -42 +42 @@
 
 - (void)receivedData:(NSData *)data;
-- (void)finishedLoadingWithData:(NSData *)data;
+- (void)finishedLoading;
 
 @end

trunk/WebKit/mac/ChangeLog

@@ +1 @@
+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>
 

trunk/WebKit/mac/Plugins/WebBaseNetscapePluginStream.h

@@ -46 +46 @@
     int32 offset;
     NPStream stream;
-    char *path;
+    NSString *path;
+    int fileDescriptor;
     BOOL sendNotification;
     void *notifyData;

trunk/WebKit/mac/Plugins/WebBaseNetscapePluginStream.mm

@@ -43 +43 @@
 #define WEB_REASON_NONE -1
 
-static char *CarbonPathFromPOSIXPath(const char *posixPath);
+static NSString *CarbonPathFromPOSIXPath(NSString *posixPath);
 
 typedef HashMap<NPStream*, NPP> StreamMap;
@@ -118 +118 @@
     notifyData = theNotifyData;
     sendNotification = flag;
+    fileDescriptor = -1;
 
     streams().add(&stream, thePlugin);
@@ -134 +135 @@
     // The stream file should have been deleted, and the path freed, in -_destroyStream
     ASSERT(!path);
+    ASSERT(fileDescriptor == -1);
 
     [requestURL release];
@@ -158 +160 @@
     // The stream file should have been deleted, and the path freed, in -_destroyStream
     ASSERT(!path);
+    ASSERT(fileDescriptor == -1);
 
     free((void *)stream.url);
@@ -258 +261 @@
     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
@@ -365 +370 @@
     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];
 
@@ -377 +383 @@
             // (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)
@@ -453 +458 @@
     [self setPlugin:NULL];
     [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];
 }
 
@@ -553 +521 @@
 }
 
+- (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
 {
@@ -564 +584 @@
         [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;
-        }
-    }
-
-    return NULL;
+    CFURLRef url = (CFURLRef)[NSURL fileURLWithPath:posixPath];
+    if (!url)
+        return nil;
+
+    NSString *hfsPath = NSMakeCollectable(CFURLCopyFileSystemPath(url, kCFURLHFSPathStyle));
+    return [hfsPath autorelease];
 }
 

trunk/WebKit/mac/Plugins/WebBaseNetscapePluginView.mm

@@ -2010 +2010 @@
     
     if ([self isStarted])
-        [_manualStream finishedLoadingWithData:[[self dataSource] data]];    
+        [_manualStream finishedLoading];
 }
 
@@ -2082 +2082 @@
                                headers:nil];
         [stream receivedData:JSData];
-        [stream finishedLoadingWithData:JSData];
+        [stream finishedLoading];
         [stream release];
     }