Revision: 21514
Author: [email protected]
Date: Tue May 27 09:31:06 2014 UTC
Log: Fix the "PersistentValueMap" memory leak reported here:
http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN
The bug:
The code assumed that a weak Persistent whose weak callback is being
called would still be weak. That isn't true since the persistent is
un-weakened by the garbage collector before calling the weak callback. [1]
Specifically, PersistentValueMap would funnel all 'remove' actions
through its Release method, which uses PersistentBase::ClearWeak to
obtain the callback data. [2] For 'removes' caused by the weak callback,
ClearWeak always returns a NULL-pointer since by that time the weak
persistent was already un-weakend. The result was a memory leak in
the test, since the code to delete the weak callback data would
delete NULL.
The fix:
I explicity call Traits::DisposeCallbackData from the weak callback
with the data obtained from the v8::WeakCallbackData. To avoid invalid
calls to DisposeCallbackData, I also check whether this instance is
(still) weak before calling it. (That check could easily be elided
if it's expensive, for the price of having two 'remove' code paths.)
Severety:
Probably low. At least in Chromium, noone uses the API in a way to
trigger this; only the test does.
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/global-handles.cc&q=global-handles.cc&sq=package:chromium&type=cs&l=231
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8-util.h&sq=package:chromium&l=332-345
[email protected], dcarney
BUG=
Review URL: https://codereview.chromium.org/297193004
http://code.google.com/p/v8/source/detail?r=21514
Modified:
/branches/bleeding_edge/include/v8-util.h
=======================================
--- /branches/bleeding_edge/include/v8-util.h Fri May 9 12:59:24 2014 UTC
+++ /branches/bleeding_edge/include/v8-util.h Tue May 27 09:31:06 2014 UTC
@@ -300,6 +300,7 @@
K key = Traits::KeyFromWeakCallbackData(data);
Traits::Dispose(data.GetIsolate(),
persistentValueMap->Remove(key).Pass(), key);
+ Traits::DisposeCallbackData(data.GetParameter());
}
}
@@ -337,7 +338,7 @@
static UniquePersistent<V> Release(PersistentContainerValue v) {
UniquePersistent<V> p;
p.val_ = FromVal(v);
- if (Traits::kCallbackType != kNotWeak && !p.IsEmpty()) {
+ if (Traits::kCallbackType != kNotWeak && p.IsWeak()) {
Traits::DisposeCallbackData(
p.template ClearWeak<typename Traits::WeakCallbackDataType>());
}
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
