Skip to content

Commit

Permalink
Fix data race in DartIsolateGroupData. (#15949)
Browse files Browse the repository at this point in the history
This class is meant to be thread safe. In fact, its headerdoc statement on
thread safety even mentions this. All fields on the class are readonly except
the child isolate preparer. This field is set during VM instantiated isolate
initialization. The VM may launch multiple isolate in the same isolate group on
at the same time (each on a VM backed thread pool thread). Attempting to set the
field without synchronization is a data race.

Fixes #49358
Fixes b/147798920
  • Loading branch information
chinmaygarde committed Jan 24, 2020
1 parent e08ff74 commit f10f03a
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 6 deletions.
2 changes: 1 addition & 1 deletion runtime/dart_isolate.cc
Expand Up @@ -814,7 +814,7 @@ bool DartIsolate::InitializeIsolate(
// also a root isolate) by the utility routines in the VM. However, secondary
// isolates will be run by the VM if they are marked as runnable.
if (!embedder_isolate->IsRootIsolate()) {
const ChildIsolatePreparer& child_isolate_preparer =
auto child_isolate_preparer =
embedder_isolate->GetIsolateGroupData().GetChildIsolatePreparer();
FML_DCHECK(child_isolate_preparer);
if (!child_isolate_preparer(embedder_isolate.get())) {
Expand Down
5 changes: 3 additions & 2 deletions runtime/dart_isolate_group_data.cc
Expand Up @@ -44,8 +44,8 @@ const std::string& DartIsolateGroupData::GetAdvisoryScriptEntrypoint() const {
return advisory_script_entrypoint_;
}

const ChildIsolatePreparer& DartIsolateGroupData::GetChildIsolatePreparer()
const {
ChildIsolatePreparer DartIsolateGroupData::GetChildIsolatePreparer() const {
std::scoped_lock lock(child_isolate_preparer_mutex_);
return child_isolate_preparer_;
}

Expand All @@ -59,6 +59,7 @@ const fml::closure& DartIsolateGroupData::GetIsolateShutdownCallback() const {

void DartIsolateGroupData::SetChildIsolatePreparer(
const ChildIsolatePreparer& value) {
std::scoped_lock lock(child_isolate_preparer_mutex_);
child_isolate_preparer_ = value;
}

Expand Down
14 changes: 11 additions & 3 deletions runtime/dart_isolate_group_data.h
Expand Up @@ -5,6 +5,7 @@
#ifndef FLUTTER_RUNTIME_DART_ISOLATE_GROUP_DATA_H_
#define FLUTTER_RUNTIME_DART_ISOLATE_GROUP_DATA_H_

#include <mutex>
#include <string>

#include "flutter/common/settings.h"
Expand Down Expand Up @@ -37,25 +38,32 @@ class DartIsolateGroupData {
~DartIsolateGroupData();

const Settings& GetSettings() const;

fml::RefPtr<const DartSnapshot> GetIsolateSnapshot() const;

const std::string& GetAdvisoryScriptURI() const;

const std::string& GetAdvisoryScriptEntrypoint() const;
const ChildIsolatePreparer& GetChildIsolatePreparer() const;

ChildIsolatePreparer GetChildIsolatePreparer() const;

const fml::closure& GetIsolateCreateCallback() const;

const fml::closure& GetIsolateShutdownCallback() const;

void SetChildIsolatePreparer(const ChildIsolatePreparer& value);

private:
FML_DISALLOW_COPY_AND_ASSIGN(DartIsolateGroupData);

const Settings settings_;
const fml::RefPtr<const DartSnapshot> isolate_snapshot_;
const std::string advisory_script_uri_;
const std::string advisory_script_entrypoint_;
mutable std::mutex child_isolate_preparer_mutex_;
ChildIsolatePreparer child_isolate_preparer_;
const fml::closure isolate_create_callback_;
const fml::closure isolate_shutdown_callback_;

FML_DISALLOW_COPY_AND_ASSIGN(DartIsolateGroupData);
};

} // namespace flutter
Expand Down

0 comments on commit f10f03a

Please sign in to comment.