[macOS,iOS] Expose channel buffers 'resize' and 'overflow' control co…

[bleroux]

Description

This PR update the helper function that invokes the control channel 'resize' command (previous implementation relied on a deprecated format). It also adds a similar helper function for the 'overflow' commands exposed by the control channel.

See:

engine/lib/ui/channel_buffers.dart

Lines 302 to 309 in 93e8901

	/// ## The control channel
	///
	/// A plugin can configure its channel's buffers by sending messages to the
	/// control channel, `dev.flutter/channel-buffers` (see [kControlChannelName]).
	///
	/// There are two messages that can be sent to this control channel, to adjust
	/// the buffer size and to disable the overflow warnings. See [handleMessage]
	/// for details on these messages.

Related Issue

iOS and macOS implementation for flutter/flutter#132386

Similar implementations:

• Android: [Android] Expose channel buffer resize and overflow calls #44434
• Linux: [Linux] Expose channel buffers 'resize' and 'overflow' control commands #44636

Tests

Adds two tests.

[cbracken]

Just a couple quick suggestions.

[cbracken]

Alternatively:

NSArray* args = @[ channel, @(newSize) ];

@stuartmorgan do you know if we've formalised any preference here? I tend to use the shorter syntax, but I see both in the repo. Neither the Google nor the Chromium style guides appear to mention this on a brief scan.

[stuartmorgan-g]

IIRC the Google style guide used to mention it; most likely it was culled under https://google.github.io/styleguide/objcguide.html#style-rules-should-pull-their-weight because there's no reason not to use the modern literal syntax.

Boxing with NSNumber should definitely use modern boxing syntax.

[bleroux]

When switching to the modern boxing syntax, for this particular code, it does not produce the same result at runtime.
As you can see in the change I pushed, the test is failing because the serialization result is not the same (and the result is wrong with the modern boxing syntax). I don't have enough ObjectiveC knowledge to understand why.

Test failure: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8770883490087443713/+/u/test:_Tests_for_ios_debug_sim_arm64_extension_safe__3_/stdout

Failure message:

error: -[FlutterChannelsTest testResize] : OCProtocolMockObject(FlutterBinaryMessenger): unexpected method invoked: sendOnChannel:@"dev.flutter/channel-buffers" message:{length = 33, bytes = 0x07067265 73697a65 0c02070c 666c7574 ... 03000000 00000000 } 
expected:	sendOnChannel:@"dev.flutter/channel-buffers" message:{length = 29, bytes = 0x07067265 73697a65 0c02070c 666c7574 ... 65737403 03000000 } (NSInternalInconsistencyException)

[stuartmorgan-g]

The failing test says that the expected value was created with Dart code; why? It's not clear to me why you are asserting a specific representation here that comes from a different implementation with different integer semantics.

[bleroux]

The encoded message is deserialized in the Dart side, the deserialization code is there:

engine/lib/ui/channel_buffers.dart

Lines 459 to 537 in 93e8901

	void handleMessage(ByteData data) {
	// We hard-code the deserialization here because the StandardMethodCodec class
	// is part of the framework, not dart:ui.
	final Uint8List bytes = data.buffer.asUint8List(data.offsetInBytes, data.lengthInBytes);
	if (bytes[0] == 0x07) { // 7 = value code for string
	final int methodNameLength = bytes[1];
	if (methodNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception('Unrecognized message sent to $kControlChannelName (method name too long)');
	}
	int index = 2; // where we are in reading the bytes
	final String methodName = utf8.decode(bytes.sublist(index, index + methodNameLength));
	index += methodNameLength;
	switch (methodName) {
	case 'resize':
	if (bytes[index] != 0x0C) { // 12 = value code for list
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (arguments must be a two-element list, channel name and new capacity)");
	}
	index += 1;
	if (bytes[index] < 0x02) { // We ignore extra arguments, in case we need to support them in the future, hence <2 rather than !=2.
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (arguments must be a two-element list, channel name and new capacity)");
	}
	index += 1;
	if (bytes[index] != 0x07) { // 7 = value code for string
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (first argument must be a string)");
	}
	index += 1;
	final int channelNameLength = bytes[index];
	if (channelNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (channel name must be less than 254 characters long)");
	}
	index += 1;
	final String channelName = utf8.decode(bytes.sublist(index, index + channelNameLength));
	if (channelName.contains('\u0000')) {
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (channel name must not contain any null bytes)");
	}
	index += channelNameLength;
	if (bytes[index] != 0x03) { // 3 = value code for uint32
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (second argument must be an integer in the range 0 to 2147483647)");
	}
	index += 1;
	resize(channelName, data.getUint32(index, Endian.host));
	case 'overflow':
	if (bytes[index] != 0x0C) { // 12 = value code for list
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (arguments must be a two-element list, channel name and flag state)");
	}
	index += 1;
	if (bytes[index] < 0x02) { // We ignore extra arguments, in case we need to support them in the future, hence <2 rather than !=2.
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (arguments must be a two-element list, channel name and flag state)");
	}
	index += 1;
	if (bytes[index] != 0x07) { // 7 = value code for string
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (first argument must be a string)");
	}
	index += 1;
	final int channelNameLength = bytes[index];
	if (channelNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (channel name must be less than 254 characters long)");
	}
	index += 1;
	final String channelName = utf8.decode(bytes.sublist(index, index + channelNameLength));
	index += channelNameLength;
	if (bytes[index] != 0x01 && bytes[index] != 0x02) { // 1 = value code for true, 2 = value code for false
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (second argument must be a boolean)");
	}
	allowOverflow(channelName, bytes[index] == 0x01);
	default:
	throw Exception("Unrecognized method '$methodName' sent to $kControlChannelName");
	}
	} else {
	final List<String> parts = utf8.decode(bytes).split('\r');
	if (parts.length == 1 + /*arity=*/2 && parts[0] == 'resize') {
	resize(parts[1], int.parse(parts[2]));
	} else {
	// If the message couldn't be decoded as UTF-8, a FormatException will
	// have been thrown by utf8.decode() above.
	throw Exception('Unrecognized message $parts sent to $kControlChannelName.');
	}
	}
	}

The test checks that the message that will be send to Dart as an array of bytes is exactly the expected one.
With the old boxing syntax (NSArray* args = @[ channel, [NSNumber numberWithInt:newSize] ];), calling encodeMethodCall returns the exact byte array.

The problem is not specific to the test, when running a Flutter app with a modified engine and a call to the resize API, the Dart side throws the following exception:

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: Exception: Invalid arguments for 'resize' method sent to dev.flutter/channel-buffers (second argument must be an integer in the range 0 to 2147483647)
#0      ChannelBuffers.handleMessage (dart:ui/channel_buffers.dart:496:13)
#1      PlatformDispatcher._dispatchPlatformMessage (dart:ui/platform_dispatcher.dart:674:24)
#2      _dispatchPlatformMessage (dart:ui/hooks.dart:257:31)

Based on the Dart exception, I figured out that the modern boxing syntax is not treating the NSInteger as an int32 on 64bits platforms. I updated the PR with a cast to int and the encoded message is ok now.

[stuartmorgan-g]

engine/lib/ui/channel_buffers.dart

Lines 459 to 537 in 93e8901

	void handleMessage(ByteData data) {
	// We hard-code the deserialization here because the StandardMethodCodec class
	// is part of the framework, not dart:ui.
	final Uint8List bytes = data.buffer.asUint8List(data.offsetInBytes, data.lengthInBytes);
	if (bytes[0] == 0x07) { // 7 = value code for string
	final int methodNameLength = bytes[1];
	if (methodNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception('Unrecognized message sent to $kControlChannelName (method name too long)');
	}
	int index = 2; // where we are in reading the bytes
	final String methodName = utf8.decode(bytes.sublist(index, index + methodNameLength));
	index += methodNameLength;
	switch (methodName) {
	case 'resize':
	if (bytes[index] != 0x0C) { // 12 = value code for list
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (arguments must be a two-element list, channel name and new capacity)");
	}
	index += 1;
	if (bytes[index] < 0x02) { // We ignore extra arguments, in case we need to support them in the future, hence <2 rather than !=2.
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (arguments must be a two-element list, channel name and new capacity)");
	}
	index += 1;
	if (bytes[index] != 0x07) { // 7 = value code for string
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (first argument must be a string)");
	}
	index += 1;
	final int channelNameLength = bytes[index];
	if (channelNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (channel name must be less than 254 characters long)");
	}
	index += 1;
	final String channelName = utf8.decode(bytes.sublist(index, index + channelNameLength));
	if (channelName.contains('\u0000')) {
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (channel name must not contain any null bytes)");
	}
	index += channelNameLength;
	if (bytes[index] != 0x03) { // 3 = value code for uint32
	throw Exception("Invalid arguments for 'resize' method sent to $kControlChannelName (second argument must be an integer in the range 0 to 2147483647)");
	}
	index += 1;
	resize(channelName, data.getUint32(index, Endian.host));
	case 'overflow':
	if (bytes[index] != 0x0C) { // 12 = value code for list
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (arguments must be a two-element list, channel name and flag state)");
	}
	index += 1;
	if (bytes[index] < 0x02) { // We ignore extra arguments, in case we need to support them in the future, hence <2 rather than !=2.
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (arguments must be a two-element list, channel name and flag state)");
	}
	index += 1;
	if (bytes[index] != 0x07) { // 7 = value code for string
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (first argument must be a string)");
	}
	index += 1;
	final int channelNameLength = bytes[index];
	if (channelNameLength >= 254) { // lengths greater than 253 have more elaborate encoding
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (channel name must be less than 254 characters long)");
	}
	index += 1;
	final String channelName = utf8.decode(bytes.sublist(index, index + channelNameLength));
	index += channelNameLength;
	if (bytes[index] != 0x01 && bytes[index] != 0x02) { // 1 = value code for true, 2 = value code for false
	throw Exception("Invalid arguments for 'overflow' method sent to $kControlChannelName (second argument must be a boolean)");
	}
	allowOverflow(channelName, bytes[index] == 0x01);
	default:
	throw Exception("Unrecognized method '$methodName' sent to $kControlChannelName");
	}
	} else {
	final List<String> parts = utf8.decode(bytes).split('\r');
	if (parts.length == 1 + /*arity=*/2 && parts[0] == 'resize') {
	resize(parts[1], int.parse(parts[2]));
	} else {
	// If the message couldn't be decoded as UTF-8, a FormatException will
	// have been thrown by utf8.decode() above.
	throw Exception('Unrecognized message $parts sent to $kControlChannelName.');
	}
	}
	}

That's unfortunate. Please put a comment on the cast that explains, and links to this code, because it's extremely unusual for channel code to have a requirement on int size. The normal encoder gracefully handles 32 and 64-bit values.

Based on the Dart exception, I figured out that the modern boxing syntax is not treating the NSInteger as an int32 on 64bits platforms.

That's correct, since NSInteger is in fact 64-bit. Which should be fine, and only isn't because of unusual code at the other end of the channel.

[bleroux]

I have updated the PR with a comment on the cast.

[stuartmorgan-g]

(Wrong button, sorry; that was meant to be a "request changes")

[stuartmorgan-g]

LGTM with nits

[stuartmorgan-g]

Nit: (BOOL)warns

[stuartmorgan-g]

(BOOL)warns

[stuartmorgan-g]

@(static_cast<int>(newSize))

(We should never use C-style casts in a .mm file as they are strictly worse than C++-style casts.)

[stuartmorgan-g]

warns

[stuartmorgan-g]

Why not just inline this into the call?