[Webkit-unassigned] [Bug 62345] [V8][Chromium] Use per-isolate embedder data instead of statics for caches in bindings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62345


Hans Wennborg <hans at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hans at chromium.org




--- Comment #21 from Hans Wennborg <hans at chromium.org>  2011-07-04 08:19:54 PST ---
Hi Dmitry,

This patch causes problems for Indexed DB. It seems that when creating and destroying many V8LocalContext objects, memory is not free'd properly, and we eventually run out of it.

This diff adds a test to webkit_unit_tests that exposes the problem (make sure to build in Release mode, this is pretty slow otherwise):

diff --git a/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp b/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
index 3647ce6..aad9f02 100644
--- a/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
+++ b/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
@@ -106,6 +106,14 @@ TEST(IDBKeyFromValueAndKeyPathTest, TopLevelPropertyStringValue)
     checkKeyPathNullValue(serializedScriptValue.get(), "[3]");
 }

+TEST(IDBKeyFromValueAndKeyPathTest, ManyV8LocalContexts)
+{
+    for (int i = 0; i < 100000; ++i) {
+        printf("%d\n", i);
+        V8LocalContext v8context;
+    }
+}
+


There seems to be something subtle going wrong with the use of OwnHandle in V8LocalContext. If I apply the following patch, the problem goes away, but I'm not sure why; can you take a look?


diff --git a/Source/WebCore/bindings/v8/V8Utilities.cpp b/Source/WebCore/bindings/v8/V8Utilities.cpp
index 490cb74..4f783b6 100644
--- a/Source/WebCore/bindings/v8/V8Utilities.cpp
+++ b/Source/WebCore/bindings/v8/V8Utilities.cpp
@@ -49,16 +49,17 @@
 namespace WebCore {

 V8LocalContext::V8LocalContext()
+    : m_context(v8::Context::New())
 {
     V8BindingPerIsolateData::ensureInitialized(v8::Isolate::GetCurrent());
-    m_context.set(v8::Context::New());
-    m_context.get()->Enter();
+    m_context->Enter();
 }


-V8LocalContext::~V8LocalContext() 
+V8LocalContext::~V8LocalContext()
 {
-    m_context.get()->Exit();
+    m_context->Exit();
+    m_context.Dispose();
 }

 // Use an array to hold dependents. It works like a ref-counted scheme.
diff --git a/Source/WebCore/bindings/v8/V8Utilities.h b/Source/WebCore/bindings/v8/V8Utilities.h
index 6f48907..c217964 100644
--- a/Source/WebCore/bindings/v8/V8Utilities.h
+++ b/Source/WebCore/bindings/v8/V8Utilities.h
@@ -71,7 +71,7 @@ namespace WebCore {
         virtual ~V8LocalContext();
     private:
         v8::HandleScope m_handleScope;
-        OwnHandle<v8::Context> m_context;
+        v8::Persistent<v8::Context> m_context;

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.