Fixed memory leak by way of accidental retain on implicit self

[gaaclarke]

Related issue: flutter/flutter#26007

One potential problem with this change is that it may break clients code because they might have been relying on the fact that there is a retain cycle that keeps channels alive instead of keeping them alive themselves. We even actively encourage not taking care of channels correctly like in the following doc: https://flutter.dev/docs/development/platform-integration/platform-channels#step-4a-add-an-ios-platform-specific-implementation-using-objective-c

After this fix there will be no need to fiddle with the channel in order to get the FlutterViewController to dealloc, as seen in this gist: https://gist.github.com/gaaclarke/aa1f2512bb41c0972e381d06f610c4a0#file-gistfile1-txt-L50

[dnfield]

This LGTM. When it lands in the engine, we should add a test to ios_add2app in the framework to catch it.

Alternatively, we should add functionality in the engine to run unit tests for this kind of thing on Objective C code (that's long been desired).

[gaaclarke]

We can't check this in as it currently stands yet because it will break plugins. Authors of plugins are assuming that channels won't get deleted. Here is example code from the connectivity plugin:

+ (void)registerWithRegistrar:(NSObject<FlutterPluginRegistrar>*)registrar {
  FLTConnectivityPlugin* instance = [[FLTConnectivityPlugin alloc] init];

  FlutterMethodChannel* channel =
      [FlutterMethodChannel methodChannelWithName:@"plugins.flutter.io/connectivity"
                                  binaryMessenger:[registrar messenger]];
  [registrar addMethodCallDelegate:instance channel:channel];

  FlutterEventChannel* streamChannel =
      [FlutterEventChannel eventChannelWithName:@"plugins.flutter.io/connectivity_status"
                                binaryMessenger:[registrar messenger]];
  [streamChannel setStreamHandler:instance];
}

[cbracken]

@gaaclarke is the plan to eventually land this or would you like to close it then land the larger API refactor you've proposed, which'll involve a breaking change?

[xster]

I think #9419 is the replacement of this

[gaaclarke]

After #9419 this is much lower priority. I'd like to commit this only with the larger change. I was leaving it out there until we addressed the other concerns. I didn't realize it was going to take so long. Do you want me to close it? Maybe I can just flag it WIP?

[gaaclarke]

I had a chat with Amir and it seems that this might work without further change. It's a bit unintuitive but you don't need a channel in order for communication to work, so the deleting of the channels might not be a big deal because the binary messenger will hold onto the necessary objects. This will require a bit more testing.

[xster]

Interesting. Can you describe what the needed linkage (that's still held after the channel's gone) is?

[gaaclarke]

The binary messenger holds on to all the required state by-way-of a lambda.

engine/shell/platform/darwin/common/framework/Source/FlutterChannels.mm

Line 222 in e4ad3a2

[_messenger setMessageHandlerOnChannel:_name binaryMessageHandler:messageHandler];

I don't like the way this is setup, I think it is confusing that you don't need a channel in order to communicate but that is also how it is setup on Android.

[gaaclarke]

I was able to test this and Amir was right. The channels don't need to be in memory to be communicated upon, just like in Android. This change should be safe.

[gaaclarke]

I had to change the original PR to make sure that the FlutterEventChannel will function beyond being deallocated like the other channels to match the functionality that Amir wants.

[amirh]

LGTM

[a-siva]

The engine buildbot turned red after this commit, any ideas?