Bug 62345

Created attachment 96525 [details] Proposed fix

Comment on attachment 96525 [details] Proposed fix Not asking for cq, need trybots results - but would appreciate a review

Comment on attachment 96525 [details] Proposed fix Patch broke chromium unit-tests; I'll be back.

Comment on attachment 96525 [details] Proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=96525&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206 > + V8BindingPerIsolateData::TemplateMap::iterator result = > + data->rawTemplateMap().find(&info); One line pls. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2207 > + if (result != data->rawTemplateMap().end()) { No { pls > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2210 > + v8::HandleScope handle_scope; handle_scope => handleScope > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2223 > + V8BindingPerIsolateData::TemplateMap::iterator result = > + data->templateMap().find(&info); > + if (result != data->templateMap().end()) { > + return result->second; > + } ditto > Source/WebCore/bindings/v8/V8Binding.cpp:72 > + void* data = isolate->GetData(); > + if (data) > + delete static_cast<V8BindingPerIsolateData*>(data); This pattern is really ugly. > Source/WebCore/bindings/v8/V8Binding.cpp:595 > + v8::Persistent<v8::FunctionTemplate>& toStringTemplate = V8BindingPerIsolateData::current()->toStringTemplate(); Using a reference here is very subtle. Maybe use a pointer instead? > Source/WebCore/bindings/v8/V8Binding.h:76 > + V8BindingPerIsolateData(v8::Isolate*); explicit

Comment on attachment 96525 [details] Proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=96525&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206 >> + data->rawTemplateMap().find(&info); > > One line pls. Thanks - will fix. I should also update example generation results in bindings/scripts/test - then the style checker would have caught me. >> Source/WebCore/bindings/v8/V8Binding.cpp:72 >> + delete static_cast<V8BindingPerIsolateData*>(data); > > This pattern is really ugly. What is ugly? How would you write this instead? >> Source/WebCore/bindings/v8/V8Binding.cpp:595 >> + v8::Persistent<v8::FunctionTemplate>& toStringTemplate = V8BindingPerIsolateData::current()->toStringTemplate(); > > Using a reference here is very subtle. Maybe use a pointer instead? I tend to use references when I want to enforce that pointers are not NULL. Tell me If this rubs strongly against webkit tradition - I'll change to pointers.

Created attachment 96747 [details] CR feedback + fixes for chromium tests Chromium trybots successful

Comment on attachment 96747 [details] CR feedback + fixes for chromium tests Attachment 96747 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8826430

Created attachment 96765 [details] Rebased patch

Comment on attachment 96765 [details] Rebased patch Attachment 96765 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8825447

Created attachment 96803 [details] Release mode compilation fixed.

Comment on attachment 96803 [details] Release mode compilation fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=96803&action=review I'm not in love with the raw memory management, but I don't see any other choice. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206 > + if (result != data->rawTemplateMap().end()) return result->second; One statement per line, plz. > Source/WebCore/bindings/v8/V8Binding.cpp:79 > + void* data = isolate->GetData(); > + if (data) > + delete static_cast<V8BindingPerIsolateData*>(data); This code is still ugly, but I guess that's what we get for using void* in the V8 API. This branch is probably not needed. delete 0 is supposed to be safe. > Source/WebCore/bindings/v8/V8Utilities.h:73 > + v8::Persistent<v8::Context> m_context; Please use OwnHandle.

Created attachment 97008 [details] CR feedback fixed

Created attachment 97010 [details] Oops wrong patch uploaded

Attachment 97010 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/V8Utilities.h:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 97010 [details] Oops wrong patch uploaded Clearing flags on attachment: 97010 Committed r88729: <http://trac.webkit.org/changeset/88729>

All reviewed patches have been landed. Closing bug.

Dmitry, may you provide more details why it is needed and what's the performance impact of this change? (In reply to comment #16) > All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > Dmitry, may you provide more details why it is needed and what's the performance impact of this change? This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change.

(In reply to comment #18) > (In reply to comment #17) > > Dmitry, may you provide more details why it is needed and what's the performance impact of this change? > > This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change. That part I understand. I'd be glad to know more why it's needed for in-process webworkers.

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > Dmitry, may you provide more details why it is needed and what's the performance impact of this change? > > > > This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change. > > That part I understand. I'd be glad to know more why it's needed for in-process webworkers. Workers access V8 bindings, starting with V8DedicatedWorkerContext. Therefore they need a per-isolate cache for function templates.

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;

Please file a new bug rather than posting to this closed bug. The problem is that OwnHandle is responsible for calling New and Disposed in a balanced way. Because this code calls New, that New is never balanced with a Dispose, which leads to the leak.

Hans, thanks for tracking this down - I guess it wasn't easy... (In reply to comment #22) > Please file a new bug rather than posting to this closed bug. > > The problem is that OwnHandle is responsible for calling New and Disposed in a balanced way. Because this code calls New, that New is never balanced with a Dispose, which leads to the leak. I do not think you are right: I am not sure what is the problem with the code yet, but OwnHandle only calls New and Dispose on Persistent, and this code calls New on Context. I feel this usage should be OK, but there might be a subtle reason for it not to - maybe I somehow misunderstand OwnHandle? Anyway, it looks like OwnHandle leads to too much subtlety in this code, and direct use of Persistent (as in https://bug-62345-attachments.webkit.org/attachment.cgi?id=96803 or Hans' patch) is much easier to reason about.

OK. The goal of OwnHandle is to make the code less subtle. I guess that means ownhandle needs more work. :( In any case, please feel free to remove the use of ownhandle if its not helpful.