- Revision
- 87179
- Author
- aro...@apple.com
- Date
- 2011-05-24 11:51:49 -0700 (Tue, 24 May 2011)
Log Message
Invalidate JS wrappers for NPObjects when they are finalized
This will cause the underlying NPObject to be released at finalization time, rather than at
destruction time (which is unpredictable and could occur after the plugin has been
unloaded).
Test: plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html
Fixes <http://webkit.org/b/61316> <rdar://problem/9489824> Crash in deallocateNPObject when
reloading yahoo.com webarchive in WebKit2
and
<http://webkit.org/b/61317> <rdar://problem/9489829> Crash in _NPN_DeallocateObject when
reloading yahoo.com webarchive in WebKit1
Reviewed by Oliver Hunt.
Source/WebCore:
* bridge/runtime_object.cpp:
(JSC::Bindings::RuntimeObject::~RuntimeObject): Assert that we've already been invalidated.
* bridge/runtime_root.cpp:
(JSC::Bindings::RootObject::invalidate):
(JSC::Bindings::RootObject::addRuntimeObject):
Updated for m_runtimeObjects type change.
(JSC::Bindings::RootObject::finalize): Added. Invalidates the RuntimeObject and removes it
from the map.
* bridge/runtime_root.h: Now inherits from WeakHandleOwner.
Changed m_runtimeObjects from a WeakGCMap to a HashMap of JSC::Weak objects so that we will
be notified when the RuntimeObjects are finalized.
Source/WebKit2:
* WebProcess/Plugins/Netscape/JSNPObject.cpp:
(WebKit::JSNPObject::~JSNPObject): Assert that we've already been invalidated, rather than
trying to perform invalidation now (when the plugin might already be unloaded).
* WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:
(WebKit::NPRuntimeObjectMap::getOrCreateJSObject):
(WebKit::NPRuntimeObjectMap::invalidate):
Updated for m_jsNPObjects type change.
(WebKit::NPRuntimeObjectMap::finalize): Added. Invalidates the JSNPObject and removes it
from the map.
* WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h: Now inherits from WeakHandleOwner.
Changed m_jsNPObjects from a WeakGCMap to a HashMap of JSC::Weak objects so that we will be
notified when the JSNPObjects are finalized.
LayoutTests:
Test that we don't crash when a JS wrapper for an NPObject is destroyed after its plugin is unloaded
* plugins/npobject-js-wrapper-destroyed-after-plugin-unload-expected.txt: Added.
* plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html: Added.
(startTest): Gets a JS wrapper for an NPObject from the plugin, allocate a bunch of memory
so the JS wrapper will be finalized, then destroy the plugin and wait for a little bit
before calling finishTest.
(finishTest): Force a GC so the JS wrapper will be destroyed. If we didn't crash, we passed!
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (87178 => 87179)
--- trunk/LayoutTests/ChangeLog 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/LayoutTests/ChangeLog 2011-05-24 18:51:49 UTC (rev 87179)
@@ -1,3 +1,24 @@
+2011-05-24 Adam Roben <aro...@apple.com>
+
+ Test that we don't crash when a JS wrapper for an NPObject is destroyed after its plugin is unloaded
+
+ Test for <http://webkit.org/b/61316> <rdar://problem/9489824> Crash in deallocateNPObject
+ when reloading yahoo.com webarchive in WebKit2
+
+ and
+
+ <http://webkit.org/b/61317> <rdar://problem/9489829> Crash in _NPN_DeallocateObject when
+ reloading yahoo.com webarchive in WebKit1
+
+ Reviewed by Oliver Hunt.
+
+ * plugins/npobject-js-wrapper-destroyed-after-plugin-unload-expected.txt: Added.
+ * plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html: Added.
+ (startTest): Gets a JS wrapper for an NPObject from the plugin, allocate a bunch of memory
+ so the JS wrapper will be finalized, then destroy the plugin and wait for a little bit
+ before calling finishTest.
+ (finishTest): Force a GC so the JS wrapper will be destroyed. If we didn't crash, we passed!
+
2011-05-24 Adam Klein <ad...@chromium.org>
Unreviewed, rolling out r87145.
Added: trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload-expected.txt (0 => 87179)
--- trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload-expected.txt (rev 0)
+++ trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload-expected.txt 2011-05-24 18:51:49 UTC (rev 87179)
@@ -0,0 +1,3 @@
+This test will only work in DumpRenderTree/WebKitTestRunner.
+
+PASSED
Added: trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html (0 => 87179)
--- trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html (rev 0)
+++ trunk/LayoutTests/plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html 2011-05-24 18:51:49 UTC (rev 87179)
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <script>
+ function startTest() {
+ if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+ }
+
+ // Access all objects/properties that we're going to use later in the test so that JS
+ // allocations only happen when we expect.
+ var body = document.body;
+ body.removeChild;
+ var plugin = body.getElementsByTagName('embed')[0];
+ var testObject = plugin.testObject;
+ setTimeout;
+
+ testObject = null;
+
+ // Allocate a bunch of JS memory. This should cause testObject to be finalized, but it's
+ // destructor shouldn't run until the GCController.collect call we make later.
+ var array = new Array(10000);
+ for (var i = 0; i < 10000; ++i)
+ array[i] = new Object();
+
+ // Remove the plugin and wait for a little bit to ensure it has been unloaded (WebKit1
+ // on Windows unloads plugins after a delay).
+ body.removeChild(plugin);
+ setTimeout(finishTest, 250);
+ }
+
+ function finishTest() {
+ // Force a GC. If we don't crash here, we've passed the test.
+ if (window.GCController)
+ GCController.collect();
+
+ document.body.appendChild(document.createTextNode('PASSED'));
+
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+ }
+
+ addEventListener('load', startTest, false);
+ </script>
+</head>
+<body>
+ <p>This test will only work in DumpRenderTree/WebKitTestRunner.</p>
+ <embed type="application/x-webkit-test-netscape">
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (87178 => 87179)
--- trunk/Source/WebCore/ChangeLog 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebCore/ChangeLog 2011-05-24 18:51:49 UTC (rev 87179)
@@ -1,3 +1,33 @@
+2011-05-24 Adam Roben <aro...@apple.com>
+
+ Invalidate RuntimeObjects when they are finalized
+
+ This will cause the underlying NPObject to be released at finalization time, rather than at
+ destruction time (which is unpredictable and could occur after the plugin has been
+ unloaded).
+
+ Test: plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html
+
+ Fixes <http://webkit.org/b/61317> <rdar://problem/9489829> Crash in _NPN_DeallocateObject
+ when reloading yahoo.com webarchive in WebKit1
+
+ Reviewed by Oliver Hunt.
+
+ * bridge/runtime_object.cpp:
+ (JSC::Bindings::RuntimeObject::~RuntimeObject): Assert that we've already been invalidated.
+
+ * bridge/runtime_root.cpp:
+ (JSC::Bindings::RootObject::invalidate):
+ (JSC::Bindings::RootObject::addRuntimeObject):
+ Updated for m_runtimeObjects type change.
+
+ (JSC::Bindings::RootObject::finalize): Added. Invalidates the RuntimeObject and removes it
+ from the map.
+
+ * bridge/runtime_root.h: Now inherits from WeakHandleOwner.
+ Changed m_runtimeObjects from a WeakGCMap to a HashMap of JSC::Weak objects so that we will
+ be notified when the RuntimeObjects are finalized.
+
2011-05-24 Mike Reed <r...@google.com>
Reviewed by Kenneth Russell.
Modified: trunk/Source/WebCore/bridge/runtime_object.cpp (87178 => 87179)
--- trunk/Source/WebCore/bridge/runtime_object.cpp 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebCore/bridge/runtime_object.cpp 2011-05-24 18:51:49 UTC (rev 87179)
@@ -47,6 +47,7 @@
RuntimeObject::~RuntimeObject()
{
+ ASSERT(!m_instance);
}
void RuntimeObject::invalidate()
Modified: trunk/Source/WebCore/bridge/runtime_root.cpp (87178 => 87179)
--- trunk/Source/WebCore/bridge/runtime_root.cpp 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebCore/bridge/runtime_root.cpp 2011-05-24 18:51:49 UTC (rev 87179)
@@ -101,9 +101,9 @@
return;
{
- WeakGCMap<RuntimeObject*, RuntimeObject>::iterator end = m_runtimeObjects.end();
- for (WeakGCMap<RuntimeObject*, RuntimeObject>::iterator it = m_runtimeObjects.begin(); it != end; ++it) {
- it.get().second->invalidate();
+ HashMap<RuntimeObject*, JSC::Weak<RuntimeObject> >::iterator end = m_runtimeObjects.end();
+ for (HashMap<RuntimeObject*, JSC::Weak<RuntimeObject> >::iterator it = m_runtimeObjects.begin(); it != end; ++it) {
+ it->second.get()->invalidate();
}
m_runtimeObjects.clear();
@@ -179,7 +179,7 @@
ASSERT(m_isValid);
ASSERT(!m_runtimeObjects.get(object));
- m_runtimeObjects.set(globalData, object, object);
+ m_runtimeObjects.set(object, JSC::Weak<RuntimeObject>(globalData, object, this));
}
void RootObject::removeRuntimeObject(RuntimeObject* object)
@@ -192,4 +192,13 @@
m_runtimeObjects.take(object);
}
+void RootObject::finalize(JSC::Handle<JSC::Unknown> handle, void*)
+{
+ RuntimeObject* object = static_cast<RuntimeObject*>(asObject(handle.get()));
+ ASSERT(m_runtimeObjects.contains(object));
+
+ object->invalidate();
+ m_runtimeObjects.remove(object);
+}
+
} } // namespace JSC::Bindings
Modified: trunk/Source/WebCore/bridge/runtime_root.h (87178 => 87179)
--- trunk/Source/WebCore/bridge/runtime_root.h 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebCore/bridge/runtime_root.h 2011-05-24 18:51:49 UTC (rev 87179)
@@ -31,8 +31,10 @@
#endif
#include <heap/Strong.h>
-#include <runtime/WeakGCMap.h>
+#include <_javascript_Core/HandleHeap.h>
#include <wtf/Forward.h>
+#include <wtf/HashCountedSet.h>
+#include <wtf/HashSet.h>
#include <wtf/Noncopyable.h>
#include <wtf/PassRefPtr.h>
#include <wtf/RefCounted.h>
@@ -52,7 +54,7 @@
extern RootObject* findProtectingRootObject(JSObject*);
extern RootObject* findRootObject(JSGlobalObject*);
-class RootObject : public RefCounted<RootObject> {
+class RootObject : public RefCounted<RootObject>, private JSC::WeakHandleOwner {
friend class JavaJSObject;
public:
@@ -82,14 +84,17 @@
private:
RootObject(const void* nativeHandle, JSGlobalObject*);
-
+
+ // WeakHandleOwner
+ virtual void finalize(JSC::Handle<JSC::Unknown>, void* context);
+
bool m_isValid;
const void* m_nativeHandle;
Strong<JSGlobalObject> m_globalObject;
ProtectCountSet m_protectCountSet;
- WeakGCMap<RuntimeObject*, RuntimeObject> m_runtimeObjects; // Really need a WeakGCSet, but this will do.
+ HashMap<RuntimeObject*, JSC::Weak<RuntimeObject> > m_runtimeObjects; // Really need a WeakGCSet, but this will do.
HashSet<InvalidationCallback*> m_invalidationCallbacks;
};
Modified: trunk/Source/WebKit2/ChangeLog (87178 => 87179)
--- trunk/Source/WebKit2/ChangeLog 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebKit2/ChangeLog 2011-05-24 18:51:49 UTC (rev 87179)
@@ -1,3 +1,34 @@
+2011-05-24 Adam Roben <aro...@apple.com>
+
+ Invalidate JSNPObjects when they are finalized
+
+ This will cause the underlying NPObject to be released at finalization time, rather than at
+ destruction time (which is unpredictable and could occur after the plugin has been
+ unloaded).
+
+ Test: plugins/npobject-js-wrapper-destroyed-after-plugin-unload.html
+
+ Fixes <http://webkit.org/b/61316> <rdar://problem/9489824> Crash in deallocateNPObject when
+ reloading yahoo.com webarchive in WebKit2
+
+ Reviewed by Oliver Hunt.
+
+ * WebProcess/Plugins/Netscape/JSNPObject.cpp:
+ (WebKit::JSNPObject::~JSNPObject): Assert that we've already been invalidated, rather than
+ trying to perform invalidation now (when the plugin might already be unloaded).
+
+ * WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp:
+ (WebKit::NPRuntimeObjectMap::getOrCreateJSObject):
+ (WebKit::NPRuntimeObjectMap::invalidate):
+ Updated for m_jsNPObjects type change.
+
+ (WebKit::NPRuntimeObjectMap::finalize): Added. Invalidates the JSNPObject and removes it
+ from the map.
+
+ * WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h: Now inherits from WeakHandleOwner.
+ Changed m_jsNPObjects from a WeakGCMap to a HashMap of JSC::Weak objects so that we will be
+ notified when the JSNPObjects are finalized.
+
2011-05-24 Qi Zhang <qi.2.zh...@nokia.com>
Reviewed by Adam Roben.
Modified: trunk/Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp (87178 => 87179)
--- trunk/Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp 2011-05-24 18:51:49 UTC (rev 87179)
@@ -65,9 +65,7 @@
JSNPObject::~JSNPObject()
{
- if (!m_npObject)
- return;
- releaseNPObject(m_npObject);
+ ASSERT(!m_npObject);
}
void JSNPObject::invalidate()
Modified: trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp (87178 => 87179)
--- trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.cpp 2011-05-24 18:51:49 UTC (rev 87179)
@@ -95,11 +95,11 @@
if (NPJSObject::isNPJSObject(npObject))
return NPJSObject::toNPJSObject(npObject)->jsObject();
- if (JSNPObject* jsNPObject = m_jsNPObjects.get(npObject))
- return jsNPObject;
+ if (JSC::Weak<JSNPObject> jsNPObject = m_jsNPObjects.get(npObject))
+ return jsNPObject.get();
JSNPObject* jsNPObject = new (&globalObject->globalData()) JSNPObject(globalObject, this, npObject);
- m_jsNPObjects.set(globalObject->globalData(), npObject, jsNPObject);
+ m_jsNPObjects.set(npObject, JSC::Weak<JSNPObject>(globalObject->globalData(), jsNPObject, this, npObject));
return jsNPObject;
}
@@ -217,9 +217,9 @@
// We shouldn't have any NPJSObjects left now.
ASSERT(m_npJSObjects.isEmpty());
- WeakGCMap<NPObject*, JSNPObject>::iterator end = m_jsNPObjects.end();
- for (WeakGCMap<NPObject*, JSNPObject>::iterator ptr = m_jsNPObjects.begin(); ptr != end; ++ptr)
- ptr.get().second->invalidate();
+ HashMap<NPObject*, JSC::Weak<JSNPObject> >::iterator end = m_jsNPObjects.end();
+ for (HashMap<NPObject*, JSC::Weak<JSNPObject> >::iterator ptr = m_jsNPObjects.begin(); ptr != end; ++ptr)
+ ptr->second.get()->invalidate();
m_jsNPObjects.clear();
}
@@ -265,4 +265,14 @@
globalExceptionString() = String();
}
+void NPRuntimeObjectMap::finalize(JSC::Handle<JSC::Unknown> handle, void* context)
+{
+ HashMap<NPObject*, JSC::Weak<JSNPObject> >::iterator found = m_jsNPObjects.find(static_cast<NPObject*>(context));
+ ASSERT(found != m_jsNPObjects.end());
+ ASSERT_UNUSED(handle, asObject(handle.get()) == found->second);
+
+ found->second.get()->invalidate();
+ m_jsNPObjects.remove(found);
+}
+
} // namespace WebKit
Modified: trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h (87178 => 87179)
--- trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h 2011-05-24 18:50:48 UTC (rev 87178)
+++ trunk/Source/WebKit2/WebProcess/Plugins/Netscape/NPRuntimeObjectMap.h 2011-05-24 18:51:49 UTC (rev 87179)
@@ -26,7 +26,7 @@
#ifndef NPJSObjectWrapperMap_h
#define NPJSObjectWrapperMap_h
-#include <_javascript_Core/WeakGCMap.h>
+#include <_javascript_Core/HandleHeap.h>
#include <wtf/Forward.h>
#include <wtf/HashMap.h>
@@ -48,7 +48,7 @@
class PluginView;
// A per plug-in map of NPObjects that wrap _javascript_ objects.
-class NPRuntimeObjectMap {
+class NPRuntimeObjectMap : private JSC::WeakHandleOwner {
public:
explicit NPRuntimeObjectMap(PluginView*);
@@ -85,10 +85,13 @@
static void moveGlobalExceptionToExecState(JSC::ExecState*);
private:
+ // WeakHandleOwner
+ virtual void finalize(JSC::Handle<JSC::Unknown>, void* context);
+
PluginView* m_pluginView;
HashMap<JSC::JSObject*, NPJSObject*> m_npJSObjects;
- JSC::WeakGCMap<NPObject*, JSNPObject> m_jsNPObjects;
+ HashMap<NPObject*, JSC::Weak<JSNPObject> > m_jsNPObjects;
};
} // namespace WebKit
