NPObjectWrapper may not address all window script object lifetime issues
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Jun 2012 02:02:09 +0000 (02:02 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Jun 2012 02:02:09 +0000 (02:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=85679

Source/WebCore:

The ScriptController implementations force-deallocate the window script object to ensure that DOM objects are not leaked if an NPAPI plugin fails to release a reference to it before being destroyed. The NPObjectWrapper was added to ensure that NPAPI scripting could not touch the real window script object after it had been deallocated, by providing the plugin with a small wrapper which will leak if the plugin fails to dereference it.

This patch removes NPObjectWrapper and instead drops the window script NPObject's reference to the underlying V8Object in ScriptController::clearScriptObjects(). If a plugin fails to dereference the object then the NPV8Object wrapper will be leaked but the DOM objects it references will not.

Patch by James Weatherall <wez@chromium.org> on 2012-06-29
Reviewed by Nate Chapin.

Test: plugins/npruntime/leak-window-scriptable-object.html

* WebCore.gypi:
* bindings/v8/NPObjectWrapper.cpp: Removed.
* bindings/v8/NPObjectWrapper.h: Removed.
* bindings/v8/NPV8Object.cpp:
(WebCore::disposeUnderlyingV8Object):
(WebCore):
(WebCore::freeV8NPObject):
(_NPN_Invoke):
(_NPN_InvokeDefault):
(_NPN_EvaluateHelper):
(_NPN_GetProperty):
(_NPN_SetProperty):
(_NPN_RemoveProperty):
(_NPN_HasProperty):
(_NPN_HasMethod):
(_NPN_Enumerate):
(_NPN_Construct):
* bindings/v8/NPV8Object.h:
(WebCore):
* bindings/v8/ScriptController.cpp:
(WebCore::ScriptController::ScriptController):
(WebCore::ScriptController::clearScriptObjects):
(WebCore::ScriptController::windowScriptNPObject):
* bindings/v8/ScriptController.h:
(ScriptController):

Tools:

TestNetscapePlugin now has a leak-window-scriptable-object test which takes a reference to the window script object, and a second reference to it via the "self" property, and does not release those references. This is used to simulate a leaky plugin in layout tests of the NPAPI scripting interface glue code.

Patch by James Weatherall <wez@chromium.org> on 2012-06-29
Reviewed by Nate Chapin.

* DumpRenderTree/DumpRenderTree.gypi:
* DumpRenderTree/TestNetscapePlugIn/PluginTest.cpp:
(PluginTest::NPN_GetProperty):
* DumpRenderTree/TestNetscapePlugIn/PluginTest.h:
(PluginTest):
* DumpRenderTree/TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp: Added.
(LeakWindowScriptableObject):
(LeakWindowScriptableObject::LeakWindowScriptableObject):
(LeakWindowScriptableObject::NPP_New):

LayoutTests:

Test that NPAPI plugins that leak references to the window script object don't cause the containing document to be leaked.

Patch by James Weatherall <wez@chromium.org> on 2012-06-29
Reviewed by Nate Chapin.

* plugins/npruntime/leak-window-scriptable-object-expected.txt: Added.
* plugins/npruntime/leak-window-scriptable-object.html: Added.

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/plugins/npruntime/leak-window-scriptable-object-expected.txt [new file with mode: 0644]
LayoutTests/plugins/npruntime/leak-window-scriptable-object.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/WebCore.gypi
Source/WebCore/bindings/v8/NPObjectWrapper.cpp [deleted file]
Source/WebCore/bindings/v8/NPObjectWrapper.h [deleted file]
Source/WebCore/bindings/v8/NPV8Object.cpp
Source/WebCore/bindings/v8/NPV8Object.h
Source/WebCore/bindings/v8/ScriptController.cpp
Source/WebCore/bindings/v8/ScriptController.h
Source/WebKit/chromium/src/WebBindings.cpp
Tools/ChangeLog
Tools/DumpRenderTree/DumpRenderTree.gypi
Tools/DumpRenderTree/TestNetscapePlugIn/PluginTest.cpp
Tools/DumpRenderTree/TestNetscapePlugIn/PluginTest.h
Tools/DumpRenderTree/TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp [new file with mode: 0644]

index 3062c0b..58f781d 100644 (file)
@@ -1,3 +1,15 @@
+2012-06-29  James Weatherall  <wez@chromium.org>
+
+        NPObjectWrapper may not address all window script object lifetime issues
+        https://bugs.webkit.org/show_bug.cgi?id=85679
+
+        Test that NPAPI plugins that leak references to the window script object don't cause the containing document to be leaked.
+
+        Reviewed by Nate Chapin.
+
+        * plugins/npruntime/leak-window-scriptable-object-expected.txt: Added.
+        * plugins/npruntime/leak-window-scriptable-object.html: Added.
+
 2012-06-29  Emil A Eklund  <eae@chromium.org>
 
         Unreviewed chromium windows rebaselines for r121599.
diff --git a/LayoutTests/plugins/npruntime/leak-window-scriptable-object-expected.txt b/LayoutTests/plugins/npruntime/leak-window-scriptable-object-expected.txt
new file mode 100644 (file)
index 0000000..1112248
--- /dev/null
@@ -0,0 +1,11 @@
+Test that a plugin that fails to release the window script object doesn't result in the window's document being leaked
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.internals.numberOfLiveDocuments() > initialLiveDocumentCount is true
+PASS window.internals.numberOfLiveDocuments() == initialLiveDocumentCount is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/plugins/npruntime/leak-window-scriptable-object.html b/LayoutTests/plugins/npruntime/leak-window-scriptable-object.html
new file mode 100644 (file)
index 0000000..5d5de50
--- /dev/null
@@ -0,0 +1,31 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+description("Test that a plugin that fails to release the window script object doesn't result in the window's document being leaked");
+
+if (window.internals !== undefined) {
+    // Test that the live document count returns to the initial value, rather than to a specific value, so this test is robust to leaks in preceding tests.
+    gc();
+    var initialLiveDocumentCount = window.internals.numberOfLiveDocuments();
+
+    var iframe = document.createElement('iframe');
+    document.body.appendChild(iframe);
+    iframe.contentWindow.document.documentElement.innerHTML = '<embed id="plugin" type="application/x-webkit-test-netscape" test="leak-window-scriptable-object"></embed>';
+
+    shouldBeTrue("window.internals.numberOfLiveDocuments() > initialLiveDocumentCount");
+
+    document.body.removeChild(iframe);
+
+    gc();
+    shouldBeTrue("window.internals.numberOfLiveDocuments() == initialLiveDocumentCount");
+}
+
+var successfullyParsed = true;
+</script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
index 17467f5..a20cff6 100644 (file)
@@ -1,3 +1,42 @@
+2012-06-29  James Weatherall  <wez@chromium.org>
+
+        NPObjectWrapper may not address all window script object lifetime issues
+        https://bugs.webkit.org/show_bug.cgi?id=85679
+
+        The ScriptController implementations force-deallocate the window script object to ensure that DOM objects are not leaked if an NPAPI plugin fails to release a reference to it before being destroyed. The NPObjectWrapper was added to ensure that NPAPI scripting could not touch the real window script object after it had been deallocated, by providing the plugin with a small wrapper which will leak if the plugin fails to dereference it.
+
+        This patch removes NPObjectWrapper and instead drops the window script NPObject's reference to the underlying V8Object in ScriptController::clearScriptObjects(). If a plugin fails to dereference the object then the NPV8Object wrapper will be leaked but the DOM objects it references will not.
+
+        Reviewed by Nate Chapin.
+
+        Test: plugins/npruntime/leak-window-scriptable-object.html
+
+        * WebCore.gypi:
+        * bindings/v8/NPObjectWrapper.cpp: Removed.
+        * bindings/v8/NPObjectWrapper.h: Removed.
+        * bindings/v8/NPV8Object.cpp:
+        (WebCore::disposeUnderlyingV8Object):
+        (WebCore):
+        (WebCore::freeV8NPObject):
+        (_NPN_Invoke):
+        (_NPN_InvokeDefault):
+        (_NPN_EvaluateHelper):
+        (_NPN_GetProperty):
+        (_NPN_SetProperty):
+        (_NPN_RemoveProperty):
+        (_NPN_HasProperty):
+        (_NPN_HasMethod):
+        (_NPN_Enumerate):
+        (_NPN_Construct):
+        * bindings/v8/NPV8Object.h:
+        (WebCore):
+        * bindings/v8/ScriptController.cpp:
+        (WebCore::ScriptController::ScriptController):
+        (WebCore::ScriptController::clearScriptObjects):
+        (WebCore::ScriptController::windowScriptNPObject):
+        * bindings/v8/ScriptController.h:
+        (ScriptController):
+
 2012-06-29  Adam Barth  <abarth@webkit.org>
 
         Update complex fonts on Android to use fonts from a newer SDK
index d9d2d3a..fe4a095 100644 (file)
             'bindings/v8/IsolatedWorld.h',
             'bindings/v8/JavaScriptCallFrame.cpp',
             'bindings/v8/JavaScriptCallFrame.h',
-            'bindings/v8/NPObjectWrapper.cpp',
-            'bindings/v8/NPObjectWrapper.h',
             'bindings/v8/NPV8Object.cpp',
             'bindings/v8/NPV8Object.h',
             'bindings/v8/Dictionary.cpp',
diff --git a/Source/WebCore/bindings/v8/NPObjectWrapper.cpp b/Source/WebCore/bindings/v8/NPObjectWrapper.cpp
deleted file mode 100644 (file)
index 7c73b2f..0000000
+++ /dev/null
@@ -1,181 +0,0 @@
-/*
- * Copyright (C) 2011 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include "config.h"
-#include "NPObjectWrapper.h"
-
-namespace WebCore {
-
-struct NPProxyObject {
-    NPObject object;
-    NPObjectWrapper* wrapper;
-};
-
-NPClass NPObjectWrapper::m_npClassWrapper = {
-    NP_CLASS_STRUCT_VERSION,
-    NPObjectWrapper::NPAllocate,
-    NPObjectWrapper::NPDeallocate,
-    NPObjectWrapper::NPPInvalidate,
-    NPObjectWrapper::NPHasMethod,
-    NPObjectWrapper::NPInvoke,
-    NPObjectWrapper::NPInvokeDefault,
-    NPObjectWrapper::NPHasProperty,
-    NPObjectWrapper::NPGetProperty,
-    NPObjectWrapper::NPSetProperty,
-    NPObjectWrapper::NPRemoveProperty,
-    NPObjectWrapper::NPNEnumerate,
-    NPObjectWrapper::NPNConstruct
-};
-
-NPObjectWrapper::NPObjectWrapper(NPObject* obj)
-    : m_wrappedNPObject(obj)
-{
-}
-
-NPObject* NPObjectWrapper::create(NPObject* object)
-{
-    ASSERT(object);
-    NPProxyObject* proxyObject = reinterpret_cast<NPProxyObject*>(_NPN_CreateObject(0, &m_npClassWrapper));
-    proxyObject->wrapper = new NPObjectWrapper(object);
-    return reinterpret_cast<NPObject*>(proxyObject);
-}
-
-void NPObjectWrapper::clear()
-{
-    m_wrappedNPObject = 0;   
-}
-
-NPObjectWrapper* NPObjectWrapper::getWrapper(NPObject* obj)
-{
-    if (&m_npClassWrapper == obj->_class) {
-        NPProxyObject* proxyObject = reinterpret_cast<NPProxyObject*>(obj);
-        return proxyObject->wrapper;
-    }
-    return 0;
-}
-
-NPObject* NPObjectWrapper::getUnderlyingNPObject(NPObject* obj)
-{
-    NPObjectWrapper* wrapper = getWrapper(obj);
-    return wrapper ? wrapper->m_wrappedNPObject : 0;
-}
-
-NPObject* NPObjectWrapper::getObjectForCall(NPObject* obj)
-{
-    NPObject* actualObject = getUnderlyingNPObject(obj);
-    return actualObject ? actualObject : 0;
-}
-
-NPObject* NPObjectWrapper::NPAllocate(NPP, NPClass*)
-{
-    return reinterpret_cast<NPObject*>(new NPProxyObject);
-}
-
-void NPObjectWrapper::NPDeallocate(NPObject* obj)
-{
-    NPProxyObject* proxyObject = reinterpret_cast<NPProxyObject*>(obj);
-    delete proxyObject->wrapper;
-    delete proxyObject;
-}
-
-void NPObjectWrapper::NPPInvalidate(NPObject* obj)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    if (actualObject && actualObject->_class->invalidate)
-        actualObject->_class->invalidate(actualObject);
-}
-
-bool NPObjectWrapper::NPHasMethod(NPObject* obj, NPIdentifier name)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_HasMethod(0, actualObject, name) : false;
-}
-
-bool NPObjectWrapper::NPInvoke(NPObject* obj, NPIdentifier name, const NPVariant* args, uint32_t argCount, NPVariant* result)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_Invoke(0, actualObject, name, args, argCount, result) : false;
-}
-
-bool NPObjectWrapper::NPInvokeDefault(NPObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_InvokeDefault(0, actualObject, args, argCount, result) : false;
-}
-bool NPObjectWrapper::NPHasProperty(NPObject* obj, NPIdentifier name)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_HasProperty(0, actualObject, name) : false;
-}
-
-bool NPObjectWrapper::NPGetProperty(NPObject* obj, NPIdentifier name, NPVariant* result)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_GetProperty(0, actualObject, name, result) : false;
-}
-
-bool NPObjectWrapper::NPSetProperty(NPObject* obj, NPIdentifier name, const NPVariant* value)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_SetProperty(0, actualObject, name, value) : false;
-}
-
-bool NPObjectWrapper::NPRemoveProperty(NPObject* obj, NPIdentifier name) {
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_RemoveProperty(0, actualObject, name) : false;
-}
-
-bool NPObjectWrapper::NPNEnumerate(NPObject* obj, NPIdentifier** value, uint32_t* count)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_Enumerate(0, actualObject, value, count) : false;
-}
-
-bool NPObjectWrapper::NPNConstruct(NPObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    return actualObject ? _NPN_Construct(0, actualObject, args, argCount, result) : false;
-}
-
-bool NPObjectWrapper::NPInvokePrivate(NPP npp, NPObject* obj, bool isDefault, NPIdentifier name, const NPVariant* args, uint32_t argCount, NPVariant* result)
-{
-    NPObject* actualObject = getObjectForCall(obj);
-    if (!actualObject)
-        return false;
-
-    if (isDefault) {
-        return _NPN_InvokeDefault(0, actualObject, args, argCount, result);
-    } else {
-        return _NPN_Invoke(0, actualObject, name, args, argCount, result);
-    }
-}
-
-} // namespace WebCore
diff --git a/Source/WebCore/bindings/v8/NPObjectWrapper.h b/Source/WebCore/bindings/v8/NPObjectWrapper.h
deleted file mode 100644 (file)
index 6fdf90b..0000000
+++ /dev/null
@@ -1,88 +0,0 @@
-/*
- * Copyright (C) 2011 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#ifndef NPObjectWrapper_h
-#define NPObjectWrapper_h
-
-#include "npruntime_impl.h"
-
-namespace WebCore {
-
-// This class wraps a NPObject and provides functionality for the wrapped
-// object to be cleared out when this object is destroyed. This is to ensure
-// that callers trying to access the underlying object don't crash while
-// invoking methods on the NPObject.
-class NPObjectWrapper {
-public:
-    // Creates an instance of the NPObjectWrapper class and wraps the object
-    // passed in.
-    static NPObject* create(NPObject* object);
-
-    // This method should be called to invalidate the underlying NPObject pointer.
-    void clear();
-
-    // Returns a pointer to NPObjectWrapper if the object passed in was wrapped by us.
-    static NPObjectWrapper* getWrapper(NPObject* obj);
-
-    // Returns a pointer to the underlying raw NPObject pointer or 0 if the object
-    // passed in was not wrapped.
-    static NPObject* getUnderlyingNPObject(NPObject* obj);
-
-    // NPObject functions implemented by the wrapper.
-    static NPObject* NPAllocate(NPP, NPClass*);
-    static void NPDeallocate(NPObject* obj);
-    static void NPPInvalidate(NPObject *obj);
-    static bool NPHasMethod(NPObject* obj, NPIdentifier name);
-    static bool NPInvoke(NPObject* obj, NPIdentifier name, const NPVariant* args, uint32_t argCount, NPVariant* result);
-    static bool NPInvokeDefault(NPObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result);
-    static bool NPHasProperty(NPObject* obj, NPIdentifier name);
-    static bool NPGetProperty(NPObject* obj, NPIdentifier name, NPVariant* result);
-    static bool NPSetProperty(NPObject* obj, NPIdentifier name, const NPVariant *value);
-    static bool NPRemoveProperty(NPObject* obj, NPIdentifier name);
-    static bool NPNEnumerate(NPObject* obj, NPIdentifier **value, uint32_t* count);
-    static bool NPNConstruct(NPObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result);
-    static bool NPInvokePrivate(NPP npp, NPObject* obj,bool isDefault, NPIdentifier name, const NPVariant* args, uint32_t argCount, NPVariant* result);
-
-private:
-    NPObjectWrapper(NPObject* obj);
-
-    // Returns the underlying NPObject if the object passed in was wrapped. Otherwise
-    // just returns the object passed in.
-    static NPObject* getObjectForCall(NPObject* obj);
-
-    static NPClass m_npClassWrapper;
-    // Weak NPObject poointer.
-    NPObject* m_wrappedNPObject;
-};
-
-} // namespace WebCore
-
-#endif // NPObjectWrapper_h
-
index a3790da..d4d6cfc 100644 (file)
@@ -31,7 +31,6 @@
 #include "PlatformSupport.h"
 #include "DOMWindow.h"
 #include "Frame.h"
-#include "NPObjectWrapper.h"
 #include <wtf/OwnArrayPtr.h>
 #include "PlatformString.h"
 #include "ScriptSourceCode.h"
@@ -76,29 +75,7 @@ static NPObject* allocV8NPObject(NPP, NPClass*)
 static void freeV8NPObject(NPObject* npObject)
 {
     V8NPObject* v8NpObject = reinterpret_cast<V8NPObject*>(npObject);
-    if (int v8ObjectHash = v8NpObject->v8Object->GetIdentityHash()) {
-        V8NPObjectMap::iterator iter = staticV8NPObjectMap()->find(v8ObjectHash);
-        if (iter != staticV8NPObjectMap()->end()) {
-            V8NPObjectVector& objects = iter->second;
-            for (size_t index = 0; index < objects.size(); ++index) {
-                if (objects.at(index) == v8NpObject) {
-                    objects.remove(index);
-                    break;
-                }
-            }
-            if (objects.isEmpty())
-                staticV8NPObjectMap()->remove(v8ObjectHash);
-        } else
-            ASSERT_NOT_REACHED();
-    } else {
-        ASSERT(!v8::Context::InContext());
-        staticV8NPObjectMap()->clear();
-    }
-
-#ifndef NDEBUG
-    V8GCController::unregisterGlobalHandle(v8NpObject, v8NpObject->v8Object);
-#endif
-    v8NpObject->v8Object.Dispose();
+    disposeUnderlyingV8Object(npObject);
     free(v8NpObject);
 }
 
@@ -179,6 +156,38 @@ NPObject* npCreateV8ScriptObject(NPP npp, v8::Handle<v8::Object> object, DOMWind
     return reinterpret_cast<NPObject*>(v8npObject);
 }
 
+void disposeUnderlyingV8Object(NPObject* npObject)
+{
+    ASSERT(npObject->_class == npScriptObjectClass);
+    V8NPObject* v8NpObject = reinterpret_cast<V8NPObject*>(npObject);
+    if (v8NpObject->v8Object.IsEmpty())
+        return;
+    if (int v8ObjectHash = v8NpObject->v8Object->GetIdentityHash()) {
+        V8NPObjectMap::iterator iter = staticV8NPObjectMap()->find(v8ObjectHash);
+        if (iter != staticV8NPObjectMap()->end()) {
+            V8NPObjectVector& objects = iter->second;
+            for (size_t index = 0; index < objects.size(); ++index) {
+                if (objects.at(index) == v8NpObject) {
+                    objects.remove(index);
+                    break;
+                }
+            }
+            if (objects.isEmpty())
+                staticV8NPObjectMap()->remove(v8ObjectHash);
+        } else
+            ASSERT_NOT_REACHED();
+    } else {
+        ASSERT(!v8::Context::InContext());
+        staticV8NPObjectMap()->clear();
+    }
+
+#ifndef NDEBUG
+    V8GCController::unregisterGlobalHandle(v8NpObject, v8NpObject->v8Object);
+#endif
+    v8NpObject->v8Object.Dispose();
+    v8NpObject->v8Object.Clear();
+}
+
 } // namespace WebCore
 
 bool _NPN_Invoke(NPP npp, NPObject* npObject, NPIdentifier methodName, const NPVariant* arguments, uint32_t argumentCount, NPVariant* result)
@@ -195,6 +204,8 @@ bool _NPN_Invoke(NPP npp, NPObject* npObject, NPIdentifier methodName, const NPV
     }
 
     V8NPObject* v8NpObject = reinterpret_cast<V8NPObject*>(npObject);
+    if (v8NpObject->v8Object.IsEmpty())
+        return false;
 
     PrivateIdentifier* identifier = static_cast<PrivateIdentifier*>(methodName);
     if (!identifier->isString)
@@ -259,6 +270,8 @@ bool _NPN_InvokeDefault(NPP npp, NPObject* npObject, const NPVariant* arguments,
     }
 
     V8NPObject* v8NpObject = reinterpret_cast<V8NPObject*>(npObject);
+    if (v8NpObject->v8Object.IsEmpty())
+        return false;
 
     VOID_TO_NPVARIANT(*result);
 
@@ -305,13 +318,8 @@ bool _NPN_EvaluateHelper(NPP npp, bool popupsAllowed, NPObject* npObject, NPStri
     if (!npObject)
         return false;
 
-    if (npObject->_class != npScriptObjectClass) {
-        // Check if the object passed in is wrapped. If yes, then we need to invoke on the underlying object.
-        NPObject* actualObject = NPObjectWrapper::getUnderlyingNPObject(npObject);
-        if (!actualObject)
-            return false;
-        npObject = actualObject;
-    }
+    if (npObject->_class != npScriptObjectClass)
+        return false;
 
     v8::HandleScope handleScope;
     v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -349,6 +357,8 @@ bool _NPN_GetProperty(NPP npp, NPObject* npObject, NPIdentifier propertyName, NP
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -384,6 +394,8 @@ bool _NPN_SetProperty(NPP npp, NPObject* npObject, NPIdentifier propertyName, co
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -413,6 +425,8 @@ bool _NPN_RemoveProperty(NPP npp, NPObject* npObject, NPIdentifier propertyName)
         return false;
 
     V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+    if (object->v8Object.IsEmpty())
+        return false;
 
     v8::HandleScope handleScope;
     v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -434,6 +448,8 @@ bool _NPN_HasProperty(NPP npp, NPObject* npObject, NPIdentifier propertyName)
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -458,6 +474,8 @@ bool _NPN_HasMethod(NPP npp, NPObject* npObject, NPIdentifier methodName)
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -502,6 +520,8 @@ bool _NPN_Enumerate(NPP npp, NPObject* npObject, NPIdentifier** identifier, uint
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
@@ -557,6 +577,8 @@ bool _NPN_Construct(NPP npp, NPObject* npObject, const NPVariant* arguments, uin
 
     if (npObject->_class == npScriptObjectClass) {
         V8NPObject* object = reinterpret_cast<V8NPObject*>(npObject);
+        if (object->v8Object.IsEmpty())
+            return false;
 
         v8::HandleScope handleScope;
         v8::Handle<v8::Context> context = toV8Context(npp, npObject);
index f72ba8b..b7e0464 100644 (file)
@@ -54,8 +54,9 @@ WrapperTypeInfo* npObjectTypeInfo();
 
 extern NPClass* npScriptObjectClass;
 
-// A V8NPObject is a NPObject which carries additional V8-specific information. It is allocated and deallocated by
-// AllocV8NPObject() and FreeV8NPObject() methods.
+// A V8NPObject is a NPObject which carries additional V8-specific information.
+// It is created with npCreateV8ScriptObject() and deallocated via the deallocate
+// method in the same way as other NPObjects.
 struct V8NPObject {
     NPObject object;
     v8::Persistent<v8::Object> v8Object;
@@ -74,6 +75,8 @@ NPObject* npCreateV8ScriptObject(NPP, v8::Handle<v8::Object>, DOMWindow*);
 
 NPObject* v8ObjectToNPObject(v8::Handle<v8::Object>);
 
+void disposeUnderlyingV8Object(NPObject*);
+
 } // namespace WebCore
 
 #endif // NPV8Object_h
index d49866d..d0a7e06 100644 (file)
@@ -45,7 +45,6 @@
 #include "FrameLoaderClient.h"
 #include "Node.h"
 #include "NotImplemented.h"
-#include "NPObjectWrapper.h"
 #include "npruntime_impl.h"
 #include "npruntime_priv.h"
 #include "NPV8Object.h"
@@ -110,7 +109,7 @@ ScriptController::ScriptController(Frame* frame)
     , m_paused(false)
     , m_proxy(adoptPtr(new V8Proxy(frame)))
 #if ENABLE(NETSCAPE_PLUGIN_API)
-    , m_wrappedWindowScriptNPObject(0)
+    , m_windowScriptNPObject(0)
 #endif
 {
 }
@@ -129,21 +128,14 @@ void ScriptController::clearScriptObjects()
     m_pluginObjects.clear();
 
 #if ENABLE(NETSCAPE_PLUGIN_API)
-    if (m_wrappedWindowScriptNPObject) {
-        NPObjectWrapper* windowScriptObjectWrapper = NPObjectWrapper::getWrapper(m_wrappedWindowScriptNPObject);
-        ASSERT(windowScriptObjectWrapper);
-
-        NPObject* windowScriptNPObject = NPObjectWrapper::getUnderlyingNPObject(m_wrappedWindowScriptNPObject);
-        ASSERT(windowScriptNPObject);
-        // Call _NPN_DeallocateObject() instead of _NPN_ReleaseObject() so that we don't leak if a plugin fails to release the window
-        // script object properly.
-        // This shouldn't cause any problems for plugins since they should have already been stopped and destroyed at this point.
-        _NPN_DeallocateObject(windowScriptNPObject);
-
-        // Clear out the wrapped window script object pointer held by the wrapper.
-        windowScriptObjectWrapper->clear();
-        _NPN_ReleaseObject(m_wrappedWindowScriptNPObject);
-        m_wrappedWindowScriptNPObject = 0;
+    if (m_windowScriptNPObject) {
+        // Dispose of the underlying V8 object before releasing our reference
+        // to it, so that if the plugin fails to release it properly we will
+        // only leak the NPObject wrapper, not the object, its document, or
+        // anything else they reference.
+        disposeUnderlyingV8Object(m_windowScriptNPObject);
+        _NPN_ReleaseObject(m_windowScriptNPObject);
+        m_windowScriptNPObject = 0;
     }
 #endif
 }
@@ -391,24 +383,21 @@ static NPObject* createScriptObject(Frame* frame)
 
 NPObject* ScriptController::windowScriptNPObject()
 {
-    if (m_wrappedWindowScriptNPObject)
-        return m_wrappedWindowScriptNPObject;
+    if (m_windowScriptNPObject)
+        return m_windowScriptNPObject;
 
-    NPObject* windowScriptNPObject = 0;
     if (canExecuteScripts(NotAboutToExecuteScript)) {
         // JavaScript is enabled, so there is a JavaScript window object.
         // Return an NPObject bound to the window object.
-        windowScriptNPObject = createScriptObject(m_frame);
-        _NPN_RegisterObject(windowScriptNPObject, 0);
+        m_windowScriptNPObject = createScriptObject(m_frame);
+        _NPN_RegisterObject(m_windowScriptNPObject, 0);
     } else {
         // JavaScript is not enabled, so we cannot bind the NPObject to the
         // JavaScript window object. Instead, we create an NPObject of a
         // different class, one which is not bound to a JavaScript object.
-        windowScriptNPObject = createNoScriptObject();
+        m_windowScriptNPObject = createNoScriptObject();
     }
-
-    m_wrappedWindowScriptNPObject = NPObjectWrapper::create(windowScriptNPObject);
-    return m_wrappedWindowScriptNPObject;
+    return m_windowScriptNPObject;
 }
 
 NPObject* ScriptController::createScriptObjectForPluginElement(HTMLPlugInElement* plugin)
index 41d71ce..7c78368 100644 (file)
@@ -205,14 +205,9 @@ private:
     // invalidate all sub-objects which are associated with that plugin.
     // The frame keeps a NPObject reference for each item on the list.
     PluginObjectMap m_pluginObjects;
-    // The window script object can get destroyed while there are outstanding
-    // references to it. Please refer to ScriptController::clearScriptObjects
-    // for more information as to why this is necessary. To avoid crashes due
-    // to calls on the destroyed window object, we return a proxy NPObject
-    // which wraps the underlying window object. The wrapped window object
-    // pointer in this object is cleared out when the window object is
-    // destroyed.
-    NPObject* m_wrappedWindowScriptNPObject;
+#if ENABLE(NETSCAPE_PLUGIN_API)
+    NPObject* m_windowScriptNPObject;
+#endif
 };
 
 } // namespace WebCore
index c4f40df..232b5b4 100644 (file)
@@ -210,6 +210,8 @@ static bool getRangeImpl(NPObject* object, WebRange* webRange)
 
     V8NPObject* v8NPObject = reinterpret_cast<V8NPObject*>(object);
     v8::Handle<v8::Object> v8Object(v8NPObject->v8Object);
+    if (v8Object.IsEmpty())
+        return false;
     if (!V8Range::info.equals(V8DOMWrapper::domWrapperType(v8Object)))
         return false;
 
@@ -228,6 +230,8 @@ static bool getElementImpl(NPObject* object, WebElement* webElement)
 
     V8NPObject* v8NPObject = reinterpret_cast<V8NPObject*>(object);
     v8::Handle<v8::Object> v8Object(v8NPObject->v8Object);
+    if (v8Object.IsEmpty())
+        return false;
     Element* native = V8Element::HasInstance(v8Object) ? V8Element::toNative(v8Object) : 0;
     if (!native)
         return false;
@@ -243,6 +247,8 @@ static bool getArrayBufferImpl(NPObject* object, WebArrayBuffer* arrayBuffer)
 
     V8NPObject* v8NPObject = reinterpret_cast<V8NPObject*>(object);
     v8::Handle<v8::Object> v8Object(v8NPObject->v8Object);
+    if (v8Object.IsEmpty())
+        return false;
     ArrayBuffer* native = V8ArrayBuffer::HasInstance(v8Object) ? V8ArrayBuffer::toNative(v8Object) : 0;
     if (!native)
         return false;
@@ -258,6 +264,8 @@ static bool getArrayBufferViewImpl(NPObject* object, WebArrayBufferView* arrayBu
 
     V8NPObject* v8NPObject = reinterpret_cast<V8NPObject*>(object);
     v8::Handle<v8::Object> v8Object(v8NPObject->v8Object);
+    if (v8Object.IsEmpty())
+        return false;
     ArrayBufferView* native = V8ArrayBufferView::HasInstance(v8Object) ? V8ArrayBufferView::toNative(v8Object) : 0;
     if (!native)
         return false;
@@ -373,6 +381,8 @@ v8::Handle<v8::Value> WebBindings::toV8Value(const NPVariant* variant)
         if (object->_class != npScriptObjectClass)
             return v8::Undefined();
         V8NPObject* v8Object = reinterpret_cast<V8NPObject*>(object);
+        if (v8Object->v8Object.IsEmpty())
+            return v8::Undefined();
         return convertNPVariantToV8Object(variant, v8Object->rootObject->frame()->script()->windowScriptNPObject());
     }
     // Safe to pass 0 since we have checked the script object class to make sure the
index 5dd053b..5ee0221 100644 (file)
@@ -1,3 +1,22 @@
+2012-06-29  James Weatherall  <wez@chromium.org>
+
+        NPObjectWrapper may not address all window script object lifetime issues
+        https://bugs.webkit.org/show_bug.cgi?id=85679
+
+        TestNetscapePlugin now has a leak-window-scriptable-object test which takes a reference to the window script object, and a second reference to it via the "self" property, and does not release those references. This is used to simulate a leaky plugin in layout tests of the NPAPI scripting interface glue code.
+
+        Reviewed by Nate Chapin.
+
+        * DumpRenderTree/DumpRenderTree.gypi:
+        * DumpRenderTree/TestNetscapePlugIn/PluginTest.cpp:
+        (PluginTest::NPN_GetProperty):
+        * DumpRenderTree/TestNetscapePlugIn/PluginTest.h:
+        (PluginTest):
+        * DumpRenderTree/TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp: Added.
+        (LeakWindowScriptableObject):
+        (LeakWindowScriptableObject::LeakWindowScriptableObject):
+        (LeakWindowScriptableObject::NPP_New):
+
 2012-06-29  Dirk Pranke  <dpranke@chromium.org>
 
         webkitpy: add comment about how determine_full_port_name() works for apple ports, fix -wk2 bug
index 5412a19..a414fd2 100644 (file)
@@ -80,6 +80,7 @@
             'TestNetscapePlugIn/Tests/GetURLWithJavaScriptURL.cpp',
             'TestNetscapePlugIn/Tests/GetURLWithJavaScriptURLDestroyingPlugin.cpp',
             'TestNetscapePlugIn/Tests/GetUserAgentWithNullNPPFromNPPNew.cpp',
+            'TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp',
             'TestNetscapePlugIn/Tests/NPRuntimeObjectFromDestroyedPlugin.cpp',
             'TestNetscapePlugIn/Tests/NPRuntimeRemoveProperty.cpp',
             'TestNetscapePlugIn/Tests/NullNPPGetValuePointer.cpp',
index 65dcaec..a3e9783 100644 (file)
@@ -208,6 +208,11 @@ void PluginTest::NPN_ReleaseObject(NPObject* npObject)
     browser->releaseobject(npObject);
 }
 
+bool PluginTest::NPN_GetProperty(NPObject* npObject, NPIdentifier propertyName, NPVariant* value)
+{
+    return browser->getproperty(m_npp, npObject, propertyName, value);
+}
+
 bool PluginTest::NPN_RemoveProperty(NPObject* npObject, NPIdentifier propertyName)
 {
     return browser->removeproperty(m_npp, npObject, propertyName);
index e83e82c..e7cd903 100644 (file)
@@ -65,7 +65,6 @@ public:
     virtual NPError NPP_DestroyStream(NPStream*, NPReason);
     virtual int32_t NPP_WriteReady(NPStream*);
     virtual int32_t NPP_Write(NPStream*, int32_t offset, int32_t len, void* buffer);
-    
     virtual int16_t NPP_HandleEvent(void* event);
     virtual bool NPP_URLNotify(const char* url, NPReason, void* notifyData);
     virtual NPError NPP_GetValue(NPPVariable, void* value);
@@ -87,6 +86,7 @@ public:
     NPObject* NPN_CreateObject(NPClass*);
     NPObject* NPN_RetainObject(NPObject*);
     void NPN_ReleaseObject(NPObject*);
+    bool NPN_GetProperty(NPObject*, NPIdentifier propertyName, NPVariant* value);
     bool NPN_RemoveProperty(NPObject*, NPIdentifier propertyName);
 
 #ifdef XP_MACOSX
diff --git a/Tools/DumpRenderTree/TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp b/Tools/DumpRenderTree/TestNetscapePlugIn/Tests/LeakWindowScriptableObject.cpp
new file mode 100644 (file)
index 0000000..93f52a2
--- /dev/null
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2012 Google Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "PluginTest.h"
+
+using namespace std;
+
+class LeakWindowScriptableObject : public PluginTest {
+public:
+    LeakWindowScriptableObject(NPP npp, const string& identifier)
+        : PluginTest(npp, identifier)
+    {
+    }
+
+private:
+    virtual NPError NPP_New(NPMIMEType pluginType, uint16_t mode, int16_t argc, char *argn[], char *argv[], NPSavedData *saved)
+    {
+        // Get a new reference to the window script object.
+        NPObject* window;
+        if (NPN_GetValue(NPNVWindowNPObject, &window) != NPERR_NO_ERROR) {
+            log("Fail: Cannot fetch window script object");
+            return NPERR_NO_ERROR;
+        }
+
+        // Get another reference to the same object via window.self.
+        NPIdentifier self_name = NPN_GetStringIdentifier("self");
+        NPVariant window_self_variant;
+        if (!NPN_GetProperty(window, self_name, &window_self_variant)) {
+            log("Fail: Cannot query window.self");
+            return NPERR_NO_ERROR;
+        }
+        if (!NPVARIANT_IS_OBJECT(window_self_variant)) {
+            log("Fail: window.self is not an object");
+            return NPERR_NO_ERROR;
+        }
+
+        // Leak both references to the window script object.
+        return NPERR_NO_ERROR;
+    }
+};
+
+static PluginTest::Register<LeakWindowScriptableObject> leakWindowScriptableObject("leak-window-scriptable-object");