Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 70 additions & 2 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// found in the LICENSE file.

#define RAPIDJSON_HAS_STDSTRING 1

#include "flutter/shell/common/shell.h"

#include <memory>
Expand Down Expand Up @@ -372,6 +371,71 @@ void Shell::NotifyLowMemoryWarning() const {
// to purge them.
}

void Shell::RunEngine(RunConfiguration run_configuration) {
RunEngine(std::move(run_configuration), nullptr);
}

void Shell::RunEngine(RunConfiguration run_configuration,
std::function<void(Engine::RunStatus)> result_callback) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have the result callback be executed on the calling task runner (the platform task runner in this case). Since doing the re-threading at the each possible call-site is tedious, you can wrap the same in and closure that does the re-threading and then pass that around as normal. See the image decoder for a use of this pattern.

On a related note, since forgetting to invoke the closure in the error case is possible, maybe we should have an RAII wrapper for the same. I have a prototype for this in a different patch that is not in yet.

That way, you can wrap the callback in a closure that does the re-threading and put it in that RAII wrapper. You could then invoke the same only in the success case and ensure that all erroneous cases invoke the callback and on the right thread.

Something like the following should work for now:

auto result = [runner = task_runner_.GetPlatformTaskRunner(), result_callback]
    (Engine::RunStatus status){
  if (!result_callback) {
    return;
  }
  runner->PostTask([result_callback, status](){ result_callback(status); });
};

// Pass result around and invoke it directly (no null check is necessary as it is performed in the wrapper).
// In the future, result can be stuffed inside an AutoFireClosure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

if (!weak_engine_) {
result_callback(Engine::RunStatus::Failure);
}
fml::TaskRunner::RunNowOrPostTask(
task_runners_.GetUITaskRunner(),
fml::MakeCopyable([run_configuration = std::move(run_configuration),
weak_engine = weak_engine_,
result_callback =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::move on closures is not necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

std::move(result_callback)]() mutable {
if (!weak_engine) {
if (result_callback) {
FML_LOG(ERROR)
<< "Could not launch engine with configuration - no engine.";
result_callback(Engine::RunStatus::Failure);
}
return;
}
auto result = weak_engine->Run(std::move(run_configuration));
if (result == flutter::Engine::RunStatus::Failure) {
FML_LOG(ERROR) << "Could not launch engine with configuration.";
}
if (result_callback) {
result_callback(result);
}
}));
}

std::optional<int> Shell::GetUIIsolateLastError() const {
FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

if (!weak_engine_) {
return std::nullopt;
}
switch (weak_engine_->GetUIIsolateLastError()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a weak pointer on the non-originating thread is not thread safe. See the comment in weak_ptr.h that describes safe use. I have pasted the relevant bits below.

Now in this particular case, the engine provides a guarantee that shell subcomponents will be destroyed before its destructor finishes execution on the platform task runner. So, it happens to work out. But I would still be more comfortable if code didn't break safe threading rules because of the lifecycle assumptions. It is not immediately clear from reading just this bit of code about why this is safe even though it breaks rules of safe weak pointer use.

You can use the same pattern used right above in the RunEngine call.

// Weak pointers are not in general thread-safe. They may only be *used* on a
// single thread, namely the same thread as the "originating" |WeakPtrFactory|
// (which can invalidate the weak pointers that it generates).
//
// However, weak pointers may be passed to other threads, reset on other
// threads, or destroyed on other threads. They may also be reassigned on other
// threads (in which case they should then only be used on the thread
// corresponding to the new "originating" |WeakPtrFactory|).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going with the unique_ptr and a comment, as discussed offline

case tonic::kCompilationErrorType:
return kCompilationErrorExitCode;
case tonic::kApiErrorType:
return kApiErrorExitCode;
case tonic::kUnknownErrorType:
return kErrorExitCode;
default:
return 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is actually EXIT_SUCCESS. So we will end up saying an unknown failure is no failure. This seems dodgy. Lets use kErrorExitCode as the default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression the engine method returned 0 if ther ewas no error. Is that not the case?

The tester_main was defaulting this case to 0 as well AFAICT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be tonic::kNoError. You can return 0 in that case instead of just defaulting to success.

}
}

bool Shell::EngineHasLivePorts() const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on the comment in Shell::GetUIIsolateLastError

FML_DCHECK(is_setup_);
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());

if (!weak_engine_) {
return false;
}
return weak_engine_->UIIsolateHasLivePorts();
}

bool Shell::IsSetup() const {
return is_setup_;
}
Expand Down Expand Up @@ -425,10 +489,14 @@ fml::WeakPtr<Rasterizer> Shell::GetRasterizer() {
return weak_rasterizer_;
}

// TODO(dnfield): Remove this when either Topaz is up to date or flutter_runner
// is built out of this repo.
#ifdef OS_FUCHSIA
fml::WeakPtr<Engine> Shell::GetEngine() {
FML_DCHECK(is_setup_);
return weak_engine_;
}
#endif // OS_FUCHSIA

fml::WeakPtr<PlatformView> Shell::GetPlatformView() {
FML_DCHECK(is_setup_);
Expand Down Expand Up @@ -907,7 +975,7 @@ void Shell::ReportTimings() {

auto timings = std::move(unreported_timings_);
unreported_timings_ = {};
task_runners_.GetUITaskRunner()->PostTask([timings, engine = GetEngine()] {
task_runners_.GetUITaskRunner()->PostTask([timings, engine = weak_engine_] {
if (engine) {
engine->ReportTimings(std::move(timings));
}
Expand Down
47 changes: 46 additions & 1 deletion shell/common/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ class Shell final : public PlatformView::Delegate,
public Rasterizer::Delegate,
public ServiceProtocol::Handler {
public:
/// The Dart error code for an API error.
const int kApiErrorExitCode = 253;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these enums instead now?

/// The Dart error code for a compilation error.
const int kCompilationErrorExitCode = 254;
/// The Dart error code for an unkonwn error.
const int kErrorExitCode = 255;

template <class T>
using CreateCallback = std::function<std::unique_ptr<T>(Shell&)>;

Expand Down Expand Up @@ -165,6 +172,19 @@ class Shell final : public PlatformView::Delegate,
///
~Shell();

//----------------------------------------------------------------------------
/// @brief Starts an isolate for the given RunConfiguration.
///
void RunEngine(RunConfiguration run_configuration);

//----------------------------------------------------------------------------
/// @brief Starts an isolate for the given RunConfiguration. The
/// result_callback will be called with the status of the
/// operation.
///
void RunEngine(RunConfiguration run_configuration,
std::function<void(Engine::RunStatus)> result_callback);

//------------------------------------------------------------------------------
/// @return The settings used to launch this shell.
///
Expand All @@ -191,12 +211,18 @@ class Shell final : public PlatformView::Delegate,
///
fml::WeakPtr<Rasterizer> GetRasterizer();

// TODO(dnfield): Remove this when either Topaz is up to date or flutter_runner
// is built out of this repo.
#ifdef OS_FUCHSIA
//------------------------------------------------------------------------------
/// @brief Engines may only be accessed on the UI thread.
/// @brief Engines may only be accessed on the UI thread. This method is
/// deprecated, and implementers should instead use other API
/// available on the Shell or the PlatformView.
///
/// @return A weak pointer to the engine.
///
fml::WeakPtr<Engine> GetEngine();
#endif // OS_FUCHSIA

//----------------------------------------------------------------------------
/// @brief Platform views may only be accessed on the platform task
Expand Down Expand Up @@ -253,6 +279,25 @@ class Shell final : public PlatformView::Delegate,
///
fml::Status WaitForFirstFrame(fml::TimeDelta timeout);

//----------------------------------------------------------------------------
/// @brief Used by embedders to get the last error from the Dart UI
/// Isolate, if one exists.
///
/// @return Returns the last error code from the UI Isolate.
///
std::optional<int> GetUIIsolateLastError() const;

//----------------------------------------------------------------------------
/// @brief Used by embedders to check if the Engine is running and has
/// any live ports remaining. For example, the Flutter tester uses
/// this method to check whether it should continue to wait for
/// a running test or not.
///
/// @return Returns if the shell has an engine and the engine has any live
/// Dart ports.
///
bool EngineHasLivePorts() const;

private:
using ServiceProtocolHandler =
std::function<bool(const ServiceProtocol::Handler::ServiceProtocolMap&,
Expand Down
19 changes: 10 additions & 9 deletions shell/common/shell_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,14 @@ void ShellTest::PlatformViewNotifyCreated(Shell* shell) {
void ShellTest::RunEngine(Shell* shell, RunConfiguration configuration) {
fml::AutoResetWaitableEvent latch;
fml::TaskRunner::RunNowOrPostTask(
shell->GetTaskRunners().GetUITaskRunner(),
fml::MakeCopyable([&latch, config = std::move(configuration),
engine = shell->GetEngine()]() mutable {
ASSERT_TRUE(engine);
ASSERT_EQ(engine->Run(std::move(config)), Engine::RunStatus::Success);
latch.Signal();
}));
shell->GetTaskRunners().GetPlatformTaskRunner(),
[shell, &latch, &configuration]() {
shell->RunEngine(std::move(configuration),
[&latch](Engine::RunStatus run_status) {
ASSERT_EQ(run_status, Engine::RunStatus::Success);
latch.Signal();
});
});
latch.Wait();
}

Expand All @@ -91,7 +92,7 @@ void ShellTest::PumpOneFrame(Shell* shell) {
// won't be rasterized.
fml::AutoResetWaitableEvent latch;
shell->GetTaskRunners().GetUITaskRunner()->PostTask(
[&latch, engine = shell->GetEngine()]() {
[&latch, engine = shell->weak_engine_]() {
engine->SetViewportMetrics({1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0});
engine->animator_->BeginFrame(fml::TimePoint::Now(),
fml::TimePoint::Now());
Expand All @@ -101,7 +102,7 @@ void ShellTest::PumpOneFrame(Shell* shell) {

latch.Reset();
// Call |Render| to rasterize a layer tree and trigger |OnFrameRasterized|
fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->GetEngine();
fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->weak_engine_;
shell->GetTaskRunners().GetUITaskRunner()->PostTask(
[&latch, runtime_delegate]() {
auto layer_tree = std::make_unique<LayerTree>();
Expand Down
48 changes: 1 addition & 47 deletions shell/platform/android/android_shell_holder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -157,53 +157,7 @@ void AndroidShellHolder::Launch(RunConfiguration config) {
return;
}

shell_->GetTaskRunners().GetUITaskRunner()->PostTask(
fml::MakeCopyable([engine = shell_->GetEngine(), //
config = std::move(config) //
]() mutable {
FML_LOG(INFO) << "Attempting to launch engine configuration...";
if (!engine ||
engine->Run(std::move(config)) == Engine::RunStatus::Failure) {
FML_LOG(ERROR) << "Could not launch engine in configuration.";
} else {
FML_LOG(INFO) << "Isolate for engine configuration successfully "
"started and run.";
}
}));
}

void AndroidShellHolder::SetViewportMetrics(
const flutter::ViewportMetrics& metrics) {
if (!IsValid()) {
return;
}

shell_->GetTaskRunners().GetUITaskRunner()->PostTask(
[engine = shell_->GetEngine(), metrics]() {
if (engine) {
engine->SetViewportMetrics(metrics);
}
});
}

void AndroidShellHolder::DispatchPointerDataPacket(
std::unique_ptr<flutter::PointerDataPacket> packet) {
if (!IsValid()) {
return;
}

TRACE_EVENT0("flutter", "AndroidShellHolder::DispatchPointerDataPacket");
TRACE_FLOW_BEGIN("flutter", "PointerEvent", next_pointer_flow_id_);

shell_->GetTaskRunners().GetUITaskRunner()->PostTask(fml::MakeCopyable(
[engine = shell_->GetEngine(), packet = std::move(packet),
flow_id = next_pointer_flow_id_] {
if (engine) {
engine->DispatchPointerDataPacket(*packet, flow_id);
}
}));

next_pointer_flow_id_++;
shell_->RunEngine(std::move(config));
}

Rasterizer::Screenshot AndroidShellHolder::Screenshot(
Expand Down
5 changes: 0 additions & 5 deletions shell/platform/android/android_shell_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,6 @@ class AndroidShellHolder {

void Launch(RunConfiguration configuration);

void SetViewportMetrics(const flutter::ViewportMetrics& metrics);

void DispatchPointerDataPacket(
std::unique_ptr<flutter::PointerDataPacket> packet);

const flutter::Settings& GetSettings() const;

fml::WeakPtr<PlatformViewAndroid> GetPlatformView();
Expand Down
5 changes: 3 additions & 2 deletions shell/platform/android/platform_view_android_jni.cc
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ static void SetViewportMetrics(JNIEnv* env,
static_cast<double>(physicalViewInsetLeft),
};

ANDROID_SHELL_HOLDER->SetViewportMetrics(metrics);
ANDROID_SHELL_HOLDER->GetPlatformView()->SetViewportMetrics(metrics);
}

static jobject GetBitmap(JNIEnv* env, jobject jcaller, jlong shell_holder) {
Expand Down Expand Up @@ -387,7 +387,8 @@ static void DispatchPointerDataPacket(JNIEnv* env,
jint position) {
uint8_t* data = static_cast<uint8_t*>(env->GetDirectBufferAddress(buffer));
auto packet = std::make_unique<flutter::PointerDataPacket>(data, position);
ANDROID_SHELL_HOLDER->DispatchPointerDataPacket(std::move(packet));
ANDROID_SHELL_HOLDER->GetPlatformView()->DispatchPointerDataPacket(
std::move(packet));
}

static void DispatchSemanticsAction(JNIEnv* env,
Expand Down
39 changes: 10 additions & 29 deletions shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ @implementation FlutterEngine {

int64_t _nextTextureId;

uint64_t _nextPointerFlowId;

BOOL _allowHeadlessExecution;
FlutterBinaryMessengerRelay* _binaryMessenger;
}
Expand Down Expand Up @@ -126,24 +124,17 @@ - (void)dealloc {
}

- (void)updateViewportMetrics:(flutter::ViewportMetrics)viewportMetrics {
self.shell.GetTaskRunners().GetUITaskRunner()->PostTask(
[engine = self.shell.GetEngine(), metrics = viewportMetrics]() {
if (engine) {
engine->SetViewportMetrics(std::move(metrics));
}
});
if (!self.platformView) {
return;
}
self.platformView->SetViewportMetrics(std::move(viewportMetrics));
}

- (void)dispatchPointerDataPacket:(std::unique_ptr<flutter::PointerDataPacket>)packet {
TRACE_EVENT0("flutter", "dispatchPointerDataPacket");
TRACE_FLOW_BEGIN("flutter", "PointerEvent", _nextPointerFlowId);
self.shell.GetTaskRunners().GetUITaskRunner()->PostTask(fml::MakeCopyable(
[engine = self.shell.GetEngine(), packet = std::move(packet), flow_id = _nextPointerFlowId] {
if (engine) {
engine->DispatchPointerDataPacket(*packet, flow_id);
}
}));
_nextPointerFlowId++;
if (!self.platformView) {
return;
}
self.platformView->DispatchPointerDataPacket(std::move(packet));
}

- (fml::WeakPtr<flutter::PlatformView>)platformView {
Expand Down Expand Up @@ -312,18 +303,8 @@ - (void)maybeSetupPlatformViewChannels {

- (void)launchEngine:(NSString*)entrypoint libraryURI:(NSString*)libraryOrNil {
// Launch the Dart application with the inferred run configuration.
self.shell.GetTaskRunners().GetUITaskRunner()->PostTask(fml::MakeCopyable(
[engine = _shell->GetEngine(),
config = [_dartProject.get() runConfigurationForEntrypoint:entrypoint
libraryOrNil:libraryOrNil] //
]() mutable {
if (engine) {
auto result = engine->Run(std::move(config));
if (result == flutter::Engine::RunStatus::Failure) {
FML_LOG(ERROR) << "Could not launch engine with configuration.";
}
}
}));
self.shell.RunEngine([_dartProject.get() runConfigurationForEntrypoint:entrypoint
libraryOrNil:libraryOrNil]);
}

- (BOOL)createShell:(NSString*)entrypoint libraryURI:(NSString*)libraryURI {
Expand Down
Loading