From 06086a90e04070c1a558355d43edf9f8b2d2a74f Mon Sep 17 00:00:00 2001 From: "vogelheim@chromium.org" Date: Tue, 27 May 2014 09:31:06 +0000 Subject: [PATCH] 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 R=dcarney@chromium.org, dcarney BUG= Review URL: https://codereview.chromium.org/297193004 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21514 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- include/v8-util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/v8-util.h b/include/v8-util.h index c58f4ca..8654925 100644 --- a/include/v8-util.h +++ b/include/v8-util.h @@ -300,6 +300,7 @@ class PersistentValueMap { K key = Traits::KeyFromWeakCallbackData(data); Traits::Dispose(data.GetIsolate(), persistentValueMap->Remove(key).Pass(), key); + Traits::DisposeCallbackData(data.GetParameter()); } } @@ -337,7 +338,7 @@ class PersistentValueMap { static UniquePersistent Release(PersistentContainerValue v) { UniquePersistent p; p.val_ = FromVal(v); - if (Traits::kCallbackType != kNotWeak && !p.IsEmpty()) { + if (Traits::kCallbackType != kNotWeak && p.IsWeak()) { Traits::DisposeCallbackData( p.template ClearWeak()); } -- 2.7.4