Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 70fe459

Browse files
dkwingsmtdkwingsmt
authored
Enforce the rule of calling FlutterView.Render (#45300)
This PR enforces the rules as documented in `FlutterView.Render`, where calls in illegal situations should be ignored - but have never been enforced. ``` /// This function must be called within the scope of the /// [PlatformDispatcher.onBeginFrame] or [PlatformDispatcher.onDrawFrame] /// callbacks being invoked. /// /// If this function is called a second time during a single /// [PlatformDispatcher.onBeginFrame]/[PlatformDispatcher.onDrawFrame] /// callback sequence or called outside the scope of those callbacks, the call /// will be ignored. ``` This rule is very important to implementing multi-view without having to introduce new APIs. However, currently these illegal calls are not ignored, and historically many tests (especially integration tests) were unknowingly running based on this fact. @goderbauer did great work by eliminating these cases in g3, and it's time for us to make sure these calls are ignored. Most effort of this PR goes to unit testing the changes. Some part of `Shell::Create` is extracted into a static function to avoid duplicate code. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
1 parent be04bf3 commit 70fe459

File tree

6 files changed

+351
-14
lines changed

6 files changed

+351
-14
lines changed

‎lib/ui/fixtures/ui_test.dart‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,3 +1076,45 @@ external void _callHook(
10761076
Object? arg20,
10771077
Object? arg21,
10781078
]);
1079+
1080+
Scene _createRedBoxScene(Size size) {
1081+
final SceneBuilder builder = SceneBuilder();
1082+
builder.pushOffset(0.0, 0.0);
1083+
final Paint paint = Paint()
1084+
..color = Color.fromARGB(255, 255, 0, 0)
1085+
..style = PaintingStyle.fill;
1086+
final PictureRecorder baseRecorder = PictureRecorder();
1087+
final Canvas canvas = Canvas(baseRecorder);
1088+
canvas.drawRect(Rect.fromLTRB(0.0, 0.0, size.width, size.height), paint);
1089+
final Picture picture = baseRecorder.endRecording();
1090+
builder.addPicture(Offset(0.0, 0.0), picture);
1091+
builder.pop();
1092+
return builder.build();
1093+
}
1094+
1095+
@pragma('vm:entry-point')
1096+
void incorrectImmediateRender() {
1097+
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(2, 2)));
1098+
_finish();
1099+
// Don't schedule a frame here. This test only checks if the
1100+
// [FlutterView.render] call is propagated to PlatformConfiguration.render
1101+
// and thus doesn't need anything from `Animator` or `Engine`, which,
1102+
// besides, are not even created in the native side at all.
1103+
}
1104+
1105+
@pragma('vm:entry-point')
1106+
void incorrectDoubleRender() {
1107+
PlatformDispatcher.instance.onBeginFrame = (Duration value) {
1108+
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(2, 2)));
1109+
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(3, 3)));
1110+
};
1111+
PlatformDispatcher.instance.onDrawFrame = () {
1112+
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(4, 4)));
1113+
PlatformDispatcher.instance.views.first.render(_createRedBoxScene(Size(5, 5)));
1114+
};
1115+
_finish();
1116+
// Don't schedule a frame here. This test only checks if the
1117+
// [FlutterView.render] call is propagated to PlatformConfiguration.render
1118+
// and thus doesn't need anything from `Animator` or `Engine`, which,
1119+
// besides, are not even created in the native side at all.
1120+
}

‎lib/ui/platform_dispatcher.dart‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,21 @@ class PlatformDispatcher {
308308
_invoke(onMetricsChanged, _onMetricsChangedZone);
309309
}
310310

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

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

330345
// Called from the engine, via hooks.dart
331346
void _beginFrame(int microseconds) {
347+
assert(_renderedViews == null);
348+
assert(_renderedViewsBetweenCallbacks == null);
349+
350+
_renderedViews = <FlutterView>{};
332351
_invoke1<Duration>(
333352
onBeginFrame,
334353
_onBeginFrameZone,
335354
Duration(microseconds: microseconds),
336355
);
356+
_renderedViewsBetweenCallbacks = _renderedViews;
357+
_renderedViews = null;
358+
359+
assert(_renderedViews == null);
360+
assert(_renderedViewsBetweenCallbacks != null);
337361
}
338362

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

352376
// Called from the engine, via hooks.dart
353377
void _drawFrame() {
378+
assert(_renderedViews == null);
379+
assert(_renderedViewsBetweenCallbacks != null);
380+
381+
_renderedViews = _renderedViewsBetweenCallbacks;
382+
_renderedViewsBetweenCallbacks = null;
354383
_invoke(onDrawFrame, _onDrawFrameZone);
384+
_renderedViews = null;
385+
386+
assert(_renderedViews == null);
387+
assert(_renderedViewsBetweenCallbacks == null);
355388
}
356389

357390
/// A callback that is invoked when pointer data is available.

‎lib/ui/window.dart‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,14 @@ class FlutterView {
353353
/// scheduling of frames.
354354
/// * [RendererBinding], the Flutter framework class which manages layout and
355355
/// painting.
356-
void render(Scene scene) => _render(scene as _NativeScene);
356+
void render(Scene scene) {
357+
if (platformDispatcher._renderedViews?.add(this) != true) {
358+
// Duplicated calls or calls outside of onBeginFrame/onDrawFrame
359+
// (indicated by _renderedViews being null) are ignored, as documented.
360+
return;
361+
}
362+
_render(scene as _NativeScene);
363+
}
357364

358365
@Native<Void Function(Pointer<Void>)>(symbol: 'PlatformConfigurationNativeApi::Render')
359366
external static void _render(_NativeScene scene);

‎lib/ui/window/platform_configuration_unittests.cc‎

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,171 @@
1515
#include "flutter/shell/common/shell_test.h"
1616
#include "flutter/shell/common/thread_host.h"
1717
#include "flutter/testing/testing.h"
18+
#include "gmock/gmock.h"
1819

1920
namespace flutter {
21+
22+
namespace {
23+
24+
static constexpr int64_t kImplicitViewId = 0;
25+
26+
static void PostSync(const fml::RefPtr<fml::TaskRunner>& task_runner,
27+
const fml::closure& task) {
28+
fml::AutoResetWaitableEvent latch;
29+
fml::TaskRunner::RunNowOrPostTask(task_runner, [&latch, &task] {
30+
task();
31+
latch.Signal();
32+
});
33+
latch.Wait();
34+
}
35+
36+
class MockRuntimeDelegate : public RuntimeDelegate {
37+
public:
38+
MOCK_METHOD(std::string, DefaultRouteName, (), (override));
39+
MOCK_METHOD(void, ScheduleFrame, (bool), (override));
40+
MOCK_METHOD(void,
41+
Render,
42+
(std::unique_ptr<flutter::LayerTree>, float),
43+
(override));
44+
MOCK_METHOD(void,
45+
UpdateSemantics,
46+
(SemanticsNodeUpdates, CustomAccessibilityActionUpdates),
47+
(override));
48+
MOCK_METHOD(void,
49+
HandlePlatformMessage,
50+
(std::unique_ptr<PlatformMessage>),
51+
(override));
52+
MOCK_METHOD(FontCollection&, GetFontCollection, (), (override));
53+
MOCK_METHOD(std::shared_ptr<AssetManager>, GetAssetManager, (), (override));
54+
MOCK_METHOD(void, OnRootIsolateCreated, (), (override));
55+
MOCK_METHOD(void,
56+
UpdateIsolateDescription,
57+
(const std::string, int64_t),
58+
(override));
59+
MOCK_METHOD(void, SetNeedsReportTimings, (bool), (override));
60+
MOCK_METHOD(std::unique_ptr<std::vector<std::string>>,
61+
ComputePlatformResolvedLocale,
62+
(const std::vector<std::string>&),
63+
(override));
64+
MOCK_METHOD(void, RequestDartDeferredLibrary, (intptr_t), (override));
65+
MOCK_METHOD(std::weak_ptr<PlatformMessageHandler>,
66+
GetPlatformMessageHandler,
67+
(),
68+
(const, override));
69+
MOCK_METHOD(void, SendChannelUpdate, (std::string, bool), (override));
70+
MOCK_METHOD(double,
71+
GetScaledFontSize,
72+
(double font_size, int configuration_id),
73+
(const, override));
74+
};
75+
76+
class MockPlatformMessageHandler : public PlatformMessageHandler {
77+
public:
78+
MOCK_METHOD(void,
79+
HandlePlatformMessage,
80+
(std::unique_ptr<PlatformMessage> message),
81+
(override));
82+
MOCK_METHOD(bool,
83+
DoesHandlePlatformMessageOnPlatformThread,
84+
(),
85+
(const, override));
86+
MOCK_METHOD(void,
87+
InvokePlatformMessageResponseCallback,
88+
(int response_id, std::unique_ptr<fml::Mapping> mapping),
89+
(override));
90+
MOCK_METHOD(void,
91+
InvokePlatformMessageEmptyResponseCallback,
92+
(int response_id),
93+
(override));
94+
};
95+
96+
// A class that can launch a RuntimeController with the specified
97+
// RuntimeDelegate.
98+
//
99+
// To use this class, contruct this class with Create, call LaunchRootIsolate,
100+
// and use the controller with ControllerTaskSync().
101+
class RuntimeControllerContext {
102+
public:
103+
using ControllerCallback = std::function<void(RuntimeController&)>;
104+
105+
[[nodiscard]] static std::unique_ptr<RuntimeControllerContext> Create(
106+
Settings settings, //
107+
const TaskRunners& task_runners, //
108+
RuntimeDelegate& client) {
109+
auto [vm, isolate_snapshot] = Shell::InferVmInitDataFromSettings(settings);
110+
FML_CHECK(vm) << "Must be able to initialize the VM.";
111+
// Construct the class with `new` because `make_unique` has no access to the
112+
// private constructor.
113+
RuntimeControllerContext* raw_pointer = new RuntimeControllerContext(
114+
settings, task_runners, client, std::move(vm), isolate_snapshot);
115+
return std::unique_ptr<RuntimeControllerContext>(raw_pointer);
116+
}
117+
118+
~RuntimeControllerContext() {
119+
PostSync(task_runners_.GetUITaskRunner(),
120+
[&]() { runtime_controller_.reset(); });
121+
}
122+
123+
// Launch the root isolate. The post_launch callback will be executed in the
124+
// same UI task, which can be used to create initial views.
125+
void LaunchRootIsolate(RunConfiguration& configuration,
126+
ControllerCallback post_launch) {
127+
PostSync(task_runners_.GetUITaskRunner(), [&]() {
128+
bool launch_success = runtime_controller_->LaunchRootIsolate(
129+
settings_, //
130+
[]() {}, //
131+
configuration.GetEntrypoint(), //
132+
configuration.GetEntrypointLibrary(), //
133+
configuration.GetEntrypointArgs(), //
134+
configuration.TakeIsolateConfiguration()); //
135+
ASSERT_TRUE(launch_success);
136+
post_launch(*runtime_controller_);
137+
});
138+
}
139+
140+
// Run a task that operates the RuntimeController on the UI thread, and wait
141+
// for the task to end.
142+
void ControllerTaskSync(ControllerCallback task) {
143+
ASSERT_TRUE(runtime_controller_);
144+
ASSERT_TRUE(task);
145+
PostSync(task_runners_.GetUITaskRunner(),
146+
[&]() { task(*runtime_controller_); });
147+
}
148+
149+
private:
150+
RuntimeControllerContext(const Settings& settings,
151+
const TaskRunners& task_runners,
152+
RuntimeDelegate& client,
153+
DartVMRef vm,
154+
fml::RefPtr<const DartSnapshot> isolate_snapshot)
155+
: settings_(settings),
156+
task_runners_(task_runners),
157+
isolate_snapshot_(std::move(isolate_snapshot)),
158+
vm_(std::move(vm)),
159+
runtime_controller_(std::make_unique<RuntimeController>(
160+
client,
161+
&vm_,
162+
std::move(isolate_snapshot_),
163+
settings.idle_notification_callback, // idle notification callback
164+
flutter::PlatformData(), // platform data
165+
settings.isolate_create_callback, // isolate create callback
166+
settings.isolate_shutdown_callback, // isolate shutdown callback
167+
settings.persistent_isolate_data, // persistent isolate data
168+
UIDartState::Context{task_runners})) {}
169+
170+
Settings settings_;
171+
TaskRunners task_runners_;
172+
fml::RefPtr<const DartSnapshot> isolate_snapshot_;
173+
DartVMRef vm_;
174+
std::unique_ptr<RuntimeController> runtime_controller_;
175+
};
176+
} // namespace
177+
20178
namespace testing {
21179

180+
using ::testing::_;
181+
using ::testing::Return;
182+
22183
class PlatformConfigurationTest : public ShellTest {};
23184

24185
TEST_F(PlatformConfigurationTest, Initialization) {
@@ -332,5 +493,84 @@ TEST_F(PlatformConfigurationTest, SetDartPerformanceMode) {
332493
DestroyShell(std::move(shell), task_runners);
333494
}
334495

496+
TEST_F(PlatformConfigurationTest, OutOfScopeRenderCallsAreIgnored) {
497+
Settings settings = CreateSettingsForFixture();
498+
TaskRunners task_runners = GetTaskRunnersForFixture();
499+
500+
MockRuntimeDelegate client;
501+
auto platform_message_handler =
502+
std::make_shared<MockPlatformMessageHandler>();
503+
EXPECT_CALL(client, GetPlatformMessageHandler)
504+
.WillOnce(Return(platform_message_handler));
505+
// Render should not be called.
506+
EXPECT_CALL(client, Render).Times(0);
507+
508+
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
509+
auto finish = [message_latch](Dart_NativeArguments args) {
510+
message_latch->Signal();
511+
};
512+
AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish));
513+
514+
auto runtime_controller_context =
515+
RuntimeControllerContext::Create(settings, task_runners, client);
516+
517+
auto configuration = RunConfiguration::InferFromSettings(settings);
518+
configuration.SetEntrypoint("incorrectImmediateRender");
519+
runtime_controller_context->LaunchRootIsolate(
520+
configuration, [](RuntimeController& runtime_controller) {
521+
runtime_controller.AddView(
522+
kImplicitViewId,
523+
ViewportMetrics(
524+
/*pixel_ratio=*/1.0, /*width=*/20, /*height=*/20,
525+
/*touch_slop=*/2, /*display_id=*/0));
526+
});
527+
528+
// Wait for the Dart main function to end.
529+
message_latch->Wait();
530+
}
531+
532+
TEST_F(PlatformConfigurationTest, DuplicateRenderCallsAreIgnored) {
533+
Settings settings = CreateSettingsForFixture();
534+
TaskRunners task_runners = GetTaskRunnersForFixture();
535+
536+
MockRuntimeDelegate client;
537+
auto platform_message_handler =
538+
std::make_shared<MockPlatformMessageHandler>();
539+
EXPECT_CALL(client, GetPlatformMessageHandler)
540+
.WillOnce(Return(platform_message_handler));
541+
// Render should only be called once, because the second call is ignored.
542+
EXPECT_CALL(client, Render).Times(1);
543+
544+
auto message_latch = std::make_shared<fml::AutoResetWaitableEvent>();
545+
auto finish = [message_latch](Dart_NativeArguments args) {
546+
message_latch->Signal();
547+
};
548+
AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish));
549+
550+
auto runtime_controller_context =
551+
RuntimeControllerContext::Create(settings, task_runners, client);
552+
553+
auto configuration = RunConfiguration::InferFromSettings(settings);
554+
configuration.SetEntrypoint("incorrectDoubleRender");
555+
runtime_controller_context->LaunchRootIsolate(
556+
configuration, [](RuntimeController& runtime_controller) {
557+
runtime_controller.AddView(
558+
kImplicitViewId,
559+
ViewportMetrics(
560+
/*pixel_ratio=*/1.0, /*width=*/20, /*height=*/20,
561+
/*touch_slop=*/2, /*display_id=*/0));
562+
});
563+
564+
// Wait for the Dart main function to end.
565+
message_latch->Wait();
566+
567+
runtime_controller_context->ControllerTaskSync(
568+
[](RuntimeController& runtime_controller) {
569+
// This BeginFrame calls PlatformDispatcher's handleBeginFrame and
570+
// handleDrawFrame synchronously. Therefore don't wait after it.
571+
runtime_controller.BeginFrame(fml::TimePoint::Now(), 0);
572+
});
573+
}
574+
335575
} // namespace testing
336576
} // namespace flutter

0 commit comments

Comments
 (0)