Skip to content

Commit 01c1d2c

Browse files
committed
Fix a bug PauseOnExit notification.
When we notified before, we could end up entering an isolate when it was busy handling another message. By moving the notification up and clearing the "task" later, we ensure that we own the isolate for the duration of the notification. BUG= [email protected] Review URL: https://codereview.chromium.org//1324543002 .
1 parent 54fb6cd commit 01c1d2c

File tree

1 file changed

+30
-15
lines changed

1 file changed

+30
-15
lines changed

runtime/vm/message_handler.cc

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,6 @@ void MessageHandler::TaskCallback() {
238238
ASSERT(Isolate::Current() == NULL);
239239
bool ok = true;
240240
bool run_end_callback = false;
241-
bool notify_paused_on_exit = false;
242241
{
243242
MonitorLocker ml(&monitor_);
244243
// Initialize the message handler by running its start function,
@@ -255,9 +254,11 @@ void MessageHandler::TaskCallback() {
255254
NotifyPauseOnStart();
256255
monitor_.Enter();
257256
}
257+
// More messages may have come in while we released monitor_.
258258
HandleMessages(false, false);
259259
if (pause_on_start()) {
260260
// Still paused.
261+
ASSERT(oob_queue_->IsEmpty());
261262
task_ = NULL; // No task in queue.
262263
return;
263264
} else {
@@ -280,36 +281,50 @@ void MessageHandler::TaskCallback() {
280281
if (ok) {
281282
ok = HandleMessages(true, true);
282283
}
283-
task_ = NULL; // No task in queue.
284284

285285
if (!ok || !HasLivePorts()) {
286286
if (pause_on_exit()) {
287287
if (!paused_on_exit_) {
288288
if (FLAG_trace_service_pause_events) {
289289
OS::PrintErr("Isolate %s paused before exiting. "
290-
"Use the Observatory to release it.\n", name());
290+
"Use the Observatory to release it.\n", name());
291291
}
292-
notify_paused_on_exit = true;
292+
// Temporarily drop the lock when calling out to NotifyPauseOnExit.
293+
// This avoids a dead lock that can occur when this message handler
294+
// tries to post a message while a message is being posted to it.
293295
paused_on_exit_ = true;
294296
paused_timestamp_ = OS::GetCurrentTimeMillis();
297+
monitor_.Exit();
298+
NotifyPauseOnExit();
299+
monitor_.Enter();
295300
}
296-
} else {
297-
if (FLAG_trace_isolates) {
301+
// More messages may have come in while we released monitor_.
302+
HandleMessages(false, false);
303+
if (pause_on_exit()) {
304+
// Still paused.
305+
ASSERT(oob_queue_->IsEmpty());
306+
task_ = NULL; // No task in queue.
307+
return;
308+
} else {
309+
paused_on_exit_ = false;
310+
paused_timestamp_ = -1;
311+
}
312+
}
313+
if (FLAG_trace_isolates) {
298314
OS::Print("[-] Stopping message handler (%s):\n"
299315
"\thandler: %s\n",
300316
(ok ? "no live ports" : "error"),
301317
name());
302-
}
303-
pool_ = NULL;
304-
run_end_callback = true;
305-
paused_on_exit_ = false;
306-
paused_timestamp_ = -1;
307318
}
319+
pool_ = NULL;
320+
run_end_callback = true;
308321
}
309-
}
310-
// At this point we no longer hold the message handler lock.
311-
if (notify_paused_on_exit) {
312-
NotifyPauseOnExit();
322+
323+
// Clear the task_ last. We don't want any other tasks to start up
324+
// until we are done with all messages and pause notifications.
325+
ASSERT(oob_queue_->IsEmpty());
326+
ASSERT(!ok || queue_->IsEmpty());
327+
task_ = NULL;
313328
}
314329
if (run_end_callback && end_callback_ != NULL) {
315330
end_callback_(callback_data_);

0 commit comments

Comments
 (0)