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

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Mar 24, 2021

fixes flutter/flutter#79011

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.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke force-pushed the plugin-lifecycle-async-tests branch 2 times, most recently from 7c01b59 to 4394e2d Compare March 24, 2021 23:39
@gaaclarke gaaclarke force-pushed the plugin-lifecycle-async-tests branch from 4394e2d to 8d74fd4 Compare March 24, 2021 23:40
@gaaclarke gaaclarke changed the title Started waiting for the notifications locally before asserting they Started waiting for the notifications locally before asserting side effects Mar 24, 2021
@gaaclarke gaaclarke marked this pull request as ready for review March 24, 2021 23:41
@gaaclarke gaaclarke requested a review from jmagman March 24, 2021 23:41
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!

@gaaclarke gaaclarke added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 24, 2021
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 25, 2021
@jmagman
Copy link
Member

jmagman commented Mar 25, 2021

Guess that didn't work...

FlutterViewController.mm:613: error: -[FlutterPluginAppLifeCycleDelegateTest testWillResignActive] : failed: caught "NSInternalInconsistencyException", 
"** Cannot use mock object OCClassMockObject(FlutterEngine) at 0x6000002144b0. This error usually occurs when a mock object is used after stopMocking has been called on it. In most cases it is not necessary to call stopMocking. If you know you have to, please make sure that the mock object is not used afterwards."

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8851835021381396112/+/steps/Host_Tests_for_ios_debug_sim/0/stdout

[[NSNotificationCenter defaultCenter]
postNotificationName:UIApplicationWillResignActiveNotification
object:nil];
[self waitForExpectations:@[ expectation ] timeout:5.0];
Copy link
Member

@jmagman jmagman Mar 25, 2021

Choose a reason for hiding this comment

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

It's failing on the -[NSNotificationCenterpostNotificationName::] line before it even gets here.

	0   CoreFoundation                      0x000000010e14de6e __exceptionPreprocess + 350
	1   libobjc.A.dylib                     0x000000010d5b19b2 objc_exception_throw + 48
	2   CoreFoundation                      0x000000010e14dcac +[NSException raise:format:] + 188
	3   libocmock_shared.dylib              0x0000000126ec2448 -[OCMockObject handleInvocation:] + 39
	4   libocmock_shared.dylib              0x0000000126ec2327 -[OCMockObject forwardInvocation:] + 30
	5   CoreFoundation                      0x000000010e152616 ___forwarding___ + 838
	6   CoreFoundation                      0x000000010e154c58 __forwarding_prep_1___ + 120
	7   libios_test_flutter.dylib           0x0000000126f4aa5d -[FlutterViewController surfaceUpdated:] + 187
	8   libios_test_flutter.dylib           0x0000000126f4b59d -[FlutterViewController applicationWillResignActive:] + 46
	9   CoreFoundation                      0x000000010e078d2c __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
	10  CoreFoundation                      0x000000010e0781a5 _CFXRegistrationPost1 + 421
	11  CoreFoundation                      0x000000010e077f11 ___CFXNotificationPost_block_invoke + 193
	12  CoreFoundation                      0x000000010e175473 -[_CFXNotificationRegistrar find:object:observer:enumerator:] + 1795
	13  CoreFoundation                      0x000000010e077866 _CFXNotificationPost + 950
	14  Foundation                          0x000000010cff426b -[NSNotificationCenter postNotificationName:object:userInfo:] + 59
	15  libios_test_flutter.dylib           0x0000000126ef9932 -[FlutterPluginAppLifeCycleDelegateTest testWillResignActive] + 200

Cannot use mock object OCClassMockObject(FlutterEngine)
Where even is that FlutterEngine mock even coming from? I see a OCMClassMock([FlutterEngine class]) set up in FlutterTextInputPluginTest and FlutterViewControllerTest (the latter is my guess) but not this test. Maybe the view controllers in those tests are still alive after -tearDown and are trying to notify their dead engines? Is there a retain cycle somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, it's a leak from another test somewhere. I'm guessing there is a stray mock engine that is getting talked to when the notification is sent (since the flutterengine also listens to those notifications). I gotta add a stopMocking somewhere, or maybe see where it isn't removing itself as an observer when it should be. The retain cycle might be created by not calling stopMocking and not necessarily a problem in the code. I'll see what I can find.

Copy link
Member Author

Choose a reason for hiding this comment

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

Found it, it was the partial mock in testCallsNotifyLowMemory. I added a few extra stopMocking calls for FlutterEngines.

Copy link
Member

@jmagman jmagman Mar 25, 2021

Choose a reason for hiding this comment

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

I'm not sure that's the problem, the error is complaining that stopMocking had already been called on the engine, but a FlutterViewController is trying to tell it about surfaceUpdated.

Cannot use mock object OCClassMockObject(FlutterEngine) at 0x6000002144b0. This error usually occurs when a mock object is used after stopMocking has been called on it.

The only two OCMClassMock([FlutterEngine class]) I see are in FlutterTextInputPluginTest and FlutterViewControllerTest. Are the FlutterViewControllers in FlutterViewControllerTest dealloced after -tearDown? It doesn't look like it should be since the self.mockEngine is nil'd out in -tearDown which should break the retain cycle, but maybe I'm missing something.

  FlutterViewController* viewControllerB =
      [[FlutterViewController alloc] initWithEngine:self.mockEngine nibName:nil bundle:nil];
...
OCMStub([self.mockEngine viewController]).andReturn(viewControllerB);

Might be easiest to check if the memory graph contains any FlutterViewControllers before FlutterPluginAppLifeCycleDelegateTest starts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen this problem because the mock object can cause retains on objects that aren't apparent, stopMocking flushes them out. That was the thesis behind the change. I saw those other tests that deleting the engine, that may be the problem. Let me try that out.

[self waitForExpectations:@[ backgroundExpectation ] timeout:5.0];

OCMVerify([mockEngine notifyLowMemory]);
[mockEngine stopMocking];
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix to the problem we had with the engine.

@gaaclarke gaaclarke added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 25, 2021
@fluttergithubbot fluttergithubbot merged commit 8726c78 into flutter:master Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Mar 25, 2021
gaaclarke added a commit that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2021
chandarrengoog pushed a commit to chandarrengoog/engine that referenced this pull request Mar 30, 2021
chandarrengoog pushed a commit to chandarrengoog/engine that referenced this pull request Mar 30, 2021
duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021
duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ios unit tests that post application notifications are flakey

4 participants