iOS: Improve thread safety of first frame callback

[cbracken]

Previously, we dispatched a task to a background thread to wait for the first frame to be ready. In the background task, we then throw a task on the main thread that invokes that user-provided callback to notify them that the first frame is ready.

During ARC migration, we ensured that self was strongly-captured in the task that runs on the main thread, in order to prevent the possibility that the last remaining reference to the FlutterEngine instance be the one on the background thread.

However, with the previous code, it's possible that the callback is dispatched to the main thread, executes, and completes before the block on the background thread finishes. In that case, the last remaining FlutterEngine reference may still be the one on the background thread.

In order to prevent this scenario, we use dispatch_group_notify to ensure that the tasks are executed sequentially, and that the callback task is not dispatched to the main thread until the background task has completed. Further, we capture FlutterEngine strongly in the second task to ensure that it is the last remaining task.

Why do we need to capture self strongly in the task fired to the main queue? Imagine the following:

• We queue up the first task on the background thread.
• We queue up the callback task on the main thread to be executed only after the background task completes.
• The task on the background thread starts running.
• That task captures weakSelf strongly and checks it -- it's not nil, yay.
• That task now makes the blocking first frame call. Time passes. While that time passes all other strong references to self go away. That second block only captured self weakly, so the background task now has the only remaining strong reference. Uh oh!
• We hit the end of the background task block and since we have the last remaining strong reference, we dealloc FlutterEngine... on the background thread.
• KABOOM.

No changes to tests since this is a fix to a race condition that doesn't affect app semantics, isn't testable as written, and adding shims to improve testability will likely make some already subtle/difficult-to-reason-about code even more complex and difficult to reason about.

Issue: flutter/flutter#156177

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 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.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[stuartmorgan-g]

However, with the previous code, it's possible that the callback is
dispatched to the main thread, executes, and completes before the block
on the background thread finishes.

Great catch, that's a subtle one. This is one of those things I know in theory, but it's really hard to remember when reading code that "ends" with the thread hop back.

[stuartmorgan-g]

test-exempt: there's no way to force the potential race to happen, so all we can feasibly do AFAICT is to treat this as fixing potential flake in existing tests.

[hellohuanlin]

However, with the previous code, it's possible that the callback is
dispatched to the main thread, executes, and completes before the block
on the background thread finishes.

Isn't strongSelf.shell.WaitForFirstFrame(waitTime) blocking (based on its name, i didn't read the code to be honest)? I'd expect the background thread to be done with the call before hopping to the main thread.

[hellohuanlin]

Oh nvm i saw your comment in chat.