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

[Linux] Allow BasicMessageChannel sending and responding to null message #42808

Merged
merged 2 commits into from
Jun 15, 2023
Merged

[Linux] Allow BasicMessageChannel sending and responding to null message #42808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f5e17e
[Linux] Allow BasicMessageChannel sending and responding to null message
bleroux Jun 14, 2023
20e5b55
Add 'allow_none' to basic message channel documentation
bleroux Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion shell/platform/linux/fl_basic_message_channel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ G_MODULE_EXPORT void fl_basic_message_channel_send(FlBasicMessageChannel* self,
GAsyncReadyCallback callback,
gpointer user_data) {
g_return_if_fail(FL_IS_BASIC_MESSAGE_CHANNEL(self));
g_return_if_fail(message != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will mean calling fl_message_codec_encode_message with null below; does that work? Is there unit testing of it? Does it return a non-null data pointer, such that line 246 won't return an error?

Copy link
Contributor Author

@bleroux bleroux Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This will mean calling fl_message_codec_encode_message with null below; does that work?

Yes, I have checked that fl_message_codec_encode_message allow null message, it properly replaces it with fl_value_new_null() :

engine/shell/platform/linux/fl_message_codec.cc

Lines 27 to 31 in ac954ee

g_autoptr(FlValue) null_value = nullptr;
if (message == nullptr) {
null_value = fl_value_new_null();
message = null_value;
}

Is there unit testing of it?

I think this is covered by those tests:

engine/shell/platform/linux/fl_standard_message_codec_test.cc

Lines 52 to 61 in ac954ee

TEST(FlStandardMessageCodecTest, EncodeNullptr) {
g_autofree gchar* hex_string = encode_message(nullptr);
EXPECT_STREQ(hex_string, "00");
}
TEST(FlStandardMessageCodecTest, EncodeNull) {
g_autoptr(FlValue) value = fl_value_new_null();
g_autofree gchar* hex_string = encode_message(value);
EXPECT_STREQ(hex_string, "00");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add '(allow-none)' to the documentation string in public/flutter_linux/fl_basic_message_channel.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please add '(allow-none)' to the documentation string in public/flutter_linux/fl_basic_message_channel.h

Done, thanks for noticing this 👍


g_autoptr(GTask) task =
callback != nullptr ? g_task_new(self, cancellable, callback, user_data)
Expand Down
35 changes: 34 additions & 1 deletion shell/platform/linux/fl_basic_message_channel_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ TEST(FlBasicMessageChannelTest, SendMessageWithoutResponse) {
}
// NOLINTEND(clang-analyzer-core.StackAddressEscape)

// Called when the message response is received in the SendMessage test.
// Called when the message response is received in the SendMessageWithResponse
// test.
static void echo_response_cb(GObject* object,
GAsyncResult* result,
gpointer user_data) {
Expand Down Expand Up @@ -189,3 +190,35 @@ TEST(FlBasicMessageChannelTest, ReceiveMessage) {
// Blocks here until response_cb is called.
g_main_loop_run(loop);
}

// Called when the message response is received in the
// SendNullMessageWithResponse test.
static void null_message_response_cb(GObject* object,
GAsyncResult* result,
gpointer user_data) {
g_autoptr(GError) error = nullptr;
g_autoptr(FlValue) message = fl_basic_message_channel_send_finish(
FL_BASIC_MESSAGE_CHANNEL(object), result, &error);
EXPECT_NE(message, nullptr);
EXPECT_EQ(error, nullptr);

EXPECT_EQ(fl_value_get_type(message), FL_VALUE_TYPE_NULL);

g_main_loop_quit(static_cast<GMainLoop*>(user_data));
}

// Checks sending a null message with a response works.
TEST(FlBasicMessageChannelTest, SendNullMessageWithResponse) {
g_autoptr(GMainLoop) loop = g_main_loop_new(nullptr, 0);

g_autoptr(FlEngine) engine = make_mock_engine();
FlBinaryMessenger* messenger = fl_binary_messenger_new(engine);
g_autoptr(FlStandardMessageCodec) codec = fl_standard_message_codec_new();
g_autoptr(FlBasicMessageChannel) channel = fl_basic_message_channel_new(
messenger, "test/echo", FL_MESSAGE_CODEC(codec));
fl_basic_message_channel_send(channel, nullptr, nullptr,
null_message_response_cb, loop);

// Blocks here until null_message_response_cb is called.
g_main_loop_run(loop);
}
4 changes: 4 additions & 0 deletions shell/platform/linux/fl_standard_message_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,10 @@ static GBytes* fl_standard_message_codec_encode_message(FlMessageCodec* codec,
static FlValue* fl_standard_message_codec_decode_message(FlMessageCodec* codec,
GBytes* message,
GError** error) {
if (g_bytes_get_size(message) == 0) {
return fl_value_new_null();
Copy link
Contributor

Choose a reason for hiding this comment

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

@cbracken Null messages strike again :P

How do you think we should handle null messages here: null data, or a value whose value is null? IIRC you found that this was inconsistent in #25064 but I forget whether you cleaned it up later.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to return the FlValue object here, because returning NULL implies there was an error (GObject convention).

}

FlStandardMessageCodec* self =
reinterpret_cast<FlStandardMessageCodec*>(codec);

Expand Down
13 changes: 13 additions & 0 deletions shell/platform/linux/fl_standard_message_codec_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ TEST(FlStandardMessageCodecTest, EncodeNull) {
EXPECT_STREQ(hex_string, "00");
}

TEST(FlStandardMessageCodecTest, DecodeNull) {
// Regression test for https://github.com/flutter/flutter/issues/128704.

g_autoptr(FlStandardMessageCodec) codec = fl_standard_message_codec_new();
g_autoptr(GBytes) data = g_bytes_new(nullptr, 0);
g_autoptr(GError) error = nullptr;
g_autoptr(FlValue) value =
fl_message_codec_decode_message(FL_MESSAGE_CODEC(codec), data, &error);

EXPECT_FALSE(value == nullptr);
EXPECT_EQ(fl_value_get_type(value), FL_VALUE_TYPE_NULL);
}

static gchar* encode_bool(gboolean value) {
g_autoptr(FlValue) v = fl_value_new_bool(value);
return encode_message(v);
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/linux/public/flutter_linux/fl_basic_message_channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ gboolean fl_basic_message_channel_respond(
/**
* fl_basic_message_channel_send:
* @channel: an #FlBasicMessageChannel.
* @message: message to send, must match what the #FlMessageCodec supports.
* @message: (allow-none): message to send, must match what the #FlMessageCodec
* supports.
* @cancellable: (allow-none): a #GCancellable or %NULL.
* @callback: (scope async): (allow-none): a #GAsyncReadyCallback to call when
* the request is satisfied or %NULL to ignore the response.
Expand Down