[iOS] Fixes SplashScreenView crash when remove from view hierarchy by zhongwuzw · Pull Request #34496 · flutter/engine

zhongwuzw · 2022-07-07T12:11:08Z

Fixes flutter/flutter#37818 , flutter/flutter#105423. There are some cases that we remove the splashScreenView from superview before add it to view hierarchy.

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.

jmagman · 2022-07-13T20:46:24Z

cc @cyanglaz

cyanglaz

LGTM % nits.

@gaaclarke Do you mind taking a secondary review?

cyanglaz · 2022-07-13T21:10:44Z

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

+  UIView* nilView = nil;
+  [flutterViewController setSplashScreenView:nilView];


Suggested change

UIView* nilView = nil;

[flutterViewController setSplashScreenView:nilView];

[flutterViewController setSplashScreenView:nil];

@cyanglaz Emm, setSplashScreenView:nil would build error because splashScreenView declare nonnull and we treat build warning as error. Actually we have the case that let user pass the nil to remove the splash screen view, see

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

Lines 630 to 637 in 257b05f

- (void)setSplashScreenView:(UIView*)view {

if (!view) {

// Special case: user wants to remove the splash screen view.

if (_splashScreenView) {

[self removeSplashScreenView:nil];

}

return;

}

.

Isn't this wrong that we make the view paramter nonnull and still checking nullability in the implementation? Maybe we should update the method syntax to make the parameter nonnull.

It looks like setSplashScreenView should allow nil #6834, probably an oversight from adding NS_ASSUME_NONNULL_BEGIN. Can you instead make the API nullable?

- @property(strong, nonatomic) UIView* splashScreenView; + @property(strong, nonatomic, nullable) UIView* splashScreenView;

and add a comment to the header like:

* Set to nil to remove the splash screen view.

@cyanglaz We support view nullable to remove the view from view hierarchy, but we use NS_ASSUME_NONNULL_BEGIN to declare view nonnull.

@jmagman We can make the API nullable, but it may lead to an API-breaking change for Swift users, if Swift users use the getter of splashScreenView, the return type changed from UIView to UIView?.
But I agree that we should make it nullable, should we need to make a new PR as a breaking change to add nullable keyword?

If we don't make this API nullable, it means this feature is not supported on swift, right?
So I think we should introduce the swift breaking change and make the API nullable.

If we don't make this API nullable, it means this feature is not supported on swift, right? So I think we should introduce the swift breaking change and make the API nullable.

@cyanglaz I create another PR to introduce API breaking change. Please review it #34743.

#34743 has merged, apologies for that taking so long. Can you rebase on your change and make this nil?

@jmagman Done. :)

cyanglaz

LGTM

jmagman

LGTM

…archy (flutter/engine#34496)

…lutter#34496)

flutter-dashboard bot added the platform-ios label Jul 7, 2022

zhongwuzw requested review from chinmaygarde and gaaclarke July 7, 2022 13:03

cyanglaz self-requested a review July 13, 2022 21:12

cyanglaz reviewed Jul 13, 2022

View reviewed changes

cyanglaz mentioned this pull request Jul 19, 2022

[iOS API breaking change] Make splashScreenView of FlutterViewController nullable #34743

Merged

8 tasks

zhongwuzw added 2 commits July 28, 2022 10:36

[iOS] Fixes SplashScreenView crash when remove from view hierarchy

be3fa16

Set splashScreenView nil directly

57a4ea3

zhongwuzw force-pushed the fix_splash_view_crash branch from d4bf336 to 57a4ea3 Compare July 28, 2022 02:39

zhongwuzw requested a review from jmagman July 28, 2022 06:54

cyanglaz approved these changes Jul 28, 2022

View reviewed changes

jmagman approved these changes Jul 28, 2022

View reviewed changes

zhongwuzw added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 29, 2022

auto-submit bot merged commit 260d979 into flutter:main Jul 29, 2022

fzyzcjy mentioned this pull request Jul 29, 2022

sometimes see Lost connection to device. in worker, when hot restarting it, related to Flutter's UIViewAnimationBlockDelegate fzyzcjy/flutter_convenient_test#188

Closed

engine-flutter-autoroll mentioned this pull request Jul 29, 2022

Roll Flutter Engine from 011322326113 to 260d9797263b (1 revision) flutter/flutter#108592

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2022

260d979 [iOS] Fixes SplashScreenView crash when remove from view hier…

158ba68

…archy (flutter/engine#34496)

fzyzcjy mentioned this pull request Aug 1, 2022

Add one liner documentation how to actually use CI mode fzyzcjy/flutter_convenient_test#240

Closed

emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022

[iOS] Fixes SplashScreenView crash when remove from view hierarchy (f…

e4b2b3d

…lutter#34496)

		UIView* nilView = nil;
		[flutterViewController setSplashScreenView:nilView];

	- (void)setSplashScreenView:(UIView*)view {
	if (!view) {
	// Special case: user wants to remove the splash screen view.
	if (_splashScreenView) {
	[self removeSplashScreenView:nil];
	}
	return;
	}

Conversation

zhongwuzw commented Jul 7, 2022

Pre-launch Checklist

Uh oh!

jmagman commented Jul 13, 2022

Uh oh!

cyanglaz 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!

zhongwuzw Jul 16, 2022 • 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!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhongwuzw Jul 16, 2022 •

edited

Loading