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

Started asserting the FlutterEngine is running before communicating over channels. #12469

Merged
gaaclarke merged 5 commits into from
Sep 27, 2019
Merged

Started asserting the FlutterEngine is running before communicating over channels. #12469

gaaclarke merged 5 commits into from
Sep 27, 2019

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Sep 26, 2019

This changes a null pointer exception to an NSException that will provide some meaningful data to clients incorrectly using the engine in an add-to-app situations.

@gaaclarke
Started asserting the FlutterEngine is running before communicating
3464a50 
over channels.  This changes a null pointer exception to an
NSException that will provide some meaningful data to clients
incorrectly using the engine in an add-to-app situations.
@gaaclarke gaaclarke requested a review from jmagman September 26, 2019 23:00
jmagman
jmagman reviewed Sep 26, 2019
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Outdated
message:(NSData*)message
binaryReply:(FlutterBinaryReply)callback {
NSAssert(channel, @"The channel must not be null");
NSAssert(channel, @"The channel must not be nil");
Copy link
Member

@jmagman jmagman Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

NSParameterAssert(channel);

Copy link
Member Author

@gaaclarke gaaclarke Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
NSAssert(channel, @"The channel must not be null");
NSAssert(channel, @"The channel must not be nil");
NSAssert(_shell && _shell->IsSetup(),
@"Sending a message before the FlutterEngine has been run.");
Copy link
Member

@jmagman jmagman Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we tell them how to run it?

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was careful to use the word "run" to match the verb in our run methods. It's a bit complicated, should I say something like "See run* methods"?

Copy link
Member

@jmagman jmagman Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh it's okay, it would make sense to list it if there was maybe only one way to run it. What you have is fine.

jmagman
jmagman reviewed Sep 26, 2019
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add unit tests?

@gaaclarke
Copy link
Member Author

gaaclarke commented Sep 27, 2019 •
edited
Loading

Can we add unit tests?

@jmagman Done

edit: I couldn't add the positive case since that would require actually having a module to run.

jmagman
jmagman approved these changes Sep 27, 2019
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm Outdated

- (void)testSendMessageBeforeRun {
id project = OCMClassMock([FlutterDartProject class]);
FlutterEngine* engine = [[[FlutterEngine alloc] initWithName:@"foobar"
Copy link
Member

@jmagman jmagman Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh the tests aren't arc either?

Copy link
Member Author

@gaaclarke gaaclarke Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been turning it on file by file, turned it on for this one.

jmagman
jmagman reviewed Sep 27, 2019
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterEngineTest.mm
#endif

#if !__has_feature(objc_arc)
#error ARC must be enabled!
Copy link
Member

@jmagman jmagman Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what non-clang compilers are compiling these test files?

Copy link
Member Author

@gaaclarke gaaclarke Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None today that I know of. If we accidentally used GCC it should probably be an error. I'm just being cautious. Ideally this would be a macro too, I just haven't gotten around to make it one. It's not straightforward to get a macro of macros, I'd have to spend some time looking things up and fiddling with it.

@gaaclarke gaaclarke merged commit 3c271bb into flutter:master Sep 27, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll
3c271bb Started asserting the FlutterEngine is running before communi…
a939658 
…cating over channels. (flutter/engine#12469)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 30, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll
Roll engine 5b952f2..f0c7740 (47 commits) (#41622)
bc60c94 
git@github.com:flutter/engine.git/compare/5b952f286fc0...f0c7740

git log 5b952f2..f0c7740 --no-merges --oneline
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia e242d0ea937c..89e1f600ec78 (3 commits) (flutter/engine#12696)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ncc-K... to KH7Tv... (flutter/engine#12694)
2019-09-30 50856934+nturgut@users.noreply.github.com Refactoring text_editing.dart (flutter/engine#12479)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia abc851eb3dcc..e242d0ea937c (2 commits) (flutter/engine#12693)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0e7500021402..abc851eb3dcc (3 commits) (flutter/engine#12692)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7aeabcfa6a73..0e7500021402 (1 commits) (flutter/engine#12691)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from eUBos... to sWs8N... (flutter/engine#12690)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from VACNQ... to ncc-K... (flutter/engine#12689)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from OEG20... to eUBos... (flutter/engine#12688)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6fMRC... to VACNQ... (flutter/engine#12687)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia be971636cf9b..7aeabcfa6a73 (3 commits) (flutter/engine#12686)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2e856451654d..be971636cf9b (1 commits) (flutter/engine#12685)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lHiKM... to OEG20... (flutter/engine#12683)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3oH6... to 6fMRC... (flutter/engine#12682)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nVAcH... to lHiKM... (flutter/engine#12680)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Kiq-H... to z3oH6... (flutter/engine#12679)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia bd84330ba277..2e856451654d (1 commits) (flutter/engine#12661)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8ab1530cd3cd..bd84330ba277 (1 commits) (flutter/engine#12639)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from qx53U... to nVAcH... (flutter/engine#12620)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from oob8T... to Kiq-H... (flutter/engine#12616)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28ad6f869822..8ab1530cd3cd (5 commits) (flutter/engine#12614)
2019-09-27 liyuqian@google.com Revert "[fuchsia] Wire up OpacityLayer to Scenic (#11322)" (flutter/engine#12610)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Started asserting the FlutterEngine is running before communicating over channels. (flutter/engine#12469)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Split out the logic to handle status bar touches into its own function (flutter/engine#12587)
2019-09-27 bkonyi@google.com Roll src/third_party/dart c3c31b74fd..132bee48d0 (16 commits)
2019-09-27 iska.kaushik@gmail.com [flutter_runner] Refactor thread_application pair to ActiveApplication (flutter/engine#12573)
2019-09-27 gw280@google.com Reword confusing messaging surrounding unhandled exception in flutter_runner on Fuchsia (flutter/engine#12428)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23d66e10a98..28ad6f869822 (10 commits) (flutter/engine#12580)
2019-09-27 iska.kaushik@gmail.com Revert "Update linux toolchain for fuchsia" (flutter/engine#12578)
2019-09-27 jonahwilliams@google.com Add support for JIT release mode (flutter/engine#12446)
2019-09-27 iska.kaushik@gmail.com Update linux toolchain for fuchsia (flutter/engine#12572)
2019-09-27 iska.kaushik@gmail.com Remove references to topaz (flutter/engine#12565)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 7901c508c8..c3c31b74fd (4 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3787f51c65c3..b23d66e10a98 (1 commits) (flutter/engine#12559)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from HoRV8... to oob8T... (flutter/engine#12538)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from YDv3O... to qx53U... (flutter/engine#12520)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 296743a86281..3787f51c65c3 (4 commits) (flutter/engine#12512)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 403c4af720..7901c508c8 (7 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b83cc76a5e3a..296743a86281 (1 commits) (flutter/engine#12491)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 327bc451f8..403c4af720 (9 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from HfPKR... to zpVtV... (flutter/engine#12473)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 5b952f2..f0c7740 (47 commits) (flutter#41622)
d62a9f4 
git@github.com:flutter/engine.git/compare/5b952f286fc0...f0c7740

git log 5b952f2..f0c7740 --no-merges --oneline
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia e242d0ea937c..89e1f600ec78 (3 commits) (flutter/engine#12696)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from ncc-K... to KH7Tv... (flutter/engine#12694)
2019-09-30 50856934+nturgut@users.noreply.github.com Refactoring text_editing.dart (flutter/engine#12479)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia abc851eb3dcc..e242d0ea937c (2 commits) (flutter/engine#12693)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 0e7500021402..abc851eb3dcc (3 commits) (flutter/engine#12692)
2019-09-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia 7aeabcfa6a73..0e7500021402 (1 commits) (flutter/engine#12691)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from eUBos... to sWs8N... (flutter/engine#12690)
2019-09-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from VACNQ... to ncc-K... (flutter/engine#12689)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from OEG20... to eUBos... (flutter/engine#12688)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 6fMRC... to VACNQ... (flutter/engine#12687)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia be971636cf9b..7aeabcfa6a73 (3 commits) (flutter/engine#12686)
2019-09-29 skia-flutter-autoroll@skia.org Roll src/third_party/skia 2e856451654d..be971636cf9b (1 commits) (flutter/engine#12685)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from lHiKM... to OEG20... (flutter/engine#12683)
2019-09-29 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from z3oH6... to 6fMRC... (flutter/engine#12682)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nVAcH... to lHiKM... (flutter/engine#12680)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from Kiq-H... to z3oH6... (flutter/engine#12679)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia bd84330ba277..2e856451654d (1 commits) (flutter/engine#12661)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8ab1530cd3cd..bd84330ba277 (1 commits) (flutter/engine#12639)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from qx53U... to nVAcH... (flutter/engine#12620)
2019-09-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from oob8T... to Kiq-H... (flutter/engine#12616)
2019-09-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28ad6f869822..8ab1530cd3cd (5 commits) (flutter/engine#12614)
2019-09-27 liyuqian@google.com Revert "[fuchsia] Wire up OpacityLayer to Scenic (flutter#11322)" (flutter/engine#12610)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Started asserting the FlutterEngine is running before communicating over channels. (flutter/engine#12469)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Split out the logic to handle status bar touches into its own function (flutter/engine#12587)
2019-09-27 bkonyi@google.com Roll src/third_party/dart c3c31b74fd..132bee48d0 (16 commits)
2019-09-27 iska.kaushik@gmail.com [flutter_runner] Refactor thread_application pair to ActiveApplication (flutter/engine#12573)
2019-09-27 gw280@google.com Reword confusing messaging surrounding unhandled exception in flutter_runner on Fuchsia (flutter/engine#12428)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23d66e10a98..28ad6f869822 (10 commits) (flutter/engine#12580)
2019-09-27 iska.kaushik@gmail.com Revert "Update linux toolchain for fuchsia" (flutter/engine#12578)
2019-09-27 jonahwilliams@google.com Add support for JIT release mode (flutter/engine#12446)
2019-09-27 iska.kaushik@gmail.com Update linux toolchain for fuchsia (flutter/engine#12572)
2019-09-27 iska.kaushik@gmail.com Remove references to topaz (flutter/engine#12565)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 7901c508c8..c3c31b74fd (4 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 3787f51c65c3..b23d66e10a98 (1 commits) (flutter/engine#12559)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from HoRV8... to oob8T... (flutter/engine#12538)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from YDv3O... to qx53U... (flutter/engine#12520)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia 296743a86281..3787f51c65c3 (4 commits) (flutter/engine#12512)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 403c4af720..7901c508c8 (7 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia b83cc76a5e3a..296743a86281 (1 commits) (flutter/engine#12491)
2019-09-27 bkonyi@google.com Roll src/third_party/dart 327bc451f8..403c4af720 (9 commits)
2019-09-27 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from HfPKR... to zpVtV... (flutter/engine#12473)
2019-09-27 skia-flutter-autoroll@skia.org Roll src/third_party/skia ec85f407bfee..b83cc76a5e3a (15 commits) (flutter/engine#12472)
2019-09-27 30870216+gaaclarke@users.noreply.github.com Added a default entrypoint variable to match android syntax. (flutter/engine#12370)
2019-09-26 tauu@h2overclock.de [web_ui] add missing dispose handler for MethodCalls to flutter/platform_view (flutter/engine#12226)
2019-09-26 tauu@h2overclock.de [web_ui] PersistedPlatformView attribute update handling to enable resizing (flutter/engine#12227)
...
@jmagman jmagman mentioned this pull request Jan 9, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jmagman jmagman jmagman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gaaclarke @jmagman @googlebot