|
|
Created:
6 years, 7 months ago by luken Modified:
6 years, 7 months ago CC:
chromium-reviews, anantha, ashejole_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionfix leak in RTree::Remove()
Caused in the common case when RTree::RemoveNode would remove a non-record-node
child but would have more than min_children_ remaining nodes.
BUG=370779
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269154
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixing nit #Messages
Total messages: 24 (0 generated)
Could you add a unit test that exercises the problematic path? that way it'll be run via ASAN/LSAN and can confirm that the leak is fixed. https://codereview.chromium.org/270373002/diff/1/ui/gfx/geometry/r_tree.cc File ui/gfx/geometry/r_tree.cc (right): https://codereview.chromium.org/270373002/diff/1/ui/gfx/geometry/r_tree.cc#ne... ui/gfx/geometry/r_tree.cc:630: root_.reset(root_->RemoveAndReturnLastChild().release()); nit: root_ = root_->RemoveAndReturnLastChild(); should work
So when I ran LSAN locally almost all of the unit tests reported leaks in this code path, until it was fixed. Not sure why the first RTRee cl was allowed to land, we must not be running LSAN on unit tests. https://codereview.chromium.org/270373002/diff/1/ui/gfx/geometry/r_tree.cc File ui/gfx/geometry/r_tree.cc (right): https://codereview.chromium.org/270373002/diff/1/ui/gfx/geometry/r_tree.cc#ne... ui/gfx/geometry/r_tree.cc:630: root_.reset(root_->RemoveAndReturnLastChild().release()); On 2014/05/07 20:29:09, piman wrote: > nit: root_ = root_->RemoveAndReturnLastChild(); should work Done.
lgtm
The CQ bit was checked by scottmg@chromium.org
The CQ bit was checked by luken@chromium.org
The CQ bit was unchecked by luken@chromium.org
The CQ bit was checked by luken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/luken@chromium.org/270373002/20001
The CQ bit was unchecked by luken@chromium.org
LGTM
The CQ bit was checked by luken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/luken@chromium.org/270373002/20001
The CQ bit was unchecked by luken@chromium.org
Rubber stamp LGTM
The CQ bit was checked by luken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/luken@chromium.org/270373002/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium
The CQ bit was checked by luken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/luken@chromium.org/270373002/20001
Message was sent while issue was closed.
Change committed as 269154 |