Fix race in deletion of native message handlers (fixes #28484).

runtime/vm/message_handler.cc

Index: runtime/vm/message_handler.cc
diff --git a/runtime/vm/message_handler.cc b/runtime/vm/message_handler.cc
index f98ec653beb553c8a515560bad690959dbb900f8..adfda8e2fd6c95786c7787fa6c0f5dab389c31f6 100644
--- a/runtime/vm/message_handler.cc
+++ b/runtime/vm/message_handler.cc
@@ -414,7 +414,8 @@ void MessageHandler::TaskCallback() {
         }
       }
       pool_ = NULL;
-      run_end_callback = true;
+      // Decide if we have a callback before releasing the monitor.
+      run_end_callback = end_callback_ != NULL;
       delete_me = delete_me_;
     }
 
@@ -424,10 +425,13 @@ void MessageHandler::TaskCallback() {
     task_ = NULL;
   }
 
+  // The handler may have been deleted by another thread here if it is a native
+  // message handler.
+
   // Message handlers either use delete_me or end_callback but not both.
-  ASSERT(!delete_me || end_callback_ == NULL);
+  ASSERT(!delete_me || !run_end_callback);
 
-  if (run_end_callback && end_callback_ != NULL) {
+  if (run_end_callback) {
     end_callback_(callback_data_);

[siva, 2017/01/24 03:18:42]

If the handler is deleted wouldn't end_callback_ be garbage at this point? (i.e
the handler was deleted after we released the monitor above).

[rmacnak, 2017/01/24 03:28:36]

Those cases are mutually exclusive. Only a native message handler may be deleted
here, and they never have an end callback.

[Cutch, 2017/01/24 15:00:48]

DBC-

Can you add an ASSERT that checks for this mutual exclusivity? 

[siva, 2017/01/24 15:58:26]

In that case why not store end_callback_ into a local variable (end_callback)
above when holding the monitor and then do the callback using that local
ASSERT(!delete_me || end_callback == NULL);
if (end_callback != NULL) {
  ...
}

     // The handler may have been deleted after this point.
   }