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 5 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
129 changes: 124 additions & 5 deletions lib/ui/dart_runtime_hooks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,27 @@ namespace blink {
#define BUILTIN_NATIVE_LIST(V) \
V(Logger_PrintString, 1) \
V(SaveCompilationTrace, 0) \
V(ScheduleMicrotask, 1)
V(ScheduleMicrotask, 1) \
V(LookupClosure, 3) \
V(GetFunctionClassName, 1) \
V(GetFunctionLibraryUrl, 1) \
V(GetFunctionName, 1)

BUILTIN_NATIVE_LIST(DECLARE_FUNCTION);

void DartRuntimeHooks::RegisterNatives(tonic::DartLibraryNatives* natives) {
natives->Register({BUILTIN_NATIVE_LIST(REGISTER_FUNCTION)});
}

static Dart_Handle GetClosure(Dart_Handle builtin_library, const char* name) {
static Dart_Handle GetFunction(Dart_Handle builtin_library, const char* name) {
Dart_Handle getter_name = ToDart(name);
Dart_Handle closure = Dart_Invoke(builtin_library, getter_name, 0, nullptr);
DART_CHECK_VALID(closure);
return closure;
}

static void InitDartInternal(Dart_Handle builtin_library, bool is_ui_isolate) {
Dart_Handle print = GetClosure(builtin_library, "_getPrintClosure");
Dart_Handle print = GetFunction(builtin_library, "_getPrintClosure");

Dart_Handle internal_library = Dart_LookupLibrary(ToDart("dart:_internal"));

Expand Down Expand Up @@ -101,7 +105,7 @@ static void InitDartAsync(Dart_Handle builtin_library, bool is_ui_isolate) {
Dart_Handle schedule_microtask;
if (is_ui_isolate) {
schedule_microtask =
GetClosure(builtin_library, "_getScheduleMicrotaskClosure");
GetFunction(builtin_library, "_getScheduleMicrotaskClosure");
} else {
Dart_Handle isolate_lib = Dart_LookupLibrary(ToDart("dart:isolate"));
Dart_Handle method_name =
Expand All @@ -125,7 +129,8 @@ static void InitDartIO(Dart_Handle builtin_library,
DART_CHECK_VALID(Dart_SetField(platform_type, ToDart("_nativeScript"),
ToDart(script_uri)));
}
Dart_Handle locale_closure = GetClosure(builtin_library, "_getLocaleClosure");
Dart_Handle locale_closure =
GetFunction(builtin_library, "_getLocaleClosure");
DART_CHECK_VALID(
Dart_SetField(platform_type, ToDart("_localeClosure"), locale_closure));
}
Expand Down Expand Up @@ -223,4 +228,118 @@ void ScheduleMicrotask(Dart_NativeArguments args) {
UIDartState::Current()->ScheduleMicrotask(closure);
}

void LookupClosure(Dart_NativeArguments args) {

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.

This code will probably propagate error handles correctly, but I like to check Dart_Handle return values with DART_CHECK_VALID() unless there's some reason not to.

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.

Dart_Handle closure_name = Dart_GetNativeArgument(args, 0);
Dart_Handle library_name = Dart_GetNativeArgument(args, 1);
Dart_Handle class_name = Dart_GetNativeArgument(args, 2);
DART_CHECK_VALID(closure_name);
DART_CHECK_VALID(library_name);
DART_CHECK_VALID(class_name);

Dart_Handle library;
if (library_name == Dart_Null()) {
library = Dart_RootLibrary();
} else {
library = Dart_LookupLibrary(library_name);
}
DART_CHECK_VALID(library);

Dart_Handle closure;
if (Dart_IsNull(class_name)) {
closure = Dart_GetClosure(library, closure_name);
} else {
Dart_Handle cls = Dart_GetClass(library, class_name);
DART_CHECK_VALID(cls);
if (Dart_IsNull(cls)) {
closure = Dart_Null();
} else {
closure = Dart_GetStaticMethodClosure(library, cls, closure_name);
}
}
DART_CHECK_VALID(closure);

Dart_SetReturnValue(args, closure);
}

void GetFunctionLibraryUrl(Dart_NativeArguments args) {
Dart_Handle closure = Dart_GetNativeArgument(args, 0);
if (Dart_IsClosure(closure)) {

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.

If the incoming arg closure is not a closure, don't you need a check that it is a Function Object? further down you are calling Dart_FunctionOwner on it and this method will return an error if it is not a function object.

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.

Yes, good catch. I'll add a check.

closure = Dart_ClosureFunction(closure);
DART_CHECK_VALID(closure);
}

if (!Dart_IsFunction(closure)) {
Dart_SetReturnValue(args, Dart_Null());
return;
}

Dart_Handle url = Dart_Null();
Dart_Handle owner = Dart_FunctionOwner(closure);
if (Dart_IsInstance(owner)) {
owner = Dart_ClassLibrary(owner);
}
if (Dart_IsLibrary(owner)) {
url = Dart_LibraryUrl(owner);
DART_CHECK_VALID(url);
}
Dart_SetReturnValue(args, url);
}

void GetFunctionClassName(Dart_NativeArguments args) {
Dart_Handle closure = Dart_GetNativeArgument(args, 0);
Dart_Handle result;

if (Dart_IsClosure(closure)) {
closure = Dart_ClosureFunction(closure);
DART_CHECK_VALID(closure);
}

if (!Dart_IsFunction(closure)) {
Dart_SetReturnValue(args, Dart_Null());
return;
}

bool is_static = false;
result = Dart_FunctionIsStatic(closure, &is_static);
DART_CHECK_VALID(result);
if (!is_static) {
Dart_SetReturnValue(args, Dart_Null());
return;
}

result = Dart_FunctionOwner(closure);
DART_CHECK_VALID(result);

if (Dart_IsLibrary(result) || !Dart_IsInstance(result)) {
Dart_SetReturnValue(args, Dart_Null());
return;
}

result = Dart_ClassName(result);
DART_CHECK_VALID(result);
Dart_SetReturnValue(args, result);
}

void GetFunctionName(Dart_NativeArguments args) {
Dart_Handle func = Dart_GetNativeArgument(args, 0);
DART_CHECK_VALID(func);

if (Dart_IsClosure(func)) {
func = Dart_ClosureFunction(func);
DART_CHECK_VALID(func);
}

if (!Dart_IsFunction(func)) {
Dart_SetReturnValue(args, Dart_Null());
return;
}

Dart_Handle result = Dart_FunctionName(func);
if (Dart_IsError(result)) {
Dart_PropagateError(result);
return;
}
Dart_SetReturnValue(args, result);
}

} // namespace blink
1 change: 1 addition & 0 deletions lib/ui/dart_ui.gni
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ dart_ui_files = [
"$flutter_root/lib/ui/lerp.dart",
"$flutter_root/lib/ui/natives.dart",
"$flutter_root/lib/ui/painting.dart",
"$flutter_root/lib/ui/plugins.dart",
"$flutter_root/lib/ui/pointer.dart",
"$flutter_root/lib/ui/semantics.dart",
"$flutter_root/lib/ui/text.dart",
Expand Down
5 changes: 5 additions & 0 deletions lib/ui/natives.dart
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ dynamic _saveCompilationTrace() native 'SaveCompilationTrace';

void _scheduleMicrotask(void callback()) native 'ScheduleMicrotask';

Function _lookupClosure(String name, String libraryPath, String className) native 'LookupClosure';
String _getNameOfFunctionClass(Function closure) native 'GetFunctionClassName';
String _getFunctionLibraryUrl(Function closure) native 'GetFunctionLibraryUrl';
String _getFunctionName(Function func) native 'GetFunctionName';

// Required for gen_snapshot to work correctly.
int _isolateId;

Expand Down
74 changes: 74 additions & 0 deletions lib/ui/plugins.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

part of dart.ui;

/// Functionality for Flutter plugin authors.
abstract class PluginUtilities {

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.

Is it possible to write tests of these methods? If so, I think we should have some.

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.

Yes, it should be. I'll go ahead and write some.

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.

Tests added in latest commit.

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 you talk about why you need these?

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.

These are needed to lookup callback methods which are top-level functions or static methods in a class by name. The general flow is something like this:

  • Plugin user passes callback fooBar to the plugin.
  • The plugin calls PluginUtilities.getPathForFunctionLibrary, PluginUtilities.getNameOfFunction, and PluginUtilities.getNameOfFunctionClass to get the library URL, function name, and class name (if applicable). These values are passed from native as strings to a callback handler (this is a Dart method) setup by the plugin when a background event is triggered (e.g., AlarmManager fires).
  • When in the callback handler is invoked, PluginUtilities.getClosureByName is called with the function name, class name, and library path to retrieve the closure for the plugin user's callback. The plugin can then invoke said closure.

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 am concerned about this API. From most serious to least serious concerns:

  • If someone uses this API directly with input that is in any way under the control of the user, it's a ripe for becoming an attack vector, where the user causes the app to jump to arbitrary top-level static functions.

  • This seems like it will be abused. For example, someone might store this data, pass it over the network to another instance of the program, then look up the address to jump to and do that. This would be very sketchy code, but this API doesn't make it feel bad.

  • What happens if someone caches these values across version updates, then uses them in a subsequent version of their code where the function has been tree-shaken out?

  • Least serious: if we have to have this API, it seems like we should have a single type that represents the path, name, and class of the closure.

Can you talk more broadly about what problem these methods solve? Maybe we can find a safer solution to this problem.

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.

Good point. I feel like a lot of your concerns could be addressed by an opaque class with static methods that could:

  • Create an instance of the class given a closure which contains all the path, name, and class information.
  • Take in an instance of this class and return the closure it references.

This way, we can avoid having these strings visible and modifiable by the user, prevent abuse of what are basically Dart VM APIs, and make caching difficult. The only difficulty I see with this approach is passing said object over method channels since it would have to be serialized and deserialized in such a way that the data itself still remains private, but anyone can listen to a method channel if they know the name of the channel. I'd have to give this part some thought...

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.

Your latest update resolves my concerns. Thanks!

/// Get a closure instance for a specific static or top-level function given

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.

This should warn that it will fail to find the closure and return null if the closure is not otherwise used in the program. That is, it may get tree-shaken away in an AOT build.

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.

/// a name.
///
/// **Warning:** if the function corresponding with `name` is not referenced
/// explicitly (e.g., invoked, passed as a closure, etc.) it may be optimized
/// out of the compiled application. To prevent this, avoid hard coding names

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.

nit: hardcoding

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.

/// as [String]s and prefer to use `PluginUtilities.getNameOfFunction` to
/// retrieve the function's name.
///
/// `name` is the name of the function we want a closure for. This is a
/// required parameter.

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.

Typical wording is 'This must not be null.'

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.

///
/// `libraryPath` is the path which points to the library which contains the
/// function of interest. This path can be retrieved using
/// [PluginUtilities.getPathForFunctionLibrary]. If not specified, the root
/// library will be used for the function lookup.
///
/// `className` is the name of the class in which function `name` is defined.
/// This parameter is required for retrieving closures for static methods. If
/// not provided, `name` is assumed to be referring to a top-level function.
///
/// Returns a closure for `name` as a [Function] if the lookup was successful.
/// Otherwise, `null` is returned on error.
static Function getClosureByName({String name, String libraryPath, String className}) {
if (name == null) {
throw new ArgumentError.notNull('name');
}

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.

Here and elsewhere: for null checks, generally prefer: assert(name != null);

We typically reserve ArgumentError for malformed values -- e.g. we're passed a list of length 3 when we document it must be 4, etc.

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.

return _lookupClosure(name, libraryPath, className);
}

/// Get the path for the library which contains a given method.
///
/// `closure` is the closure of the function that we want to find the library
/// path for.

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.

Add: 'This must not be null.'

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.

///
/// Returns a [String] representing the path to the library which contains
/// `closure`. Returns `null` if an error occurs or `closure` is not found

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.

Trailing period.

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.

static String getPathForFunctionLibrary(Function closure) {
if (closure == null) {
throw new ArgumentError.notNull('closure');
}
return _getFunctionLibraryUrl(closure);
}

/// Get the name of a [Function] as a [String].
///
/// Returns a [String] representing the name of the function if the function
/// exists, otherwise `null` is returned.

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.

Document that closure must not be null.

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.

static String getNameOfFunction(Function closure) {
if (closure == null) {
throw new ArgumentError.notNull('closure');
}
return _getFunctionName(closure);
}

/// Get the name of a class containing [Function] as a [String].
///
/// Returns a [String] representing the name of the function if the function
/// exists and is a member of a class, otherwise `null` is returned.
static String getNameOfFunctionClass(Function closure) {
if(closure == null) {

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.

nit: if (closure == null) {

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.

throw new ArgumentError.notNull('closure');
}
return _getNameOfFunctionClass(closure);
}
}
1 change: 1 addition & 0 deletions lib/ui/ui.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ part 'isolate_name_server.dart';
part 'lerp.dart';
part 'natives.dart';
part 'painting.dart';
part 'plugins.dart';
part 'pointer.dart';
part 'semantics.dart';
part 'text.dart';
Expand Down
42 changes: 42 additions & 0 deletions runtime/dart_isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,48 @@ bool DartIsolate::Run(const std::string& entrypoint_name) {
return true;
}

FXL_WARN_UNUSED_RESULT
bool DartIsolate::RunFromLibrary(const std::string& library_name,
const std::string& entrypoint_name) {
TRACE_EVENT0("flutter", "DartIsolate::RunFromLibrary");
if (phase_ != Phase::Ready) {
return false;
}

tonic::DartState::Scope scope(this);

Dart_Handle library = Dart_LookupLibrary(tonic::ToDart(library_name.c_str()));
if (tonic::LogIfError(library)) {
return false;
}

Dart_Handle entrypoint =
Dart_GetClosure(library, tonic::ToDart(entrypoint_name.c_str()));
if (tonic::LogIfError(entrypoint)) {
return false;
}

Dart_Handle isolate_lib = Dart_LookupLibrary(tonic::ToDart("dart:isolate"));
if (tonic::LogIfError(isolate_lib)) {
return false;
}

Dart_Handle isolate_args[] = {
entrypoint,
Dart_Null(),
};

if (tonic::LogIfError(Dart_Invoke(
isolate_lib, tonic::ToDart("_startMainIsolate"),
sizeof(isolate_args) / sizeof(isolate_args[0]), isolate_args))) {
return false;
}

phase_ = Phase::Running;
FXL_DLOG(INFO) << "New isolate is in the running state.";
return true;
}

bool DartIsolate::Shutdown() {
TRACE_EVENT0("flutter", "DartIsolate::Shutdown");
// This call may be re-entrant since Dart_ShutdownIsolate can invoke the
Expand Down
4 changes: 4 additions & 0 deletions runtime/dart_isolate.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ class DartIsolate : public UIDartState {
FXL_WARN_UNUSED_RESULT
bool Run(const std::string& entrypoint);

FXL_WARN_UNUSED_RESULT
bool RunFromLibrary(const std::string& library_name,
const std::string& entrypoint);

FXL_WARN_UNUSED_RESULT
bool Shutdown();

Expand Down
14 changes: 11 additions & 3 deletions shell/common/engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,17 @@ bool Engine::PrepareAndLaunchIsolate(RunConfiguration configuration) {
return false;
}

if (!isolate->Run(configuration.GetEntrypoint())) {
FXL_LOG(ERROR) << "Could not run the isolate.";
return false;
if (configuration.GetEntrypointLibrary().empty()) {
if (!isolate->Run(configuration.GetEntrypoint())) {
FXL_LOG(ERROR) << "Could not run the isolate.";
return false;
}
} else {
if (!isolate->RunFromLibrary(configuration.GetEntrypointLibrary(),
configuration.GetEntrypoint())) {
FXL_LOG(ERROR) << "Could not run the isolate.";
return false;
}
}

return true;
Expand Down
10 changes: 10 additions & 0 deletions shell/common/run_configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ void RunConfiguration::SetEntrypoint(std::string entrypoint) {
entrypoint_ = std::move(entrypoint);
}

void RunConfiguration::SetEntrypointAndLibrary(std::string entrypoint,
std::string library) {
SetEntrypoint(entrypoint);
entrypoint_library_ = std::move(library);
}

fml::RefPtr<blink::AssetManager> RunConfiguration::GetAssetManager() const {
return asset_manager_;
}
Expand All @@ -68,6 +74,10 @@ const std::string& RunConfiguration::GetEntrypoint() const {
return entrypoint_;
}

const std::string& RunConfiguration::GetEntrypointLibrary() const {
return entrypoint_library_;
}

std::unique_ptr<IsolateConfiguration>
RunConfiguration::TakeIsolateConfiguration() {
return std::move(isolate_configuration_);
Expand Down
5 changes: 5 additions & 0 deletions shell/common/run_configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,21 @@ class RunConfiguration {

void SetEntrypoint(std::string entrypoint);

void SetEntrypointAndLibrary(std::string entrypoint, std::string library);

fml::RefPtr<blink::AssetManager> GetAssetManager() const;

const std::string& GetEntrypoint() const;

const std::string& GetEntrypointLibrary() const;

std::unique_ptr<IsolateConfiguration> TakeIsolateConfiguration();

private:
std::unique_ptr<IsolateConfiguration> isolate_configuration_;
fml::RefPtr<blink::AssetManager> asset_manager_;
std::string entrypoint_ = "main";
std::string entrypoint_library_ = "";

FXL_DISALLOW_COPY_AND_ASSIGN(RunConfiguration);
};
Expand Down
Loading