Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753723003:
Fix crash in InvokeOnMemoryDump when tracing (Closed)
|
Created:
3 years, 9 months ago by hjd Modified:
3 years, 9 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix crash in InvokeOnMemoryDump when tracing
Fixes a bug in ProcessMetricsMemoryDumpProvider::RegisterForProcess
where if we get a duplicate pid we register an MDP but then immediately
destroy it. This later causes a crash. Instead we should not register
the MDP if we get a duplicate pid. This won't help the root problem
of why we are getting duplicate pids which will now just cause
information loss instead of crashing.
BUG=695731
Review-Url: https://codereview.chromium.org/2753723003
Cr-Commit-Position: refs/heads/master@{#457392}
Committed: https://chromium.googlesource.com/chromium/src/+/96f0a2b238852a56c0a30aa67f4a6943c5271688
Patch Set 1 #Patch Set 2 : be more explicit #
Total comments: 20
Patch Set 3 : fix for comments #
Total comments: 1
Patch Set 4 : fix FactoryFunction dup #Patch Set 5 : fix format #Patch Set 6 : dont run test on mac or windows #
Messages
Total messages: 26 (13 generated)
hjd@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks! :)
Excellent thanks. You can remove the "chromium:" in the BUG= label. It's the default in chrome CLs (I guess you just got too used to catapult) https://codereview.chromium.org/2753723003/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2753723003/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:490: auto mdp_iter = dump_providers_.begin(); you can use C++11 foreach these days, which is quite neat. Makes it look like a 20th century language. It should be something like: for (const auto& mdp : dump_providers) ... https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:218: std::unique_ptr<ProcessMetricsMemoryDumpProvider> (*)(base::ProcessId); this (the using) seems redudant, as you have already in the header. https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:219: FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr; this instead is necessary, yup https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:549: ProcessMetricsMemoryDumpProvider::CreateMetricsProvider( since this function is quite small and is used only once (I think) could you just do this in the RegisterForProcess function and avoiding this extra helper? Never been too much of a fun of splitting out functions that are used only once, as requires the reader to jump across code. unless the original function grows too big, but that doesn't seem to be the case here, RegisterForProcess is quite small https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:552: std::unique_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider( just return factory_for_testing(process); should be enough, and more compact right? (If for any reason doesn't compile, just do return std::move(factory_for_testing(process))) https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:558: return metrics_provider; here instead you can just do: return MakeUnique<PMDP>(process) https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:19: #include "testing/gmock/include/gmock/gmock.h" I think this is a leftover, right? you are not using gmock anymore (I think) https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:400: MOCK_METHOD2(OnMemoryDump, are you really using this mocked method? I think this is a leftover of the previous attempt right? https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:408: std::unordered_set<MockMemoryDumpProvider*> live_mocks; nit: g_ -> g_live_mocks (same with dead) as these are globals https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:430: EXPECT_EQ(1u, live_mocks.size()); I'd make this an ASSERT_EQ and remove the if(...==1) below
Thanks! I can't work out how to only write the using FactoryFunction = ... only once but other than that seems good :) https://codereview.chromium.org/2753723003/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2753723003/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:490: auto mdp_iter = dump_providers_.begin(); On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > you can use C++11 foreach these days, which is quite neat. Makes it look like a > 20th century language. > It should be something like: > > for (const auto& mdp : dump_providers) ... Done, thanks! https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:218: std::unique_ptr<ProcessMetricsMemoryDumpProvider> (*)(base::ProcessId); On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > this (the using) seems redudant, as you have already in the header. Doesn't seem visible here? ../../components/tracing/common/process_metrics_memory_dump_provider.cc:217:1: error: unknown type name 'FactoryFunction' FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr; ^ https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:549: ProcessMetricsMemoryDumpProvider::CreateMetricsProvider( On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > since this function is quite small and is used only once (I think) could you > just do this in the RegisterForProcess function and avoiding this extra helper? > Never been too much of a fun of splitting out functions that are used only once, > as requires the reader to jump across code. unless the original function grows > too big, but that doesn't seem to be the case here, RegisterForProcess is quite > small Done. https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:552: std::unique_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider( On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > just return factory_for_testing(process); should be enough, and more compact > right? > (If for any reason doesn't compile, just do return > std::move(factory_for_testing(process))) thanks :) https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:558: return metrics_provider; On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > here instead you can just do: > return MakeUnique<PMDP>(process) Hmm, I think it doesn't work because the constructor is private? In file included from ../../components/tracing/common/process_metrics_memory_dump_provider.cc:17: ../../base/memory/ptr_util.h:56:33: error: calling a protected constructor of class 'tracing::ProcessMetricsMemoryDumpProvider' return std::unique_ptr<T>(new T(std::forward<Args>(args)...)); ^ ../../components/tracing/common/process_metrics_memory_dump_provider.cc:554:28: note: in instantiation of function template specialization 'base::MakeUnique<tracing::ProcessMetricsMemoryDumpProvider, int &>' requested here owned_provider = base::MakeUnique<ProcessMetricsMemoryDumpProvider>(process); ^ ../../components/tracing/common/process_metrics_memory_dump_provider.h:41:3: note: declared protected here ProcessMetricsMemoryDumpProvider(base::ProcessId process); ^ 1 error generated. [7/25] CXX obj/content/browser/browser/browser_main_loop.o https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:19: #include "testing/gmock/include/gmock/gmock.h" On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > I think this is a leftover, right? you are not using gmock anymore (I think) Done. https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:400: MOCK_METHOD2(OnMemoryDump, On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > are you really using this mocked method? I think this is a leftover of the > previous attempt right? Done. https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:408: std::unordered_set<MockMemoryDumpProvider*> live_mocks; On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > nit: g_ -> g_live_mocks (same with dead) as these are globals Done. https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:430: EXPECT_EQ(1u, live_mocks.size()); On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > I'd make this an ASSERT_EQ and remove the if(...==1) below Done.
LGTM with the using fixed https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:218: std::unique_ptr<ProcessMetricsMemoryDumpProvider> (*)(base::ProcessId); On 2017/03/15 14:23:12, hjd wrote: > On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > > this (the using) seems redudant, as you have already in the header. > > Doesn't seem visible here? > > ../../components/tracing/common/process_metrics_memory_dump_provider.cc:217:1: > error: unknown type name 'FactoryFunction' > FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr; > ^ well it's because, 1: is private 2: you need the scope. So once you move it to the public section than you can write something like: ProcessMemoryMemoryDumpProvider::FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr; (did I say ProcessMetricsMemoryDumpProvider already? :P) https://codereview.chromium.org/2753723003/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2753723003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:9: #include <memory> nit: IWYU (include what you use) can you add #include <unordered_set> here?
Description was changed from ========== Fix crash in InvokeOnMemoryDump when tracing Fixes a bug in ProcessMetricsMemoryDumpProvider::RegisterForProcess where if we get a duplicate pid we register an MDP but then immediately destroy it. This later causes a crash. Instead we should not register the MDP if we get a duplicate pid. This won't help the root problem of why we are getting duplicate pids which will now just cause information loss instead of crashing. BUG=chromium:695731 ========== to ========== Fix crash in InvokeOnMemoryDump when tracing Fixes a bug in ProcessMetricsMemoryDumpProvider::RegisterForProcess where if we get a duplicate pid we register an MDP but then immediately destroy it. This later causes a crash. Instead we should not register the MDP if we get a duplicate pid. This won't help the root problem of why we are getting duplicate pids which will now just cause information loss instead of crashing. BUG=695731 ==========
On 2017/03/15 15:18:15, Primiano Tucci (slow - perf) wrote: > LGTM with the using fixed > > https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... > File components/tracing/common/process_metrics_memory_dump_provider.cc (right): > > https://codereview.chromium.org/2753723003/diff/20001/components/tracing/comm... > components/tracing/common/process_metrics_memory_dump_provider.cc:218: > std::unique_ptr<ProcessMetricsMemoryDumpProvider> (*)(base::ProcessId); > On 2017/03/15 14:23:12, hjd wrote: > > On 2017/03/15 12:44:40, Primiano Tucci (slow - perf) wrote: > > > this (the using) seems redudant, as you have already in the header. > > > > Doesn't seem visible here? > > > > ../../components/tracing/common/process_metrics_memory_dump_provider.cc:217:1: > > error: unknown type name 'FactoryFunction' > > FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = > nullptr; > > ^ > > well it's because, > 1: is private > 2: you need the scope. So once you move it to the public section than you can > write something like: > > ProcessMemoryMemoryDumpProvider::FactoryFunction > ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr; > > (did I say ProcessMetricsMemoryDumpProvider already? :P) Doh! I forgot you have to explicitly write the scope for everything in C++ Thanks! > https://codereview.chromium.org/2753723003/diff/40001/components/tracing/comm... > File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc > (right): > > https://codereview.chromium.org/2753723003/diff/40001/components/tracing/comm... > components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:9: > #include <memory> > nit: IWYU (include what you use) > can you add #include <unordered_set> here? Done
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2753723003/#ps70001 (title: "fix format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hjd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2753723003/#ps90001 (title: "dont run test on mac or windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1489655000971290,
"parent_rev": "17fa68c396c45e6650df5dd2777a0112800ae61d", "commit_rev":
"96f0a2b238852a56c0a30aa67f4a6943c5271688"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash in InvokeOnMemoryDump when tracing Fixes a bug in ProcessMetricsMemoryDumpProvider::RegisterForProcess where if we get a duplicate pid we register an MDP but then immediately destroy it. This later causes a crash. Instead we should not register the MDP if we get a duplicate pid. This won't help the root problem of why we are getting duplicate pids which will now just cause information loss instead of crashing. BUG=695731 ========== to ========== Fix crash in InvokeOnMemoryDump when tracing Fixes a bug in ProcessMetricsMemoryDumpProvider::RegisterForProcess where if we get a duplicate pid we register an MDP but then immediately destroy it. This later causes a crash. Instead we should not register the MDP if we get a duplicate pid. This won't help the root problem of why we are getting duplicate pids which will now just cause information loss instead of crashing. BUG=695731 Review-Url: https://codereview.chromium.org/2753723003 Cr-Commit-Position: refs/heads/master@{#457392} Committed: https://chromium.googlesource.com/chromium/src/+/96f0a2b238852a56c0a30aa67f4a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/96f0a2b238852a56c0a30aa67f4a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
