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

[Android] Fix BasicMessageChannel.resizeChannelBuffer #41982

Merged
auto-submit merged 2 commits into from
May 19, 2023
Merged

[Android] Fix BasicMessageChannel.resizeChannelBuffer #41982

auto-submit merged 2 commits into from
May 19, 2023

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented May 12, 2023

Description

This PR updates BasicMessageChannel.resizeChannelBuffer implementation. Previous implementation builds a wrong ByteBuffer and was not tested so it is difficult to know when it stopped working.

Related Issue

Fixes flutter/flutter#126632

Tests

Adds 1 test.

@bleroux bleroux mentioned this pull request May 12, 2023
Closed
2 tasks
@bleroux bleroux force-pushed the android_fix_resize_channel_buffer branch from 1f896f9 to 4639ff9 Compare May 12, 2023 18:33
@gmackall gmackall requested review from gaaclarke and gmackall and removed request for gmackall May 18, 2023 18:23
@gmackall
Copy link
Member

gmackall commented May 18, 2023

Note from android triage: @gaaclarke Do you know who might be a good reviewer for this?

@chinmaygarde
Copy link
Member

chinmaygarde commented May 18, 2023

I don't think this is a fix. We are just swallowing an exception.

dnfield
dnfield reviewed May 18, 2023
View reviewed changes
shell/platform/android/test/io/flutter/plugin/common/MethodChannelTest.java Outdated
Comment on lines 34 to 42
ByteBuffer packet = null;
try {
final String content = String.format("resize\r%s\r%d", "flutter/test", 3);
final byte[] bytes = content.getBytes("UTF-8");
packet = ByteBuffer.allocateDirect(bytes.length);
packet.put(bytes);
} catch (UnsupportedEncodingException e) {
throw new AssertionError("UTF-8 only");
}
Copy link
Contributor

@dnfield dnfield May 18, 2023

Choose a reason for hiding this comment

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

Why is this code in the test? It's not actually contributing to the test.

Copy link
Contributor Author

@bleroux bleroux May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The packet variable is used below, line 47.

Copy link
Contributor

@dnfield dnfield May 18, 2023

Choose a reason for hiding this comment

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

Ahh I see now.

dnfield
dnfield reviewed May 18, 2023
View reviewed changes
shell/platform/android/io/flutter/plugin/common/BasicMessageChannel.java Outdated
Comment on lines 153 to 155
} catch (UnsupportedEncodingException e) {
Log.e(TAG, "Failed to resize channel buffer named " + channel, e);
}
Copy link
Contributor

@dnfield dnfield May 18, 2023

Choose a reason for hiding this comment

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

This seems to be unrelated to the bug filed upstream and intended to support channel names that aren't valid as UTF-8 strings or something. Can it just be removed or perhaps serpated out into its own PR?

Copy link
Member

@jason-simmons jason-simmons May 18, 2023

Choose a reason for hiding this comment

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

The catch is necessary because String.getBytes(charset) is declared as throws UnsupportedEncodingException

(In practice the exception will not be thrown because UTF-8 is guaranteed to be supported)

Alternatively, this could continue using Charset.forName (which does not throw any checked exceptions)

@jason-simmons
Copy link
Member

jason-simmons commented May 18, 2023

I don't think this is a fix. We are just swallowing an exception.

The fix here is the use of ByteBuffer.allocateDirect instead of ByteBuffer.wrap.

(The current Android Java/JNI platform message implementation assumes that all buffers passed to native are direct buffers)

@bleroux
Copy link
Contributor Author

bleroux commented May 18, 2023 •
edited
Loading

I don't think this is a fix. We are just swallowing an exception.

@chinmaygarde The fix is not to swallow an exception, it is to replace the way the ByteBuffer is build.
Previously it relied on ByteBuffer.wrap and throws a RangeError, see flutter/flutter#126632 (comment).

The fix is to rely on ByteBuffer.allocateDirect.

Edited: Oops I just read Jason comment after posting mine 😅

@bleroux bleroux force-pushed the android_fix_resize_channel_buffer branch from 4639ff9 to 688b0f4 Compare May 18, 2023 21:50
@bleroux
Copy link
Contributor Author

bleroux commented May 18, 2023

I have updated the PR to use Charset.forName as suggested by Jason and it makes the PR simpler and less confusing. Thanks @jason-simmons!

@bleroux bleroux requested review from dnfield and jason-simmons May 19, 2023 00:10
@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label May 19, 2023
@auto-submit auto-submit bot merged commit 6bc60c8 into flutter:main May 19, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 19, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2023
@engine-flutter-autoroll
6bc60c8a9 [Android] Fix BasicMessageChannel.resizeChannelBuffer (flut…
9851401 
…ter/engine#41982)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 19, 2023
@engine-flutter-autoroll
Roll Flutter Engine from bca11a423f9c to 6bc60c8a9877 (1 revision) (#…
fcba066 
…127162)

flutter/engine@bca11a4...6bc60c8

2023-05-19 [email protected] [Android] Fix BasicMessageChannel.resizeChannelBuffer (flutter/engine#41982)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@bleroux bleroux deleted the android_fix_resize_channel_buffer branch May 19, 2023 14:08
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@engine-flutter-autoroll
Roll Flutter Engine from bca11a423f9c to 6bc60c8a9877 (1 revision) (f…
d6aa581 
…lutter#127162)

flutter/engine@bca11a4...6bc60c8

2023-05-19 [email protected] [Android] Fix BasicMessageChannel.resizeChannelBuffer (flutter/engine#41982)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@bleroux bleroux mentioned this pull request Jul 28, 2023
Closed
@Hixie
Copy link
Contributor

Hixie commented Jul 31, 2023

FWIW, the format used for the message in this PR is deprecated, ideally we would use the binary format instead. It should be slightly more efficient.

This was referenced Aug 7, 2023
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gaaclarke gaaclarke Awaiting requested review from gaaclarke

2 more reviewers

@dnfield dnfield dnfield approved these changes

@jason-simmons jason-simmons jason-simmons approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-android

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error with 'resizeChannelBuffer' call in MethodChannel on Android platform

6 participants

@bleroux @gmackall @chinmaygarde @jason-simmons @Hixie @dnfield