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

Issue 10821091: Fix leak in JsSyncManagerObserver (Closed)

Created:
8 years, 4 months ago by rlarocque
Modified:
8 years, 4 months ago
Reviewers:
Lei Zhang, Nicolas Zea
CC:
chromium-reviews, glider+watch_chromium.org, pam+watch_chromium.org, timurrrr+watch_chromium.org, bruening+watch_chromium.org, Lei Zhang
Visibility:
Public.

Description

Fix leak in JsSyncManagerObserver This leak was introduced in r148926. BUG=129825, 139635 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149062

Patch Set 1 #

Total comments: 2

Patch Set 2 : Remove another suppression #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -18 lines) Patch
M sync/internal_api/js_sync_manager_observer.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M tools/valgrind/memcheck/suppressions.txt View 1 1 chunk +0 lines, -15 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
rlarocque
Please review.
8 years, 4 months ago (2012-07-30 18:59:44 UTC) #1
Nicolas Zea
http://codereview.chromium.org/10821091/diff/1/tools/valgrind/memcheck/suppressions.txt File tools/valgrind/memcheck/suppressions.txt (left): http://codereview.chromium.org/10821091/diff/1/tools/valgrind/memcheck/suppressions.txt#oldcode5751 tools/valgrind/memcheck/suppressions.txt:5751: fun:_ZN6syncer21JsSyncManagerObserver24OnInitializationCompleteERKNS_10WeakHandleINS_9JsBackendEEEbNS_7EnumSetINS_9ModelTypeELS7_2ELS7_16EEE remove this one too?
8 years, 4 months ago (2012-07-30 19:05:11 UTC) #2
rlarocque
Good point. Updated patch removes the other suppression. http://codereview.chromium.org/10821091/diff/1/tools/valgrind/memcheck/suppressions.txt File tools/valgrind/memcheck/suppressions.txt (left): http://codereview.chromium.org/10821091/diff/1/tools/valgrind/memcheck/suppressions.txt#oldcode5751 tools/valgrind/memcheck/suppressions.txt:5751: fun:_ZN6syncer21JsSyncManagerObserver24OnInitializationCompleteERKNS_10WeakHandleINS_9JsBackendEEEbNS_7EnumSetINS_9ModelTypeELS7_2ELS7_16EEE ...
8 years, 4 months ago (2012-07-30 19:39:17 UTC) #3
Nicolas Zea
LGTM
8 years, 4 months ago (2012-07-30 19:40:30 UTC) #4
Lei Zhang
valgrind LGTM
8 years, 4 months ago (2012-07-30 19:41:28 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10821091/4001
8 years, 4 months ago (2012-07-30 19:44:19 UTC) #6
commit-bot: I haz the power
8 years, 4 months ago (2012-07-30 23:09:37 UTC) #7
Change committed as 149062

Powered by Google App Engine
This is Rietveld 408576698