Resolution for issue #28746: changed order in which locks/flags are grabbed in MallocHooks to preve…

runtime/vm/malloc_hooks.cc

Index: runtime/vm/malloc_hooks.cc
diff --git a/runtime/vm/malloc_hooks.cc b/runtime/vm/malloc_hooks.cc
index 9e5c6587c3065498c8a757de121770e315e1b03d..b1cfef046d7ae73c4573efd637faf0e2293b9078 100644
--- a/runtime/vm/malloc_hooks.cc
+++ b/runtime/vm/malloc_hooks.cc
@@ -173,8 +173,10 @@ class MallocHooksState : public AllStatic {
 };
 
 
-// MallocHooks state / locks.
+// MallocHookScope state.
 ThreadLocalKey MallocHookScope::in_malloc_hook_flag_ = kUnsetThreadLocalKey;
+
+// MallocHooks state / locks.
 bool MallocHooksState::active_ = false;
 intptr_t MallocHooksState::original_pid_ = MallocHooksState::kInvalidPid;
 Mutex* MallocHooksState::malloc_hook_mutex_ = new Mutex();
@@ -270,11 +272,11 @@ void MallocHooksState::RecordAllocHook(const void* ptr, size_t size) {
     return;
   }
 
-  // Set the malloc hook flag before grabbing the mutex to avoid calling hooks
-  // again.
-  MallocHookScope mhs;
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());

[zra, 2017/02/17 18:41:02]

I think taking the log and checking Active() needs to come even before
MallocHookScope::IsInHook() etc above, otherwise we could hit the assert in
MallocHookScope::IsInHook().

[bkonyi, 2017/02/17 21:15:11]

We can't take the lock before anything else, or else we'll end up in a deadlock.
See comments in newest patch set for my proposed solution.

   if ((ptr != NULL) && MallocHooksState::Active()) {
+    // Set the malloc hook flag before to avoid calling hooks again if memory is
+    // allocated/freed below.
+    MallocHookScope mhs;
     MallocHooksState::IncrementHeapAllocatedMemoryInBytes(size);
     MallocHooksState::address_map()->Insert(ptr, size);
   }
@@ -286,11 +288,11 @@ void MallocHooksState::RecordFreeHook(const void* ptr) {
     return;
   }
 
-  // Set the malloc hook flag before grabbing the mutex to avoid calling hooks
-  // again.
-  MallocHookScope mhs;
   MutexLocker ml(MallocHooksState::malloc_hook_mutex());

[zra, 2017/02/17 18:41:02]

ditto

[bkonyi, 2017/02/17 21:15:11]

See above.

   if ((ptr != NULL) && MallocHooksState::Active()) {
+    // Set the malloc hook flag before to avoid calling hooks again if memory is
+    // allocated/freed below.
+    MallocHookScope mhs;
     intptr_t size = 0;
     if (MallocHooksState::address_map()->Lookup(ptr, &size)) {
       MallocHooksState::DecrementHeapAllocatedMemoryInBytes(size);