Reviewed by John Sullivan.
authortomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Aug 2006 23:06:00 +0000 (23:06 +0000)
committertomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Aug 2006 23:06:00 +0000 (23:06 +0000)
        <rdar://problem/4480737> Flash crashes after it replaces itself via a document.write()

        I kind of hate to do this, but this is the best way to work around buggy plug-ins like Flash that assume that
        NPP_Destroy() cannot be called while the browser is calling one of its other plug-in functions.  The classic
        situation is a plug-in that replaces itself via an NPN_Invoke() that executes a document.write().

        * Plugins/WebBaseNetscapePluginView.h:
        * Plugins/WebBaseNetscapePluginView.m:
        (-[WebBaseNetscapePluginView sendEvent:]):
        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
        (-[WebBaseNetscapePluginView setWindowIfNecessary]):
        ditto
        (-[WebBaseNetscapePluginView start]):
        It should not be possible to start a plug-in instance while we are calling into it (one of those chicken/egg
        problems).  Added a sanity-checking assertion.
        (-[WebBaseNetscapePluginView stop]):
        If we're already calling a plug-in function, do not call NPP_Destroy().  The plug-in function we are calling
        may assume that its instance->pdata, or other memory freed by NPP_Destroy(), is valid and unchanged until said
        plugin-function returns.
        (-[WebBaseNetscapePluginView pluginScriptableObject]):
        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
        (-[WebBaseNetscapePluginView willCallPlugInFunction]):
        Increment plug-in function call depth.
        (-[WebBaseNetscapePluginView didCallPlugInFunction]):
        Decrement plug-in function call depth.  Stop if we're supposed to stop.
        (-[WebBaseNetscapePluginView evaluateJavaScriptPluginRequest:]):
        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
        (-[WebBaseNetscapePluginView webFrame:didFinishLoadWithReason:]):
        ditto
        (-[WebBaseNetscapePluginView _printedPluginBitmap]):
        ditto

        * Plugins/WebBaseNetscapePluginStream.m:
        (-[WebBaseNetscapePluginStream startStreamResponseURL:expectedContentLength:lastModifiedDate:MIMEType:]):
        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
        (-[WebBaseNetscapePluginStream _destroyStream]):
        ditto
        (-[WebBaseNetscapePluginStream _deliverData]):
        ditto

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

WebKit/ChangeLog
WebKit/Plugins/WebBaseNetscapePluginStream.m
WebKit/Plugins/WebBaseNetscapePluginView.h
WebKit/Plugins/WebBaseNetscapePluginView.m

index c03ad080eeff68bef0d520977fe6ada560088e28..0591f463b0447f7a3f8ca62e2e285ad8732a484c 100644 (file)
@@ -1,3 +1,47 @@
+2006-08-01  Tim Omernick  <timo@apple.com>
+
+        Reviewed by John Sullivan.
+
+        <rdar://problem/4480737> Flash crashes after it replaces itself via a document.write()
+        
+        I kind of hate to do this, but this is the best way to work around buggy plug-ins like Flash that assume that
+        NPP_Destroy() cannot be called while the browser is calling one of its other plug-in functions.  The classic
+        situation is a plug-in that replaces itself via an NPN_Invoke() that executes a document.write().
+
+        * Plugins/WebBaseNetscapePluginView.h:
+        * Plugins/WebBaseNetscapePluginView.m:
+        (-[WebBaseNetscapePluginView sendEvent:]):
+        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
+        (-[WebBaseNetscapePluginView setWindowIfNecessary]):
+        ditto
+        (-[WebBaseNetscapePluginView start]):
+        It should not be possible to start a plug-in instance while we are calling into it (one of those chicken/egg
+        problems).  Added a sanity-checking assertion.
+        (-[WebBaseNetscapePluginView stop]):
+        If we're already calling a plug-in function, do not call NPP_Destroy().  The plug-in function we are calling
+        may assume that its instance->pdata, or other memory freed by NPP_Destroy(), is valid and unchanged until said
+        plugin-function returns.
+        (-[WebBaseNetscapePluginView pluginScriptableObject]):
+        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
+        (-[WebBaseNetscapePluginView willCallPlugInFunction]):
+        Increment plug-in function call depth.
+        (-[WebBaseNetscapePluginView didCallPlugInFunction]):
+        Decrement plug-in function call depth.  Stop if we're supposed to stop.
+        (-[WebBaseNetscapePluginView evaluateJavaScriptPluginRequest:]):
+        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
+        (-[WebBaseNetscapePluginView webFrame:didFinishLoadWithReason:]):
+        ditto
+        (-[WebBaseNetscapePluginView _printedPluginBitmap]):
+        ditto
+
+        * Plugins/WebBaseNetscapePluginStream.m:
+        (-[WebBaseNetscapePluginStream startStreamResponseURL:expectedContentLength:lastModifiedDate:MIMEType:]):
+        Call -willCallPlugInFunction and -didCallPlugInFunction around calls to the NPP_* functions.
+        (-[WebBaseNetscapePluginStream _destroyStream]):
+        ditto
+        (-[WebBaseNetscapePluginStream _deliverData]):
+        ditto
+
 2006-08-01  Maciej Stachowiak  <mjs@apple.com>
 
         - fix build after last change
index 9e3fb60aa1ded12d92a4c2a2072677288c92ca76..38680c86adadded5a32fb7cfd1fad8106963077d 100644 (file)
@@ -221,7 +221,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
 
     // FIXME: Need a way to check if stream is seekable
 
+    [pluginView willCallPlugInFunction];
     NPError npErr = NPP_NewStream(instance, (char *)[MIMEType UTF8String], &stream, NO, &transferMode);
+    [pluginView didCallPlugInFunction];
     LOG(Plugins, "NPP_NewStream URL=%@ MIME=%@ error=%d", responseURL, MIMEType, npErr);
 
     if (npErr != NPERR_NO_ERROR) {
@@ -272,7 +274,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
             ASSERT(path != NULL);
             char *carbonPath = CarbonPathFromPOSIXPath(path);
             ASSERT(carbonPath != NULL);
+            [pluginView willCallPlugInFunction];
             NPP_StreamAsFile(instance, &stream, carbonPath);
+            [pluginView 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()
@@ -286,7 +290,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
         }
         
         NPError npErr;
+        [pluginView willCallPlugInFunction];
         npErr = NPP_DestroyStream(instance, &stream, reason);
+        [pluginView didCallPlugInFunction];
         LOG(Plugins, "NPP_DestroyStream responseURL=%@ error=%d", responseURL, npErr);
         
         stream.ndata = nil;
@@ -294,7 +300,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
     
     if (sendNotification) {
         // NPP_URLNotify expects the request URL, not the response URL.
+        [pluginView willCallPlugInFunction];
         NPP_URLNotify(instance, [requestURL _web_URLCString], reason, notifyData);
+        [pluginView didCallPlugInFunction];
         LOG(Plugins, "NPP_URLNotify requestURL=%@ reason=%d", requestURL, reason);
     }
     
@@ -383,7 +391,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
     int32 totalBytesDelivered = 0;
     
     while (totalBytesDelivered < totalBytes) {
+        [pluginView willCallPlugInFunction];
         int32 deliveryBytes = NPP_WriteReady(instance, &stream);
+        [pluginView didCallPlugInFunction];
         LOG(Plugins, "NPP_WriteReady responseURL=%@ bytes=%d", responseURL, deliveryBytes);
         
         if (deliveryBytes <= 0) {
@@ -393,7 +403,9 @@ static char *CarbonPathFromPOSIXPath(const char *posixPath);
         } else {
             deliveryBytes = MIN(deliveryBytes, totalBytes - totalBytesDelivered);
             NSData *subdata = [deliveryData subdataWithRange:NSMakeRange(totalBytesDelivered, deliveryBytes)];
+            [pluginView willCallPlugInFunction];
             deliveryBytes = NPP_Write(instance, &stream, offset, [subdata length], (void *)[subdata bytes]);
+            [pluginView didCallPlugInFunction];
             if (deliveryBytes < 0) {
                 // Netscape documentation says that a negative result from NPP_Write means cancel the load.
                 [self cancelLoadAndDestroyStreamWithError:[self _pluginCancelledConnectionError]];
index a7b646c0117ef4340640050668d7b2a38a4f652e..d054d67c689e3c99490840db7db7395b0dda1dca 100644 (file)
@@ -76,6 +76,8 @@ typedef union PluginPort {
     BOOL currentEventIsUserGesture;
     BOOL isTransparent;
     BOOL isCompletelyObscured;
+    BOOL shouldStopSoon;
+    unsigned pluginFunctionCallDepth;
     
     DOMElement *element;
     
@@ -129,7 +131,20 @@ typedef union PluginPort {
 - (void)viewWillMoveToHostWindow:(NSWindow *)hostWindow;
 - (void)viewDidMoveToHostWindow;
 
-/* Returns the NPObject that represents the plugin interface. */
+// Returns the NPObject that represents the plugin interface.
 - (void *)pluginScriptableObject;
 
+// -willCallPlugInFunction must be called before calling any of the NPP_* functions for this view's NPP instance.
+// This is necessary to ensure that plug-ins are not destroyed while WebKit calls into them.  Some plug-ins (Flash
+// at least) are written with the assumption that nothing they do in their plug-in functions can cause NPP_Destroy()
+// to be called.  Unfortunately, this is not true, especially if the plug-in uses NPN_Invoke() to execute a
+// document.write(), which clears the document and destroys the plug-in.
+// See <rdar://problem/4480737>.
+- (void)willCallPlugInFunction;
+
+// -didCallPlugInFunction should be called after returning from a plug-in function.  It should be called exactly
+// once for every call to -willCallPlugInFunction.
+// See <rdar://problem/4480737>.
+- (void)didCallPlugInFunction;
+
 @end
index b47892bdd38c208e77e984b2250209601c3c3d5e..789754df266f5f653bf3ebcf096a2ec3b8a3241a 100644 (file)
@@ -610,7 +610,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     // Temporarily retain self in case the plug-in view is released while sending an event. 
     [[self retain] autorelease];
     
+    [self willCallPlugInFunction];
     BOOL acceptedEvent = NPP_HandleEvent(instance, event);
+    [self didCallPlugInFunction];
     
     currentEventIsUserGesture = NO;
     
@@ -1069,7 +1071,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
         // A CoreGraphics or OpenGL plugin's window may only be set while the plugin is being updated
         ASSERT((drawingModel != NPDrawingModelCoreGraphics && drawingModel != NPDrawingModelOpenGL) || [NSView focusView] == self);
         
+        [self willCallPlugInFunction];
         npErr = NPP_SetWindow(instance, &window);
+        [self didCallPlugInFunction];
         inSetWindow = NO;
 
 #ifndef NDEBUG
@@ -1207,6 +1211,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     // browser window.  Windowless plug-ins are rendered off-screen, then copied into the main browser window.
     window.type = NPWindowTypeWindow;
     
+    // NPN_New(), which creates the plug-in instance, should never be called while calling a plug-in function for that instance.
+    ASSERT(pluginFunctionCallDepth == 0);
+
     [[self class] setCurrentPluginView:self];
     NPError npErr = NPP_New((char *)[MIMEType cString], instance, mode, argsCount, cAttributes, cValues, NULL);
     [[self class] setCurrentPluginView:nil];
@@ -1253,6 +1260,15 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
 
 - (void)stop
 {
+    // If we're already calling a plug-in function, do not call NPP_Destroy().  The plug-in function we are calling
+    // may assume that its instance->pdata, or other memory freed by NPP_Destroy(), is valid and unchanged until said
+    // plugin-function returns.
+    // See <rdar://problem/4480737>.
+    if (pluginFunctionCallDepth > 0) {
+        shouldStopSoon = YES;
+        return;
+    }
+    
     [self removeTrackingRect];
 
     if (!isStarted) {
@@ -1694,7 +1710,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
 {
     if (NPP_GetValue) {
         void *value = 0;
+        [self willCallPlugInFunction];
         NPError npErr = NPP_GetValue (instance, NPPVpluginScriptableNPObject, (void *)&value);
+        [self didCallPlugInFunction];
         if (npErr == NPERR_NO_ERROR) {
             return value;
         }
@@ -1702,6 +1720,24 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     return (void *)0;
 }
 
+- (void)willCallPlugInFunction
+{
+    // Could try to prevent infinite recursion here, but it's probably not worth the effort.
+    pluginFunctionCallDepth++;
+}
+
+- (void)didCallPlugInFunction
+{
+    ASSERT(pluginFunctionCallDepth > 0);
+    pluginFunctionCallDepth--;
+    
+    // If -stop was called while we were calling into a plug-in function, and we're no longer
+    // inside a plug-in function, stop now.
+    if (pluginFunctionCallDepth == 0 && shouldStopSoon) {
+        shouldStopSoon = NO;
+        [self stop];
+    }
+}
 
 @end
 
@@ -1750,7 +1786,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
         // FIXME: If the result is a string, we probably want to put that string into the frame, just
         // like we do in KHTMLPartBrowserExtension::openURLRequest.
         if ([JSPluginRequest sendNotification]) {
+            [self willCallPlugInFunction];
             NPP_URLNotify(instance, [URL _web_URLCString], NPRES_DONE, [JSPluginRequest notifyData]);
+            [self didCallPlugInFunction];
         }
     } else if ([result length] > 0) {
         // Don't call NPP_NewStream and other stream methods if there is no JS result to deliver. This is what Mozilla does.
@@ -1777,7 +1815,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     ASSERT(pluginRequest != nil);
     ASSERT([pluginRequest sendNotification]);
         
+    [self willCallPlugInFunction];
     NPP_URLNotify(instance, [[[pluginRequest request] URL] _web_URLCString], reason, [pluginRequest notifyData]);
+    [self didCallPlugInFunction];
     
     [pendingFrameLoads removeObjectForKey:webFrame];
     [webFrame _setInternalLoadDelegate:nil];
@@ -2369,7 +2409,9 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     npPrint.print.embedPrint.platformPrint = printGWorld;
     
     // Tell the plugin to print into the GWorld
+    [self willCallPlugInFunction];
     NPP_Print(instance, &npPrint);
+    [self didCallPlugInFunction];
 
     // Don't need the GWorld anymore
     DisposeGWorld(printGWorld);