Reviewed by Eric.
authortomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Mar 2006 04:15:07 +0000 (04:15 +0000)
committertomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Mar 2006 04:15:07 +0000 (04:15 +0000)
        <rdar://problem/3694086> -[WebBaseNetscapePluginStream finalize] is incorrect; design change needed

        * Plugins/WebBaseNetscapePluginStream.m:
        (-[WebBaseNetscapePluginStream dealloc]):
        Assert that the stream file path either never existed, or was deleted and NULL-ed out.  The stream file
        is now deleted immediately after calling NPP_StreamAsFile().
        (-[WebBaseNetscapePluginStream finalize]):
        ditto
        (-[WebBaseNetscapePluginStream _destroyStream]):
        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.

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

WebKit/ChangeLog
WebKit/Plugins/WebBaseNetscapePluginStream.m

index b700531b5389fbf4800a4aeb627338d886f1597c..4258375c858b73340996de526e1af93441374804 100644 (file)
@@ -1,3 +1,21 @@
+2006-03-27  Tim Omernick  <timo@apple.com>
+
+        Reviewed by Eric.
+
+        <rdar://problem/3694086> -[WebBaseNetscapePluginStream finalize] is incorrect; design change needed
+
+        * Plugins/WebBaseNetscapePluginStream.m:
+        (-[WebBaseNetscapePluginStream dealloc]):
+        Assert that the stream file path either never existed, or was deleted and NULL-ed out.  The stream file
+        is now deleted immediately after calling NPP_StreamAsFile().
+        (-[WebBaseNetscapePluginStream finalize]):
+        ditto
+        (-[WebBaseNetscapePluginStream _destroyStream]):
+        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.
+
 2006-03-27  Tim Omernick  <timo@apple.com>
 
         Reviewed by Eric.
 2006-03-27  Tim Omernick  <timo@apple.com>
 
         Reviewed by Eric.
index 9e7ad416ce1d0cea90f94c08be7f0c04f11a61ca..0f1dbb256dc8840aaffa1b632a15adf14571aeaa 100644 (file)
@@ -108,12 +108,8 @@ static const char *CarbonPathFromPOSIXPath(const char *posixPath);
     ASSERT(isTerminated);
     ASSERT(stream.ndata == nil);
 
     ASSERT(isTerminated);
     ASSERT(stream.ndata == nil);
 
-    // FIXME: It's generally considered bad style to do work, like deleting a file,
-    // at dealloc time. We should change things around so that this is done at a
-    // more well-defined time rather than when the last release happens.
-    if (path) {
-        unlink(path);
-    }
+    // The stream file should have been deleted, and the path freed, in -_destroyStream
+    ASSERT(!path);
 
     [requestURL release];
     [responseURL release];
 
     [requestURL release];
     [responseURL release];
@@ -132,10 +128,8 @@ static const char *CarbonPathFromPOSIXPath(const char *posixPath);
     ASSERT(isTerminated);
     ASSERT(stream.ndata == nil);
 
     ASSERT(isTerminated);
     ASSERT(stream.ndata == nil);
 
-    // FIXME: Bad for all the reasons mentioned above, but even worse for GC.
-    if (path) {
-        unlink(path);
-    }
+    // The stream file should have been deleted, and the path freed, in -_destroyStream
+    ASSERT(!path);
 
     free((void *)stream.url);
     free(path);
 
     free((void *)stream.url);
     free(path);
@@ -274,6 +268,14 @@ static const char *CarbonPathFromPOSIXPath(const char *posixPath);
             const char *carbonPath = CarbonPathFromPOSIXPath(path);
             ASSERT(carbonPath != NULL);
             NPP_StreamAsFile(instance, &stream, carbonPath);
             const char *carbonPath = CarbonPathFromPOSIXPath(path);
             ASSERT(carbonPath != NULL);
             NPP_StreamAsFile(instance, &stream, carbonPath);
+
+            // 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;
             LOG(Plugins, "NPP_StreamAsFile responseURL=%@ path=%s", responseURL, carbonPath);
         }
         
             LOG(Plugins, "NPP_StreamAsFile responseURL=%@ path=%s", responseURL, carbonPath);
         }