[Android] Fix BasicMessageChannel.resizeChannelBuffer by bleroux · Pull Request #41982 · flutter/engine

bleroux · 2023-05-12T16:23:20Z

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.

gmackall · 2023-05-18T18:27:14Z

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

chinmaygarde · 2023-05-18T20:20:50Z

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

dnfield · 2023-05-18T20:24:07Z

shell/platform/android/test/io/flutter/plugin/common/MethodChannelTest.java

+    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");
+    }


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

The packet variable is used below, line 47.

Ahh I see now.

dnfield · 2023-05-18T20:27:28Z

shell/platform/android/io/flutter/plugin/common/BasicMessageChannel.java

+    } catch (UnsupportedEncodingException e) {
+      Log.e(TAG, "Failed to resize channel buffer named " + channel, e);
+    }


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?

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 · 2023-05-18T20:38:56Z

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 · 2023-05-18T20:44:51Z

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 · 2023-05-18T21:56:46Z

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!

…r/engine#41982)

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

) 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

Hixie · 2023-07-31T00:13:47Z

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.

flutter-dashboard bot added the platform-android label May 12, 2023

bleroux mentioned this pull request May 12, 2023

Error with 'resizeChannelBuffer' call in MethodChannel on Android platform flutter/flutter#126632

Closed

2 tasks

bleroux force-pushed the android_fix_resize_channel_buffer branch from 1f896f9 to 4639ff9 Compare May 12, 2023 18:33

gmackall requested review from gaaclarke and gmackall and removed request for gmackall May 18, 2023 18:23

dnfield reviewed May 18, 2023

View reviewed changes

bleroux added 2 commits May 18, 2023 23:49

[Android] Fix BasicMessageChannel.resizeChannelBuffer

cb3aa92

Use Charset.forName to avoid confusing try/catch

688b0f4

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

bleroux requested review from dnfield and jason-simmons May 19, 2023 00:10

jason-simmons approved these changes May 19, 2023

View reviewed changes

dnfield approved these changes May 19, 2023

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label May 19, 2023

auto-submit bot merged commit 6bc60c8 into flutter:main May 19, 2023

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter Engine from bca11a423f9c to 6bc60c8a9877 (1 revision) flutter/flutter#127162

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2023

6bc60c8 [Android] Fix BasicMessageChannel.resizeChannelBuffer (flutte…

9851401

…r/engine#41982)

bleroux deleted the android_fix_resize_channel_buffer branch May 19, 2023 14:08

bleroux mentioned this pull request Jul 28, 2023

Relax HarwareKeyboard assert on first received event flutter/flutter#131258

Closed

This was referenced Aug 7, 2023

Android - BasicMessageChannel.resizeChannelBuffer should use a non deprecated format and be more efficient flutter/flutter#132048

Closed

[Android] Expose channel buffer resize and overflow calls #44434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Fix BasicMessageChannel.resizeChannelBuffer#41982

[Android] Fix BasicMessageChannel.resizeChannelBuffer#41982
auto-submit[bot] merged 2 commits intoflutter:mainfrom
NevercodeHQ:android_fix_resize_channel_buffer

bleroux commented May 12, 2023

Uh oh!

gmackall commented May 18, 2023

Uh oh!

chinmaygarde commented May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

bleroux May 18, 2023 •

edited

Loading

Uh oh!

dnfield May 18, 2023

Uh oh!

dnfield May 18, 2023

Uh oh!

jason-simmons May 18, 2023

Uh oh!

jason-simmons commented May 18, 2023

Uh oh!

bleroux commented May 18, 2023 •

edited

Loading

Uh oh!

bleroux commented May 18, 2023

Uh oh!

Hixie commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

bleroux commented May 12, 2023

Description

Related Issue

Tests

Uh oh!

gmackall commented May 18, 2023

Uh oh!

chinmaygarde commented May 18, 2023

Uh oh!

dnfield May 18, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield May 18, 2023

Choose a reason for hiding this comment

Uh oh!

dnfield May 18, 2023

Choose a reason for hiding this comment

Uh oh!

jason-simmons May 18, 2023

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented May 18, 2023

Uh oh!

bleroux commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bleroux commented May 18, 2023

Uh oh!

Hixie commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bleroux May 18, 2023 •

edited

Loading

bleroux commented May 18, 2023 •

edited

Loading