Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884573002:
Fix race condition when dump providers invoked within destroyed state (Closed)
DescriptionFix race condition when dump providers invoked within destroyed state
InvokeOnMemoryDump() should never be called when tracing is disabled
because dump providers can get unexpected destroyed session state.
This case also fortified with test case.
BUG=600570
Committed: https://crrev.com/063a22a7ad94d98b2d29bf3745dedea10313889f
Cr-Commit-Position: refs/heads/master@{#387281}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Messages
Total messages: 15 (4 generated)
kraynov@chromium.org changed reviewers: + primiano@chromium.org
Hi!
Thanks. Some comments, but I think you spot the right issue :) https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:417: if (!pmd_async_state->dump_thread_task_runner.get()) { Ok looking a bit more to this CL it seems that the cleanest way to fix all of this is to do this check up in ::CreateProcess dump, and early out there. SOrry for the back an forth, I know I suggested this, but by looking at the code I don't see any reason why we should postpone this decision here. Nothing should change between the dumps, as all the state we care about is propagated by the AsyncState. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:699: session_state_ = nullptr; why are you moving this outside of the lock? we should not rely on the precise point where session_state_ gets cleared. It should always be accessed under the lock. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:117: MOCK_METHOD2(OnMemoryDump, Nothing in this test is checking that the session_state is never null, which is the thing you are fixing in this bug. Can you add a default mock implementation to this OnMemoryDump which does the assert. See the WillByDefault() above for the pattern to use. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:941: auto thread = WrapUnique(new Thread("test thread")); I think we don't allow yet auto with WrapUnique. Also, there should be no need of WrapUnique here, just: std::unique_ptr<Thread> thread(new Thread(...)) right? https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:945: RegisterDumpProvider(&mdp, thread->task_runner(), kDefaultOptions); hmm here you are overriding the previous behavior. I would much rather add two mdp, one not bound to any thread and one bound to |thread| here.
Okay, will address the comments https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:417: if (!pmd_async_state->dump_thread_task_runner.get()) { On 2016/04/13 17:08:41, Primiano Tucci wrote: > Ok looking a bit more to this CL it seems that the cleanest way to fix all of > this is to do this check up in ::CreateProcess dump, and early out there. > SOrry for the back an forth, I know I suggested this, but by looking at the code > I don't see any reason why we should postpone this decision here. > Nothing should change between the dumps, as all the state we care about is > propagated by the AsyncState. Don't know the reason but it seems to be intentionally designed this way because there is a test expecting that CreateProcessDump will do some work. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:699: session_state_ = nullptr; On 2016/04/13 17:08:41, Primiano Tucci wrote: > why are you moving this outside of the lock? > we should not rely on the precise point where session_state_ gets cleared. It > should always be accessed under the lock. Acknowledged. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:117: MOCK_METHOD2(OnMemoryDump, On 2016/04/13 17:08:41, Primiano Tucci wrote: > Nothing in this test is checking that the session_state is never null, which is > the thing you are fixing in this bug. > Can you add a default mock implementation to this OnMemoryDump which does the > assert. > See the WillByDefault() above for the pattern to use. Acknowledged. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:941: auto thread = WrapUnique(new Thread("test thread")); On 2016/04/13 17:08:41, Primiano Tucci wrote: > I think we don't allow yet auto with WrapUnique. > Also, there should be no need of WrapUnique here, just: > std::unique_ptr<Thread> thread(new Thread(...)) right? Acknowledged. https://codereview.chromium.org/1884573002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:945: RegisterDumpProvider(&mdp, thread->task_runner(), kDefaultOptions); On 2016/04/13 17:08:41, Primiano Tucci wrote: > hmm here you are overriding the previous behavior. > I would much rather add two mdp, one not bound to any thread and one bound to > |thread| here. Acknowledged.
Take a look
LGTM with comments addressed. P.S. I just realized that we want to remove the session_state() getter from the MemoryDumpManager as that should never be used to avoid races (instead the one from the PMD should be used). Can you do that in a separate CL? https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:411: // It means that tracing was disabled a moment ago. s/a moment ago/right before starting this dump/ https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:413: // event, add it to the trace and finalize process dump (invoke callback). s/(invoke callback)/, invoking the callback/ https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:124: EXPECT_TRUE(pmd->session_state().get() != nullptr); Plz add a comment here explaining that under no circumstance the session_state should be null if we do a dump. https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:948: std::unique_ptr<Thread> thread = WrapUnique(new Thread("test thread")); as commented before, this WrapUnique is unnecessary. std::unique_ptr<Thread> thread(new Thread(...)) https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:948: std::unique_ptr<Thread> thread = WrapUnique(new Thread("test thread")); s/thread/mdp_thread/ for consistency with the code above
bashi@chromium.org changed reviewers: + bashi@chromium.org
Thanks for the fix! https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1884573002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:431: task_runner = pmd_async_state->dump_thread_task_runner.get(); DCHECK(task_runner) ?
Updated
wonderfulz, LGTM. Ship it!
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1884573002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1884573002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix race condition when dump providers invoked within destroyed state InvokeOnMemoryDump() should never be called when tracing is disabled because dump providers can get unexpected destroyed session state. This case also fortified with test case. BUG=600570 ========== to ========== Fix race condition when dump providers invoked within destroyed state InvokeOnMemoryDump() should never be called when tracing is disabled because dump providers can get unexpected destroyed session state. This case also fortified with test case. BUG=600570 Committed: https://crrev.com/063a22a7ad94d98b2d29bf3745dedea10313889f Cr-Commit-Position: refs/heads/master@{#387281} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/063a22a7ad94d98b2d29bf3745dedea10313889f Cr-Commit-Position: refs/heads/master@{#387281} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
