Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce the rule of calling FlutterView.Render #45300

Merged
merged 25 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f2c3cd7
Changes from Michael
dkwingsmt Aug 25, 2023
ca0fc96
Merge remote-tracking branch 'origin/main' into enforce-rendering-rule
dkwingsmt Aug 28, 2023
f783ef3
Remove RunOnPlatformTaskRunner
dkwingsmt Aug 28, 2023
3c3ddbe
Impl
dkwingsmt Aug 28, 2023
4326d57
Merge branch 'remove-RunOnPlatformTaskRunner' into enforce-rendering-…
dkwingsmt Aug 28, 2023
9ac34e2
First running runtime controller
dkwingsmt Aug 30, 2023
5e15f5d
CallingRenderOutOfScopeIsIgnored
dkwingsmt Aug 30, 2023
8c7b5d6
Extract methods
dkwingsmt Aug 30, 2023
0d563e8
DuplicateRenderCallsAreIgnored
dkwingsmt Aug 30, 2023
49a3560
Docs and callback
dkwingsmt Aug 30, 2023
713648e
Move destruction to destructor
dkwingsmt Aug 30, 2023
48135a7
Format
dkwingsmt Aug 30, 2023
d0b1cf7
Extract Shell InferVmInitDataFromSettings
dkwingsmt Aug 30, 2023
21b2787
lint
dkwingsmt Aug 30, 2023
c38c202
Revert contents of #45190
dkwingsmt Aug 30, 2023
c4d273c
Use new mocks
dkwingsmt Aug 30, 2023
1f96e53
Merge remote-tracking branch 'origin/main' into enforce-rendering-rule
dkwingsmt Aug 30, 2023
45f0149
Lint
dkwingsmt Aug 30, 2023
cb9b8e4
Merge remote-tracking branch 'origin/main' into enforce-rendering-rule
dkwingsmt Aug 31, 2023
bc11215
correctly post sync for begin frame
dkwingsmt Aug 31, 2023
151ed77
Extract into methods
dkwingsmt Aug 31, 2023
3370e8c
Address comments
dkwingsmt Sep 1, 2023
d768cc4
Remove exact
dkwingsmt Sep 2, 2023
828b791
Merge remote-tracking branch 'dkwingsmt/enforce-rendering-rule' into …
dkwingsmt Sep 2, 2023
e266da1
Merge branch 'main' into enforce-rendering-rule
dkwingsmt Sep 5, 2023
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
42 changes: 42 additions & 0 deletions lib/ui/fixtures/ui_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1076,3 +1076,45 @@ external void _callHook(
Object? arg20,
Object? arg21,
]);

Scene _createRedBoxScene(Size size) {
final SceneBuilder builder = SceneBuilder();
builder.pushOffset(0.0, 0.0);
final Paint paint = Paint()
..color = Color.fromARGB(255, 255, 0, 0)
..style = PaintingStyle.fill;
final PictureRecorder baseRecorder = PictureRecorder();
final Canvas canvas = Canvas(baseRecorder);
canvas.drawRect(Rect.fromLTRB(0.0, 0.0, size.width, size.height), paint);
final Picture picture = baseRecorder.endRecording();
builder.addPicture(Offset(0.0, 0.0), picture);
builder.pop();
return builder.build();
}

@pragma('vm:entry-point')
void incorrectImmediateRender() {
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(2, 2)));
_finish();
// Don't schedule a frame here. This test only checks if the
// [FlutterView.render] call is propagated to PlatformConfiguration.render
// and thus doesn't need anything from `Animator` or `Engine`, which,
// besides, are not even created in the native side at all.
}

@pragma('vm:entry-point')
void incorrectDoubleRender() {
PlatformDispatcher.instance.onBeginFrame = (Duration value) {
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(2, 2)));
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(3, 3)));
};
PlatformDispatcher.instance.onDrawFrame = () {
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(4, 4)));
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(5, 5)));
};
_finish();
// Don't schedule a frame here. This test only checks if the
// [FlutterView.render] call is propagated to PlatformConfiguration.render
// and thus doesn't need anything from `Animator` or `Engine`, which,
// besides, are not even created in the native side at all.
}
dkwingsmt marked this conversation as resolved.
Show resolved Hide resolved
33 changes: 33 additions & 0 deletions lib/ui/platform_dispatcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,21 @@ class PlatformDispatcher {
_invoke(onMetricsChanged, _onMetricsChangedZone);
}

// [FlutterView]s for which [FlutterView.render] has already been called
// during the current [onBeginFrame]/[onDrawFrame] callback sequence.
//
// The field is null outside the scope of those callbacks indicating that
// calls to [FlutterView.render] must be ignored. Furthermore, if a given
// [FlutterView] is already present in this set when its [FlutterView.render]
// is called again, that call must be ignored as a duplicate.
//
// Between [onBeginFrame] and [onDrawFrame] the properties value is
// temporarily stored in `_renderedViewsBetweenCallbacks` so that it survives
// the gap between the two callbacks.
Set<FlutterView>? _renderedViews;
// Temporary storage of the `_renderedViews` value between `_beginFrame` and
// `_drawFrame`.
Set<FlutterView>? _renderedViewsBetweenCallbacks;

/// A callback invoked when any view begins a frame.
///
Expand All @@ -329,11 +344,20 @@ class PlatformDispatcher {

// Called from the engine, via hooks.dart
void _beginFrame(int microseconds) {
assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks == null);

_renderedViews = <FlutterView>{};
_invoke1<Duration>(
onBeginFrame,
_onBeginFrameZone,
Duration(microseconds: microseconds),
);
_renderedViewsBetweenCallbacks = _renderedViews;
_renderedViews = null;

assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks != null);
}

/// A callback that is invoked for each frame after [onBeginFrame] has
Expand All @@ -351,7 +375,16 @@ class PlatformDispatcher {

// Called from the engine, via hooks.dart
void _drawFrame() {
assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks != null);

_renderedViews = _renderedViewsBetweenCallbacks;
_renderedViewsBetweenCallbacks = null;
_invoke(onDrawFrame, _onDrawFrameZone);
_renderedViews = null;

assert(_renderedViews == null);
assert(_renderedViewsBetweenCallbacks == null);
}

/// A callback that is invoked when pointer data is available.
Expand Down
9 changes: 8 additions & 1 deletion lib/ui/window.dart
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,14 @@ class FlutterView {
/// scheduling of frames.
/// * [RendererBinding], the Flutter framework class which manages layout and
/// painting.
void render(Scene scene) => _render(scene as _NativeScene);
void render(Scene scene) {
if (platformDispatcher._renderedViews?.add(this) != true) {
// Duplicated calls or calls outside of onBeginFrame/onDrawFrame
// (indicated by _renderedViews being null) are ignored, as documented.
return;
}
_render(scene as _NativeScene);
}

@Native<Void Function(Pointer<Void>)>(symbol: 'PlatformConfigurationNativeApi::Render')
external static void _render(_NativeScene scene);
Expand Down
241 changes: 241 additions & 0 deletions lib/ui/window/platform_configuration_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,172 @@
#include "flutter/shell/common/shell_test.h"
#include "flutter/shell/common/thread_host.h"
#include "flutter/testing/testing.h"
#include "gmock/gmock.h"

namespace flutter {

namespace {

static constexpr int64_t kImplicitViewId = 0;

static void PostSync(const fml::RefPtr<fml::TaskRunner>& task_runner,
const fml::closure& task) {
fml::AutoResetWaitableEvent latch;
fml::TaskRunner::RunNowOrPostTask(task_runner, [&latch, &task] {
task();
latch.Signal();
});
latch.Wait();
}

class MockRuntimeDelegate : public RuntimeDelegate {
public:
MOCK_METHOD(std::string, DefaultRouteName, (), (override));
MOCK_METHOD(void, ScheduleFrame, (bool), (override));
MOCK_METHOD(void,
Render,
(std::unique_ptr<flutter::LayerTree>, float),
(override));
MOCK_METHOD(void,
UpdateSemantics,
(SemanticsNodeUpdates, CustomAccessibilityActionUpdates),
(override));
MOCK_METHOD(void,
HandlePlatformMessage,
(std::unique_ptr<PlatformMessage>),
(override));
MOCK_METHOD(FontCollection&, GetFontCollection, (), (override));
MOCK_METHOD(std::shared_ptr<AssetManager>, GetAssetManager, (), (override));
MOCK_METHOD(void, OnRootIsolateCreated, (), (override));
MOCK_METHOD(void,
UpdateIsolateDescription,
(const std::string, int64_t),
(override));
MOCK_METHOD(void, SetNeedsReportTimings, (bool), (override));
MOCK_METHOD(std::unique_ptr<std::vector<std::string>>,
ComputePlatformResolvedLocale,
(const std::vector<std::string>&),
(override));
MOCK_METHOD(void, RequestDartDeferredLibrary, (intptr_t), (override));
MOCK_METHOD(std::weak_ptr<PlatformMessageHandler>,
GetPlatformMessageHandler,
(),
(const, override));
MOCK_METHOD(void, SendChannelUpdate, (std::string, bool), (override));
MOCK_METHOD(double,
GetScaledFontSize,
(double font_size, int configuration_id),
(const, override));
};

class MockPlatformMessageHandler : public PlatformMessageHandler {
public:
MOCK_METHOD(void,
HandlePlatformMessage,
(std::unique_ptr<PlatformMessage> message),
(override));
MOCK_METHOD(bool,
DoesHandlePlatformMessageOnPlatformThread,
(),
(const, override));
MOCK_METHOD(void,
InvokePlatformMessageResponseCallback,
(int response_id, std::unique_ptr<fml::Mapping> mapping),
(override));
MOCK_METHOD(void,
InvokePlatformMessageEmptyResponseCallback,
(int response_id),
(override));
};

// A class that can launch a RuntimeController with the specified
// RuntimeDelegate.
//
// To use this class, contruct this class with Create, call LaunchRootIsolate,
// and use the controller with ControllerTaskSync().
class RuntimeControllerContext {
public:
using ControllerCallback = std::function<void(RuntimeController&)>;

[[nodiscard]] static std::unique_ptr<RuntimeControllerContext> Create(
Settings settings, //
const TaskRunners& task_runners, //
RuntimeDelegate& client) {
auto [vm, isolate_snapshot] = Shell::InferVmInitDataFromSettings(settings);
FML_CHECK(vm) << "Must be able to initialize the VM.";
// Construct the class with `new` because `make_unique` has no access to the
// private constructor.
RuntimeControllerContext* raw_pointer = new RuntimeControllerContext(
settings, task_runners, client, std::move(vm), isolate_snapshot);
return std::unique_ptr<RuntimeControllerContext>(raw_pointer);
}

~RuntimeControllerContext() {
PostSync(task_runners_.GetUITaskRunner(),
[&]() { runtime_controller_.reset(); });
}

// Launch the root isolate. The post_launch callback will be executed in the
// same UI task, which can be used to create initial views.
void LaunchRootIsolate(RunConfiguration& configuration,
ControllerCallback post_launch) {
PostSync(task_runners_.GetUITaskRunner(), [&]() {
bool launch_success = runtime_controller_->LaunchRootIsolate(
settings_, //
[]() {}, //
configuration.GetEntrypoint(), //
configuration.GetEntrypointLibrary(), //
configuration.GetEntrypointArgs(), //
configuration.TakeIsolateConfiguration()); //
ASSERT_TRUE(launch_success);
post_launch(*runtime_controller_);
});
}

// Run a task that operates the RuntimeController on the UI thread, and wait
// for the task to end.
void ControllerTaskSync(ControllerCallback task) {
ASSERT_TRUE(runtime_controller_);
ASSERT_TRUE(task);
PostSync(task_runners_.GetUITaskRunner(),
[&]() { task(*runtime_controller_); });
}

private:
RuntimeControllerContext(const Settings& settings,
const TaskRunners& task_runners,
RuntimeDelegate& client,
DartVMRef vm,
fml::RefPtr<const DartSnapshot> isolate_snapshot)
: settings_(settings),
task_runners_(task_runners),
isolate_snapshot_(std::move(isolate_snapshot)),
vm_(std::move(vm)),
runtime_controller_(std::make_unique<RuntimeController>(
client,
&vm_,
std::move(isolate_snapshot_),
settings.idle_notification_callback, // idle notification callback
flutter::PlatformData(), // platform data
settings.isolate_create_callback, // isolate create callback
settings.isolate_shutdown_callback, // isolate shutdown callback
settings.persistent_isolate_data, // persistent isolate data
UIDartState::Context{task_runners})) {}

Settings settings_;
TaskRunners task_runners_;
fml::RefPtr<const DartSnapshot> isolate_snapshot_;
DartVMRef vm_;
std::unique_ptr<RuntimeController> runtime_controller_;
};
} // namespace

namespace testing {

using ::testing::_;
using ::testing::Exactly;
using ::testing::Return;

class PlatformConfigurationTest : public ShellTest {};

TEST_F(PlatformConfigurationTest, Initialization) {
Expand Down Expand Up @@ -332,5 +494,84 @@ TEST_F(PlatformConfigurationTest, SetDartPerformanceMode) {
DestroyShell(std::move(shell), task_runners);
}

TEST_F(PlatformConfigurationTest, OutOfScopeRenderCallsAreIgnored) {
Settings settings = CreateSettingsForFixture();
TaskRunners task_runners = GetTaskRunnersForFixture();

MockRuntimeDelegate client;
auto platform_message_handler =
std::make_shared<MockPlatformMessageHandler>();
EXPECT_CALL(client, GetPlatformMessageHandler)
.WillOnce(Return(platform_message_handler));
// Render should not be called.
EXPECT_CALL(client, Render).Times(Exactly(0));
Copy link
Member

Choose a reason for hiding this comment

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

Flutter engine code base doesn't appear to use Exactly: https://github.com/search?q=repo%3Aflutter%2Fengine+%22Exactly%28%22&type=code

Suggested change
EXPECT_CALL(client, Render).Times(Exactly(0));
EXPECT_CALL(client, Render).Times(0);

Please update other locations too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this comment. I've removed it. Didn't know that Exactly is the default (I thought the default was AtLeast. :)


auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
auto finish = [message_latch](Dart_NativeArguments args) {
message_latch->Signal();
};
AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish));

auto runtime_controller_context =
RuntimeControllerContext::Create(settings, task_runners, client);

auto configuration = RunConfiguration::InferFromSettings(settings);
configuration.SetEntrypoint("incorrectImmediateRender");
runtime_controller_context->LaunchRootIsolate(
configuration, [](RuntimeController& runtime_controller) {
runtime_controller.AddView(
kImplicitViewId,
ViewportMetrics(
/*pixel_ratio=*/1.0, /*width=*/20, /*height=*/20,
/*touch_slop=*/2, /*display_id=*/0));
});

// Wait for the Dart main function to end.
message_latch->Wait();
}

TEST_F(PlatformConfigurationTest, DuplicateRenderCallsAreIgnored) {
Settings settings = CreateSettingsForFixture();
TaskRunners task_runners = GetTaskRunnersForFixture();

MockRuntimeDelegate client;
auto platform_message_handler =
std::make_shared<MockPlatformMessageHandler>();
EXPECT_CALL(client, GetPlatformMessageHandler)
.WillOnce(Return(platform_message_handler));
// Render should only be called once, because the second call is ignored.
EXPECT_CALL(client, Render).Times(Exactly(1));

auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
auto finish = [message_latch](Dart_NativeArguments args) {
message_latch->Signal();
};
AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish));

auto runtime_controller_context =
RuntimeControllerContext::Create(settings, task_runners, client);

auto configuration = RunConfiguration::InferFromSettings(settings);
configuration.SetEntrypoint("incorrectDoubleRender");
runtime_controller_context->LaunchRootIsolate(
configuration, [](RuntimeController& runtime_controller) {
runtime_controller.AddView(
kImplicitViewId,
ViewportMetrics(
/*pixel_ratio=*/1.0, /*width=*/20, /*height=*/20,
/*touch_slop=*/2, /*display_id=*/0));
});

// Wait for the Dart main function to end.
message_latch->Wait();

runtime_controller_context->ControllerTaskSync(
[](RuntimeController& runtime_controller) {
// This BeginFrame calls PlatformDispatcher's handleBeginFrame and
// handleDrawFrame synchronously. Therefore don't wait after it.
runtime_controller.BeginFrame(fml::TimePoint::Now(), 0);
});
}

} // namespace testing
} // namespace flutter
Loading