Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Issue 297193004: Fix the "PersistentValueMap" memory leak reported here: (Closed)

Created:
6 years, 7 months ago by vogelheim
Modified:
6 years, 7 months ago
Reviewers:
dcarney
CC:
v8-dev, Michael Starzinger
Visibility:
Public.

Description

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= Committed: https://code.google.com/p/v8/source/detail?r=21514

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M include/v8-util.h View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 3 (0 generated)
vogelheim
6 years, 7 months ago (2014-05-26 17:54:42 UTC) #1
dcarney
lgtm
6 years, 7 months ago (2014-05-26 18:10:15 UTC) #2
vogelheim
6 years, 7 months ago (2014-05-27 09:31:21 UTC) #3
Message was sent while issue was closed.
Committed patchset #1 manually as r21514 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698