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

Commit 8f49b4a

Browse files
authored
Fix flakiness in FlutterVSyncWaiterTest.VSyncWorks and FlutterDisplayLinkTest.WorkaroundForFB13482573 (#51405)
- Removes potential source of flakiness where the tests assumes that scheduled block will be performed within certain time period, which, despite the tolerances may not be the case on test runner. - Ensures that `viewDidChangeWindow` notification is not received while invalidating the displayLink and removing the display link view, which could deadlock at the end of `WorkaroundForFB13482573` test because the notification would come while `FlutterDisplayLink` is in `@synchronized` block. *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] 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 [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [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 622b372 commit 8f49b4a

File tree

2 files changed

+22
-5
lines changed

2 files changed

+22
-5
lines changed

shell/platform/darwin/macos/framework/Source/FlutterDisplayLink.mm

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,8 +262,11 @@ - (instancetype)initWithView:(NSView*)view {
262262
- (void)invalidate {
263263
@synchronized(self) {
264264
FML_DCHECK([NSThread isMainThread]);
265-
[_view removeFromSuperview];
265+
// Unregister observer before removing the view to ensure
266+
// that the viewDidChangeWindow notification is not received
267+
// while in @synchronized block.
266268
[[NSNotificationCenter defaultCenter] removeObserver:self];
269+
[_view removeFromSuperview];
267270
_view = nil;
268271
_delegate = nil;
269272
}

shell/platform/darwin/macos/framework/Source/FlutterVSyncWaiterTest.mm

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,14 @@ static void BusyWait(CFTimeInterval duration) {
140140
CFRunLoopStop(CFRunLoopGetCurrent());
141141
}];
142142

143+
__block CFTimeInterval expectedStartUntil;
144+
// Warm up tick is scheduled immediately in a scheduled block. Schedule another
145+
// block here to determine the maximum time when the warm up tick should be
146+
// scheduled.
143147
[waiter waitForVSync:kWarmUpBaton];
148+
[[NSRunLoop currentRunLoop] performBlock:^{
149+
expectedStartUntil = CACurrentMediaTime();
150+
}];
144151

145152
// Reference vsync to setup phase.
146153
CFTimeInterval now = CACurrentMediaTime();
@@ -166,15 +173,22 @@ static void BusyWait(CFTimeInterval duration) {
166173
// Vsync without baton should pause the display link.
167174
[displayLink tickWithTimestamp:now + 3.5 * displayLink.nominalOutputRefreshPeriod
168175
targetTimestamp:now + 5 * displayLink.nominalOutputRefreshPeriod];
169-
// Make sure to run the timer scheduled in display link callback.
170-
CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.02, NO);
176+
177+
CFTimeInterval start = CACurrentMediaTime();
178+
while (!displayLink.paused) {
179+
// Make sure to run the timer scheduled in display link callback.
180+
CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.02, NO);
181+
if (CACurrentMediaTime() - start > 1.0) {
182+
break;
183+
}
184+
}
171185
ASSERT_TRUE(displayLink.paused);
172186

173187
EXPECT_EQ(entries.size(), size_t(4));
174188

175189
// Warm up frame should be presented as soon as possible.
176-
EXPECT_TRUE(fabs(entries[0].timestamp - now) < 0.005);
177-
EXPECT_TRUE(fabs(entries[0].targetTimestamp - now) < 0.005);
190+
EXPECT_TRUE(entries[0].timestamp <= expectedStartUntil);
191+
EXPECT_TRUE(entries[0].targetTimestamp <= expectedStartUntil);
178192
EXPECT_EQ(entries[0].baton, kWarmUpBaton);
179193

180194
EXPECT_DOUBLE_EQ(entries[1].timestamp, now + displayLink.nominalOutputRefreshPeriod);

0 commit comments

Comments
 (0)