Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
453203002:
Fixing memory leak in TimedMap (Closed)
DescriptionFixing memory leak in TimedMap
BUG=402118
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288950
Patch Set 1 #
Total comments: 6
Patch Set 2 : Switching to scoped_ptr #Patch Set 3 : Re-disabling a few tests #Patch Set 4 : Disabling broken test #
Total comments: 2
Patch Set 5 : Re-enabling RpcHandler tests #Patch Set 6 : Disabling AudioDirectiveListTest #Patch Set 7 : Merging to HEAD #
Messages
Total messages: 36 (0 generated)
https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... File components/copresence/timed_map.h (right): https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:35: delete clock_; No reason for this clock to not be a scoped_ptr. Let's do that instead. https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:57: void set_clock_for_testing(base::TickClock* clock) { Change the clock to a scoped_ptr, pass in a scoped_ptr<base::TickClock> here, remove the (then unnecessary) comment.
https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... File components/copresence/timed_map.h (right): https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:57: void set_clock_for_testing(base::TickClock* clock) { That's what I wrote first. But then how do you call Advance() on the fake clock in the test? You can't use this clock anymore, because it's now a TickClock, not a SimpleTestTickClock. It doesn't have an Advance() method. So you are left with two hacky options: 1. Retain a bare pointer to the test clock in the test, after you give up ownership to the TimedMap. 2. Get the clock back from the TimedMap and cast it back to SimpleTestTickClock. Neither of those seemed better to me than this. But I'm sort of fine with all three options. On 2014/08/09 06:41:41, Rahul Chaturvedi wrote: > Change the clock to a scoped_ptr, pass in a scoped_ptr<base::TickClock> here, > remove the (then unnecessary) comment.
https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... File components/copresence/timed_map.h (right): https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:57: void set_clock_for_testing(base::TickClock* clock) { On 2014/08/09 06:48:23, Charlie wrote: > That's what I wrote first. But then how do you call Advance() on the fake clock > in the test? You can't use this clock anymore, because it's now a TickClock, not > a SimpleTestTickClock. It doesn't have an Advance() method. So you are left with > two hacky options: > > 1. Retain a bare pointer to the test clock in the test, after you give up > ownership to the TimedMap. > 2. Get the clock back from the TimedMap and cast it back to SimpleTestTickClock. > > Neither of those seemed better to me than this. But I'm sort of fine with all > three options. > > On 2014/08/09 06:41:41, Rahul Chaturvedi wrote: > > Change the clock to a scoped_ptr, pass in a scoped_ptr<base::TickClock> here, > > remove the (then unnecessary) comment. > Retain a bare pointer in the test. It is the cleanest solution. I don't want to not use a scoped_ptr to make test code, at best, cleaner.
https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... File components/copresence/timed_map.h (right): https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:35: delete clock_; On 2014/08/09 06:41:42, Rahul Chaturvedi wrote: > No reason for this clock to not be a scoped_ptr. Let's do that instead. Done. https://codereview.chromium.org/453203002/diff/1/components/copresence/timed_... components/copresence/timed_map.h:57: void set_clock_for_testing(base::TickClock* clock) { On 2014/08/09 06:53:57, Rahul Chaturvedi wrote: > On 2014/08/09 06:48:23, Charlie wrote: > > That's what I wrote first. But then how do you call Advance() on the fake > clock > > in the test? You can't use this clock anymore, because it's now a TickClock, > not > > a SimpleTestTickClock. It doesn't have an Advance() method. So you are left > with > > two hacky options: > > > > 1. Retain a bare pointer to the test clock in the test, after you give up > > ownership to the TimedMap. > > 2. Get the clock back from the TimedMap and cast it back to > SimpleTestTickClock. > > > > Neither of those seemed better to me than this. But I'm sort of fine with all > > three options. > > > > On 2014/08/09 06:41:41, Rahul Chaturvedi wrote: > > > Change the clock to a scoped_ptr, pass in a scoped_ptr<base::TickClock> > here, > > > remove the (then unnecessary) comment. > > > > Retain a bare pointer in the test. It is the cleanest solution. I don't want to > not use a scoped_ptr to make test code, at best, cleaner. Done.
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
oh oops. LGTM.
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/20001
The CQ bit was unchecked by rkc@chromium.org
On 2014/08/09 08:56:07, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/20001 Unchecking CQ; it doesn't look like all the fails have been addressed by this change. We still have a fail here, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_valgrind/... That may be a flake but more concerning is, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/... That is a clear fail.
On Sat, Aug 9, 2014 at 4:50 AM, <rkc@chromium.org> wrote: > On 2014/08/09 08:56:07, I haz the power (commit-bot) wrote: > >> CQ is trying da patch. Follow status at >> https://chromium-status.appspot.com/cq/ckehoe@ >> chromium.org/453203002/20001 >> > > Unchecking CQ; it doesn't look like all the fails have been addressed by > this > change. > We still have a fail here, > http://build.chromium.org/p/tryserver.chromium.linux/ > builders/linux_valgrind/builds/21/steps/memory%20test% > 3A%20components/logs/515265C317161D4E > > Same AudioRecorder crash I've been telling you about. See my update on the other thread. > That may be a flake but more concerning is, > http://build.chromium.org/p/tryserver.chromium.win/ > builders/win_chromium_rel/builds/5070 > > That is a clear fail. > > More tests to disable on Windows. The TimedMap leak is failing in the RpcHandler now too, so this fix needs to go in. Charlie Kehoe | Software Engineer | ckehoe@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/09 16:49:53, chromium-reviews wrote: > > Unchecking CQ; it doesn't look like all the fails have been addressed by > > this > > change. > > We still have a fail here, > > http://build.chromium.org/p/tryserver.chromium.linux/ > > builders/linux_valgrind/builds/21/steps/memory%20test% > > 3A%20components/logs/515265C317161D4E > > > > > Same AudioRecorder crash I've been telling you about. See my update on the > other thread. How is this related? This is a memory suppression; what you are seeing locally is a divide by zero. > > That may be a flake but more concerning is, > > http://build.chromium.org/p/tryserver.chromium.win/ > > builders/win_chromium_rel/builds/5070 > > > > That is a clear fail. > > > > > More tests to disable on Windows. The TimedMap leak is failing in the > RpcHandler now too, so this fix needs to go in. Those tests are also disabled for now. Please re-enable them only after letting memory try bots also run on them first. I know some of them are failing for this reason but I didn't investigate all of the fails.
https://codereview.chromium.org/453203002/diff/60001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder_unittest.cc (right): https://codereview.chromium.org/453203002/diff/60001/components/copresence/me... components/copresence/mediums/audio/audio_recorder_unittest.cc:192: // TODO(rkc): These tests are broken on all platforms. I looked at that bot again. It isn't a fail, it is a suppression. Please re-enable this test for other platforms.
On 2014/08/09 19:20:30, Rahul Chaturvedi wrote: > On 2014/08/09 16:49:53, chromium-reviews wrote: > > > Unchecking CQ; it doesn't look like all the fails have been addressed by > > > this > > > change. > > > We still have a fail here, > > > http://build.chromium.org/p/tryserver.chromium.linux/ > > > builders/linux_valgrind/builds/21/steps/memory%20test% > > > 3A%20components/logs/515265C317161D4E > > > > > > > > Same AudioRecorder crash I've been telling you about. See my update on the > > other thread. > > How is this related? This is a memory suppression; what you are seeing locally > is a divide by zero. > > > > > That may be a flake but more concerning is, > > > http://build.chromium.org/p/tryserver.chromium.win/ > > > builders/win_chromium_rel/builds/5070 > > > > > > That is a clear fail. > > > > > > > > More tests to disable on Windows. The TimedMap leak is failing in the > > RpcHandler now too, so this fix needs to go in. > > Those tests are also disabled for now. Please re-enable them only after letting > memory try bots also run on them first. I know some of them are failing for this > reason but I didn't investigate all of the fails. Didn't get an email about that change. Ok, let's wait on this until after the launch meeting.
On 2014/08/09 19:20:30, Rahul Chaturvedi wrote: > On 2014/08/09 16:49:53, chromium-reviews wrote: > > > Unchecking CQ; it doesn't look like all the fails have been addressed by > > > this > > > change. > > > We still have a fail here, > > > http://build.chromium.org/p/tryserver.chromium.linux/ > > > builders/linux_valgrind/builds/21/steps/memory%20test% > > > 3A%20components/logs/515265C317161D4E > > > > > > > > Same AudioRecorder crash I've been telling you about. See my update on the > > other thread. > > How is this related? This is a memory suppression; what you are seeing locally > is a divide by zero. There is something screwed up in the audio input code. The bug seems to result in undefined behavior, including a zero divide and a check failure. We need to talk to Dale and straighten it out. > > > > That may be a flake but more concerning is, > > > http://build.chromium.org/p/tryserver.chromium.win/ > > > builders/win_chromium_rel/builds/5070 > > > > > > That is a clear fail. > > > > > > > > More tests to disable on Windows. The TimedMap leak is failing in the > > RpcHandler now too, so this fix needs to go in. > > Those tests are also disabled for now. Please re-enable them only after letting > memory try bots also run on them first. I know some of them are failing for this > reason but I didn't investigate all of the fails.
https://codereview.chromium.org/453203002/diff/60001/components/copresence/me... File components/copresence/mediums/audio/audio_recorder_unittest.cc (right): https://codereview.chromium.org/453203002/diff/60001/components/copresence/me... components/copresence/mediums/audio/audio_recorder_unittest.cc:192: // TODO(rkc): These tests are broken on all platforms. On 2014/08/09 19:20:37, Rahul Chaturvedi wrote: > I looked at that bot again. It isn't a fail, it is a suppression. Please > re-enable this test for other platforms. This test is breaking the Linux valgrind bot: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/80001
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/copresence/rpc/rpc_handler_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file components/copresence/rpc/rpc_handler_unittest.cc
Hunk #4 FAILED at 249.
1 out of 5 hunks FAILED -- saving rejects to file
components/copresence/rpc/rpc_handler_unittest.cc.rej
Patch: components/copresence/rpc/rpc_handler_unittest.cc
Index: components/copresence/rpc/rpc_handler_unittest.cc
diff --git a/components/copresence/rpc/rpc_handler_unittest.cc
b/components/copresence/rpc/rpc_handler_unittest.cc
index
f6c93594b66915ca60886fbf3a4436f4cea4d821..7f16a8bbc3378d412acbd8bc361727d6a4640f53
100644
--- a/components/copresence/rpc/rpc_handler_unittest.cc
+++ b/components/copresence/rpc/rpc_handler_unittest.cc
@@ -175,11 +175,7 @@ class RpcHandlerTest : public testing::Test, public
CopresenceClientDelegate {
std::map<std::string, std::vector<Message> > messages_by_subscription_;
};
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_Initialize DISABLED_Initialize
-
-TEST_F(RpcHandlerTest, MAYBE_Initialize) {
+TEST_F(RpcHandlerTest, Initialize) {
SetDeviceId("");
rpc_handler_.Initialize(RpcHandler::SuccessCallback());
RegisterDeviceRequest* registration =
@@ -192,11 +188,7 @@ TEST_F(RpcHandlerTest, MAYBE_Initialize) {
// TODO(ckehoe): Fix this on Windows. See rpc_handler.cc.
#ifndef OS_WIN
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_GetDeviceCapabilities DISABLED_GetDeviceCapabilities
-
-TEST_F(RpcHandlerTest, MAYBE_GetDeviceCapabilities) {
+TEST_F(RpcHandlerTest, GetDeviceCapabilities) {
// Empty request.
rpc_handler_.SendReportRequest(make_scoped_ptr(new ReportRequest));
EXPECT_EQ(RpcHandler::kReportRequestRpcName, rpc_name_);
@@ -243,11 +235,7 @@ TEST_F(RpcHandlerTest, MAYBE_GetDeviceCapabilities) {
}
#endif
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_CreateRequestHeader DISABLED_CreateRequestHeader
-
-TEST_F(RpcHandlerTest, MAYBE_CreateRequestHeader) {
+TEST_F(RpcHandlerTest, CreateRequestHeader) {
SetDeviceId("CreateRequestHeader Device ID");
rpc_handler_.SendReportRequest(make_scoped_ptr(new ReportRequest),
"CreateRequestHeader App ID",
@@ -261,11 +249,7 @@ TEST_F(RpcHandlerTest, MAYBE_CreateRequestHeader) {
report->header().registered_device_id());
}
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_ReportTokens DISABLED_ReportTokens
-
-TEST_F(RpcHandlerTest, MAYBE_ReportTokens) {
+TEST_F(RpcHandlerTest, ReportTokens) {
std::vector<std::string> test_tokens;
test_tokens.push_back("token 1");
test_tokens.push_back("token 2");
@@ -282,11 +266,7 @@ TEST_F(RpcHandlerTest, MAYBE_ReportTokens) {
EXPECT_EQ("token 3", tokens_sent.Get(1).token_id());
}
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_ReportResponseHandler DISABLED_ReportResponseHandler
-
-TEST_F(RpcHandlerTest, MAYBE_ReportResponseHandler) {
+TEST_F(RpcHandlerTest, ReportResponseHandler) {
// Fail on HTTP status != 200.
ReportResponse empty_response;
empty_response.mutable_header()->mutable_status()->set_code(OK);
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/453203002/120001
Message was sent while issue was closed.
Change committed as 288950 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
