Fix first frame logic #11027

dnfield · 2019-08-15T17:29:40Z

The first frame callback logic has a race where it is depending on the PlatformView to determine if the ViewController is alive or not - which is not always true. This can result in a crash when a ViewController gets popped before the first frame callback has fired, and has made some of our tests very flaky of late.

/cc @jmagman @chinmaygarde @stuartmorgan

jmagman · 2019-08-15T17:32:28Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+            if (!weak_self) {
+              return;
+            }
+            if (weak_self.get()->_splashScreenView) {


weak_self and weak_platform_view could be released after their nil checks. Retain them before you use them, then release when you're done with them.

They can't be released here because we're on the queue that is responsible for them and we're not yielding.

I'm not sure how I could retain them - the get() just gives me a raw pointer that I can't retain.

I don't know how these fml::WeakPtr work, I'm going to defer to someone who knows this code better than I do.

a raw pointer that I can't retain

I don't follow; if you have a pointer to an ObjC object, then you can retain it. It's best practice to retain objects across making multiple calls to them (as jmagman is suggesting) in case one of the calls along the way would otherwise cause it to be deallocated as a side effect. (It's a rare edge case, but it's easy enough to avoid with retains, and hard enough to debug when it happens, that Apple actually added specific warnings about it in ARC code.)

Ahh ok, I can retain the view controller but not the platform view. Is that better?

dnfield · 2019-08-15T17:51:13Z

I can't think of a good way to test this. If I could somehow grab the GPU task runner and latch it, I could make this reproduce, but I don't have a good way to do that via the public API and I think it's a bad idea to expose it. Even if I could expose it, I'd have to write a C++ based test and I'm not quite sure how that would play with our current test harnesses for iOS.

stuartmorgan-g

Overall structure LG. I don't have any suggestions offhand on testing since I'm not familiar with the larger context of this code.

stuartmorgan-g · 2019-08-15T19:02:42Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+            if (!weak_self) {
+              return;
+            }
+            FlutterViewController* flutter_view_controller =


Prefer fml::scoped_nsobject to manual retain/release, as it's less error-prone.

stuartmorgan-g · 2019-08-15T19:06:58Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+       gpu_task_runner = [_engine.get() gpuTaskRunner]]() {
+        FML_DCHECK(gpu_task_runner->RunsTasksOnCurrentThread());
+        platform_task_runner->PostTask([weak_platform_view]() {
+          // The weak platform view will have a weak pointer to self, if it is


s/to self/to this object/ (I had to go read the code and come back to understand that "self" in this context was the view controller, not the view; I would expect "a pointer to self" to mean the object that the sentence is about, not the object the code is in)

stuartmorgan-g · 2019-08-15T19:08:04Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+        FML_DCHECK(gpu_task_runner->RunsTasksOnCurrentThread());
+        platform_task_runner->PostTask([weak_platform_view]() {
+          // The weak platform view will have a weak pointer to self, if it is
+          // still alive. We can check if that weak pointer is still valid to


"We" is really ambiguous in this sentence (go/avoidwe); please rephrase to be explicit about the things being referred to.

stuartmorgan-g · 2019-08-15T19:09:38Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

+          if (weak_platform_view) {
+            fml::WeakPtr<FlutterViewController> weak_self =
+                weak_platform_view->GetOwnerViewController();
+            if (!weak_self) {


The check should be on the strong pointer, after converting weak to strong. (It only actually matters when things could be released on another thread, but the convention is a fairly common one in this type of ObjC code and it's easier for readers who are familiar with it if you follow the convention.)

fml::WeakPtr has an operator defined for this, and this is more typically how it's used throughout the codebase AFAICT.

This isn't about fml::WeakPtr, it's about following standard ObjC patterns (even though the specific types are unusual). The standard, recommended approach for doing any non-trivial logic (i.e., more than a single call) on a weak pointer in a callback block is:

convert weak pointer to strong point

check that the strong pointer is non-nil; if it is don't do anything else

do whatever logic, using only the strong pointer

Reversing steps 1 and 2, as you have here, creates a race condition in some situations, and has no advantages.

I've actually simplified this a bit further - but still checking before retaining. Does that look good, or do you really want me to call get and retain on what's potentially a null pointer?

do you really want me to call get and retain on what's potentially a null pointer

Yes I do, for the reasons described in my two previous comments. The last revisions didn't change the pattern.

(And to be pedantic, you're not calling get() on a null pointer, which sounds like a crash, but calling get() on a perfectly valid object to return a pointer which may be nil. And retaining nil is a well-defined no-op.)

TIL. Updated.

stuartmorgan-g · 2019-08-15T19:13:00Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm


 - (void)installFirstFrameCallback {
-  auto weak_platform_view = [_engine.get() platformView];
+  fml::WeakPtr<flutter::PlatformViewIOS> weak_platform_view = [_engine.get() platformView];


Since at this point you are pretty much rewriting the whole method, could you fix the fact that all of the local variable names violate ObjC style? This should be weakPlatformView, and similarly for all the others.

stuartmorgan-g · 2019-08-15T19:13:49Z

shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h

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

 - (fml::RefPtr<fml::TaskRunner>)platformTaskRunner;
+- (fml::RefPtr<fml::TaskRunner>)gpuTaskRunner;


GPUTaskRunner (per ObjC style)

dnfield · 2019-08-15T19:40:00Z

Well, FWIW, before these changes the canPopFullScreen test in ios_add2app was flaking locally pretty frequently (~1/3 runs).

With this change, I haven't been able to reproduce it in 10+ runs.

Also, /cc @AlbertWang0116, as I believe you had reported something similar to this in the past.

dnfield · 2019-08-15T20:01:56Z

Given my local results, I'd like to land this without a specific test, on the supposition that it's fixing flakiness in existing tests and writing a focused test on this behavior exactly doesn't appear to be possible with our current testing setup.

dnfield · 2019-08-15T20:40:01Z

Going to land this on red, as redness is due to an infra config issue that has been fixed.

EDIT: Landing a safer PR first then will land this once infra is green.

jmagman

LGTM

[email protected]:flutter/engine.git/compare/f8e7453f1106...4b7a552 git log f8e7453..4b7a552 --no-merges --oneline 2019-08-16 [email protected] Roll Dart back to e35e8833 (flutter/engine#11043) 2019-08-16 [email protected] Roll src/third_party/dart 306f8e04bb..fecc4c8f2d (4 commits) 2019-08-16 [email protected] Roll src/third_party/skia 963a606677e1..6a519b8dd895 (6 commits) (flutter/engine#11042) 2019-08-15 [email protected] Hide verbose dart snapshot during run_test.py (flutter/engine#11040) 2019-08-15 [email protected] Remove ability to override mac_sdk_path in flutter/tools/gn (flutter/engine#11013) 2019-08-15 [email protected] Roll src/third_party/dart cd16fba718..306f8e04bb (10 commits) 2019-08-15 [email protected] Roll buildroot to pick up recent macOS changes (flutter/engine#11037) 2019-08-15 [email protected] Remove the ParagraphImpl class from the text API (flutter/engine#11012) 2019-08-15 [email protected] Disable a deprecation warning for use of a TaskDescription constructor for older platforms (flutter/engine#11029) 2019-08-15 [email protected] Re-lands platform brightness support on iOS, plus platform contrast (flutter/engine#10791) 2019-08-15 [email protected] [fuchsia] Add required trace so files for fuchsia fars (flutter/engine#11036) 2019-08-15 [email protected] Roll src/third_party/skia e5dc1ebc864a..963a606677e1 (14 commits) (flutter/engine#11032) 2019-08-15 [email protected] Add _glfw versions of the GLFW desktop libraries (flutter/engine#11024) 2019-08-15 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _fvZN... to 5Nhwb... (flutter/engine#11028) 2019-08-15 [email protected] Roll src/third_party/dart 9552646dc4..cd16fba718 (5 commits) 2019-08-15 [email protected] Fix first frame logic (flutter/engine#11027) 2019-08-15 [email protected] remove OS version (flutter/engine#11033) 2019-08-15 [email protected] Roll src/third_party/skia e30a485a68c9..e5dc1ebc864a (7 commits) (flutter/engine#11025) 2019-08-15 [email protected] Roll src/third_party/dart cae08c6813..9552646dc4 (3 commits) 2019-08-15 [email protected] Remove the output directory prefix from the Android engine JAR filename (flutter/engine#11015) 2019-08-15 [email protected] Fix flutter/flutter #34791 (flutter/engine#9977) 2019-08-15 [email protected] Roll src/third_party/dart e35e8833ee..cae08c6813 (28 commits) 2019-08-15 [email protected] Roll src/third_party/skia f3f50099533d..e30a485a68c9 (2 commits) (flutter/engine#11022) 2019-08-15 [email protected] Roll src/third_party/skia 319fd3d7bcb4..f3f50099533d (4 commits) (flutter/engine#11021) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

Fix first frame logic

b7dadb9

googlebot added the cla: yes label Aug 15, 2019

jmagman suggested changes Aug 15, 2019

View reviewed changes

jmagman requested review from jmagman and removed request for jmagman August 15, 2019 17:56

dnfield added 3 commits August 15, 2019 11:24

retain

1aaf97a

format

c5f79ef

oops

2a07923

stuartmorgan-g suggested changes Aug 15, 2019

View reviewed changes

feedback, further simplification

a7df92e

til

49c92b3

stuartmorgan-g approved these changes Aug 15, 2019

View reviewed changes

jmagman approved these changes Aug 15, 2019

View reviewed changes

dnfield merged commit f062ac4 into flutter:master Aug 15, 2019

dnfield deleted the fix_first_frame_ios branch August 15, 2019 21:38

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 16, 2019

f062ac4 Fix first frame logic (flutter/engine#11027)

0bd4276

engine-flutter-autoroll mentioned this pull request Aug 16, 2019

Roll engine f8e7453f1106..4b7a552b9eae (24 commits) flutter/flutter#38663

Merged

Fix first frame logic #11027

Fix first frame logic #11027

Uh oh!

Conversation

dnfield commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Aug 15, 2019

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Aug 15, 2019

Uh oh!

dnfield commented Aug 15, 2019

Uh oh!

dnfield commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Aug 15, 2019 •

edited

Loading

stuartmorgan-g Aug 15, 2019 •

edited

Loading

stuartmorgan-g Aug 15, 2019 •

edited

Loading

dnfield commented Aug 15, 2019 •

edited

Loading