WebCore:
authortomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Aug 2006 23:18:36 +0000 (23:18 +0000)
committertomernic <tomernic@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Aug 2006 23:18:36 +0000 (23:18 +0000)
        Reviewed by John Sullivan.

        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
        in Firefox

        No test cases added, since this is essentially a leak fix.

        A brief history of NPP_GetValue(), NPObjects, and reference counting.

        Earlier versions of WebKit incorrectly interpreted the NPRuntime reference counting rules.  We failed to take
        into account the fact that plug-ins are required to retain NPObjects before returning them.  This creates several
        classes of interesting plug-ins:

        1) Plug-ins tested in WebKit and other browsers.  These plug-ins may have WebKit-specific workarounds to not retain
           the returned NPObject, thus avoiding the memory leak in WebKit.

        2) Plug-ins tested only in other browsers.  These plug-ins must already retain their NPObjects, since other browsers
           implemented the NPRuntime retain/release rules correctly.  These plug-ins likely work in WebKit, but probably leak
           NPObjects since WebKit adds its own retain in addition to the plug-in's retain.

        3) Plug-ins tested only in WebKit, that fail to retain their NPObjects before returning them.
           Such plug-ins are guaranteed to crash in other browsers due to the missing expected retain.  These plug-ins
           work in older WebKits because WebKit did not expect the plug-in to retain the NPObject.  Now that our retain
           rules match other browsers, these plug-ins may crash due to the difference in retain/release behavior.  We could
           potentially detect that situation and correct it here, but I consider it a bug that the plug-in did not follow the
           documented NPRuntime reference counting rules.  Furthermore, it is extremely unlikely that someone would develop
           a Netscape plug-in and test it *only* in WebKit.  The entire purpose of creating a Netscape plugin is so that it
           works in all browsers!

        4) Plug-ins tested only in WebKit, that properly retain their NPObjects before returning them.
           These plug-ins probably work in other browsers, and leak their NPObjects in older WebKits because of WebKit's
           extra retain.  A developer of this type of plug-in is probably unaware of the NPObject leak.  A more savvy developer
           would create a plug-in that fits into category #1.

        I am changing our NPP_GetValue() behavior to match Firefox and other browsers -- the plug-in is now expected to retain the
        returned NPObject, and the browser is expected to release it when done.  This means that plug-ins in category #3 need to be
        changed so that they don't crash in Safari.  However, such plug-ins already crash in every other browser, so I do not feel that
        this needs to be handled specifically by WebKit.

        * bridge/mac/FrameMac.mm:
        Changed -pluginScriptableObject to -createPluginScriptableObject to make clearer the contract that the method must return a
        retained NPObject.  Also changed it to return an actual NPObject* instead of a void*.  There is only one caller of this method,
        and only one implementor.  Using void* here is a needless abstraction.  It's an NPObject*!  Admit it!
        (WebCore::getInstanceForView):
        Release the NPObject after creating the bindings instance.  This is the actual bug fix.

WebKit:

        Reviewed by John Sullivan.

        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
        in Firefox

        * Plugins/WebBaseNetscapePluginView.h:
        * Plugins/WebBaseNetscapePluginView.m:
        (-[WebBaseNetscapePluginView createPluginScriptableObject]):
        Renamed this method (see corresponding WebCore ChangeLog entry for an explanation).
        Style changes.

WebKitTools:

        Reviewed by John Sullivan.

        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
        in Firefox

        * DumpRenderTree/TestNetscapePlugIn.subproj/main.c:
        (NPP_GetValue):
        WebKit's NPP_GetValue() reference counting behavior has been changed to match Firefox.  NPObject return values
        are expected to be retained by the plug-in, and released by the caller.

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

WebCore/ChangeLog
WebCore/bridge/mac/FrameMac.mm
WebKit/ChangeLog
WebKit/Plugins/WebBaseNetscapePluginView.h
WebKit/Plugins/WebBaseNetscapePluginView.m
WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.c

index 7ecf9bbec7e7b07806914ce945724a04053a2ba2..6eb8f870084774e6b4a04fbf5f145a6c5c033fc1 100644 (file)
@@ -1,3 +1,52 @@
+2006-08-28  Tim Omernick  <timo@apple.com>
+
+        Reviewed by John Sullivan.
+
+        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
+        in Firefox
+
+        No test cases added, since this is essentially a leak fix.
+
+        A brief history of NPP_GetValue(), NPObjects, and reference counting.
+
+        Earlier versions of WebKit incorrectly interpreted the NPRuntime reference counting rules.  We failed to take
+        into account the fact that plug-ins are required to retain NPObjects before returning them.  This creates several
+        classes of interesting plug-ins:
+
+        1) Plug-ins tested in WebKit and other browsers.  These plug-ins may have WebKit-specific workarounds to not retain
+           the returned NPObject, thus avoiding the memory leak in WebKit.
+
+        2) Plug-ins tested only in other browsers.  These plug-ins must already retain their NPObjects, since other browsers
+           implemented the NPRuntime retain/release rules correctly.  These plug-ins likely work in WebKit, but probably leak
+           NPObjects since WebKit adds its own retain in addition to the plug-in's retain.
+
+        3) Plug-ins tested only in WebKit, that fail to retain their NPObjects before returning them.
+           Such plug-ins are guaranteed to crash in other browsers due to the missing expected retain.  These plug-ins
+           work in older WebKits because WebKit did not expect the plug-in to retain the NPObject.  Now that our retain
+           rules match other browsers, these plug-ins may crash due to the difference in retain/release behavior.  We could
+           potentially detect that situation and correct it here, but I consider it a bug that the plug-in did not follow the
+           documented NPRuntime reference counting rules.  Furthermore, it is extremely unlikely that someone would develop
+           a Netscape plug-in and test it *only* in WebKit.  The entire purpose of creating a Netscape plugin is so that it
+           works in all browsers!
+
+        4) Plug-ins tested only in WebKit, that properly retain their NPObjects before returning them.
+           These plug-ins probably work in other browsers, and leak their NPObjects in older WebKits because of WebKit's
+           extra retain.  A developer of this type of plug-in is probably unaware of the NPObject leak.  A more savvy developer
+           would create a plug-in that fits into category #1.
+        
+        I am changing our NPP_GetValue() behavior to match Firefox and other browsers -- the plug-in is now expected to retain the
+        returned NPObject, and the browser is expected to release it when done.  This means that plug-ins in category #3 need to be
+        changed so that they don't crash in Safari.  However, such plug-ins already crash in every other browser, so I do not feel that
+        this needs to be handled specifically by WebKit.
+
+        * bridge/mac/FrameMac.mm:
+        Changed -pluginScriptableObject to -createPluginScriptableObject to make clearer the contract that the method must return a
+        retained NPObject.  Also changed it to return an actual NPObject* instead of a void*.  There is only one caller of this method,
+        and only one implementor.  Using void* here is a needless abstraction.  It's an NPObject*!  Admit it!
+        (WebCore::getInstanceForView):
+        Release the NPObject after creating the bindings instance.  This is the actual bug fix.
+
 2006-08-28  Alice Liu  <alice.liu@apple.com>
 
         Reviewed by Geoff.
index b05ac3a490aed148acdea8295a8ee86c6655da74..81f83c00b84e0ca33e191b70a7a334ad984a3774 100644 (file)
@@ -81,7 +81,7 @@
 
 @interface NSObject (WebPlugIn)
 - (id)objectForWebScript;
-- (void *)pluginScriptableObject;
+- (NPObject *)createPluginScriptableObject;
 @end
 
 using namespace std;
@@ -2997,11 +2997,16 @@ static KJS::Bindings::Instance *getInstanceForView(NSView *aView)
             return KJS::Bindings::Instance::createBindingForLanguageInstance (KJS::Bindings::Instance::ObjectiveCLanguage, object, executionContext);
         }
     }
-    else if ([aView respondsToSelector:@selector(pluginScriptableObject)]){
-        void *object = [aView pluginScriptableObject];
+    else if ([aView respondsToSelector:@selector(createPluginScriptableObject)]) {
+        NPObject *object = [aView createPluginScriptableObject];
         if (object) {
-            KJS::Bindings::RootObject *executionContext = KJS::Bindings::RootObject::findRootObjectForNativeHandleFunction ()(aView);
-            return KJS::Bindings::Instance::createBindingForLanguageInstance (KJS::Bindings::Instance::CLanguage, object, executionContext);
+            KJS::Bindings::RootObject *executionContext = KJS::Bindings::RootObject::findRootObjectForNativeHandleFunction()(aView);
+            KJS::Bindings::Instance *instance = KJS::Bindings::Instance::createBindingForLanguageInstance(KJS::Bindings::Instance::CLanguage, object, executionContext);
+            
+            // -createPluginScriptableObject returns a retained NPObject.  The caller is expected to release it.
+            _NPN_ReleaseObject(object);
+            
+            return instance;
         }
     }
     return 0;
index 7c1c7569c3e159e730ee8e139f553506f9e9bff8..5a2e60f710da8a08d16587822addf9a554351100 100644 (file)
@@ -1,3 +1,17 @@
+2006-08-28  Tim Omernick  <timo@apple.com>
+
+        Reviewed by John Sullivan.
+
+        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
+        in Firefox
+
+        * Plugins/WebBaseNetscapePluginView.h:
+        * Plugins/WebBaseNetscapePluginView.m:
+        (-[WebBaseNetscapePluginView createPluginScriptableObject]):
+        Renamed this method (see corresponding WebCore ChangeLog entry for an explanation).
+        Style changes.
+
 2006-08-28  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Tim Hatcher's rubberstamp
index d054d67c689e3c99490840db7db7395b0dda1dca..41c2afe6a997665f572f86a5444e72c7d022ddf5 100644 (file)
@@ -132,7 +132,8 @@ typedef union PluginPort {
 - (void)viewDidMoveToHostWindow;
 
 // Returns the NPObject that represents the plugin interface.
-- (void *)pluginScriptableObject;
+// The return value is expected to be retained.
+- (NPObject *)createPluginScriptableObject;
 
 // -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
index fc2fae812026741ae5fe0c57f334d18372d05bb7..9417d6da1703c61db54fda13d15de32ec3c8b7c7 100644 (file)
@@ -1711,18 +1711,19 @@ static OSStatus TSMEventHandler(EventHandlerCallRef inHandlerRef, EventRef inEve
     }
 }
 
-- (void *)pluginScriptableObject
+- (NPObject *)createPluginScriptableObject
 {
-    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;
-        }
-    }
-    return (void *)0;
+    if (!NPP_GetValue)
+        return NULL;
+        
+    void *value = NULL;
+    [self willCallPlugInFunction];
+    NPError error = NPP_GetValue(instance, NPPVpluginScriptableNPObject, (void *)&value);
+    [self didCallPlugInFunction];
+    if (error != NPERR_NO_ERROR)
+        return NULL;
+    
+    return value;
 }
 
 - (void)willCallPlugInFunction
index aac6417f81d051bde20538c9651e07aa3317c56a..c3b46eb3beb002ffcb46ec1441834dac08794946 100644 (file)
@@ -1,3 +1,16 @@
+2006-08-16  Tim Omernick  <timo@apple.com>
+
+        Reviewed by John Sullivan.
+
+        Part of <rdar://problem/4481553> NetscapeMoviePlugIn example code scripting doesn't work in Firefox (4319)
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=4319>: NetscapeMoviePlugIn example code scripting doesn't work
+        in Firefox
+
+        * DumpRenderTree/TestNetscapePlugIn.subproj/main.c:
+        (NPP_GetValue):
+        WebKit's NPP_GetValue() reference counting behavior has been changed to match Firefox.  NPObject return values
+        are expected to be retained by the plug-in, and released by the caller.
+
 2006-08-28  Nikolas Zimmermann  <zimmermann@kde.org>
 
         Reviewed by Tim Hatcher.
index c217b52dc3d80005d0a88fbc5c5bc4ee5d30dd49..c96141e9cf91a0835f9dd92295f23c07ee5c70d0 100644 (file)
@@ -133,6 +133,8 @@ NPError NPP_GetValue(NPP instance, NPPVariable variable, void *value)
     if (variable == NPPVpluginScriptableNPObject) {
         void **v = (void **)value;
         PluginObject *obj = instance->pdata;
+        // Return value is expected to be retained
+        browser->retainobject((NPObject *)obj);
         *v = obj;
         return NPERR_NO_ERROR;
     }