Skip to content

Commit 52070e3

Browse files
stuartmorgan-gstuartmorgan-g
authored
Fix handler unregistration in C++ channels (#16794)
MethodChannel and BasicMessageChannel in the C++ wrapper didn't have the expected semantics that passing a null handler would remove any existing handler. This was inconsistent with other platforms and with the lower-level object APIs in this wrapper, and made unregistration in cases where that's desirable more difficult due to needing to keep other object references. Adds tests for this functionality, and some backfill of missing tests for basic handler behavior. See #51207
1 parent d590e98 commit 52070e3

File tree

7 files changed

+247
-3
lines changed

7 files changed

+247
-3
lines changed

‎ci/licenses_golden/licenses_flutter‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,6 +754,7 @@ FILE: ../../../flutter/shell/platform/android/platform_view_android_jni.cc
754754
FILE: ../../../flutter/shell/platform/android/platform_view_android_jni.h
755755
FILE: ../../../flutter/shell/platform/android/vsync_waiter_android.cc
756756
FILE: ../../../flutter/shell/platform/android/vsync_waiter_android.h
757+
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/basic_message_channel_unittests.cc
757758
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/byte_stream_wrappers.h
758759
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/encodable_value_unittests.cc
759760
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/engine_method_result.cc
@@ -776,6 +777,7 @@ FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/
776777
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/json_message_codec.cc
777778
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/json_method_codec.cc
778779
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/method_call_unittests.cc
780+
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/method_channel_unittests.cc
779781
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc
780782
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc
781783
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/standard_codec.cc

‎shell/platform/common/cpp/client_wrapper/BUILD.gn‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,10 @@ executable("client_wrapper_unittests") {
4646

4747
# TODO: Add more unit tests.
4848
sources = [
49+
"basic_message_channel_unittests.cc",
4950
"encodable_value_unittests.cc",
5051
"method_call_unittests.cc",
52+
"method_channel_unittests.cc",
5153
"plugin_registrar_unittests.cc",
5254
"standard_message_codec_unittests.cc",
5355
"standard_method_codec_unittests.cc",

‎shell/platform/common/cpp/client_wrapper/basic_message_channel_unittests.cc‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/basic_message_channel.h"
6+
7+
#include <memory>
8+
#include <string>
9+
10+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/binary_messenger.h"
11+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_message_codec.h"
12+
#include "gtest/gtest.h"
13+
14+
namespace flutter {
15+
16+
namespace {
17+
18+
class TestBinaryMessenger : public BinaryMessenger {
19+
public:
20+
void Send(const std::string& channel,
21+
const uint8_t* message,
22+
const size_t message_size) const override {}
23+
24+
void Send(const std::string& channel,
25+
const uint8_t* message,
26+
const size_t message_size,
27+
BinaryReply reply) const override {}
28+
29+
void SetMessageHandler(const std::string& channel,
30+
BinaryMessageHandler handler) override {
31+
last_message_handler_channel_ = channel;
32+
last_message_handler_ = handler;
33+
}
34+
35+
std::string last_message_handler_channel() {
36+
return last_message_handler_channel_;
37+
}
38+
39+
BinaryMessageHandler last_message_handler() { return last_message_handler_; }
40+
41+
private:
42+
std::string last_message_handler_channel_;
43+
BinaryMessageHandler last_message_handler_;
44+
};
45+
46+
} // namespace
47+
48+
// Tests that SetMessageHandler sets a handler that correctly interacts with
49+
// the binary messenger.
50+
TEST(BasicMessageChannelTest, Registration) {
51+
TestBinaryMessenger messenger;
52+
const std::string channel_name("some_channel");
53+
const StandardMessageCodec& codec = StandardMessageCodec::GetInstance();
54+
BasicMessageChannel channel(&messenger, channel_name, &codec);
55+
56+
bool callback_called = false;
57+
const std::string message_value("hello");
58+
channel.SetMessageHandler(
59+
[&callback_called, message_value](const auto& message, auto reply) {
60+
callback_called = true;
61+
// Ensure that the wrapper recieved a correctly decoded message and a
62+
// reply.
63+
EXPECT_EQ(message.StringValue(), message_value);
64+
EXPECT_NE(reply, nullptr);
65+
});
66+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
67+
EXPECT_NE(messenger.last_message_handler(), nullptr);
68+
// Send a test message to trigger the handler test assertions.
69+
auto message = codec.EncodeMessage(EncodableValue(message_value));
70+
71+
messenger.last_message_handler()(
72+
message->data(), message->size(),
73+
[](const uint8_t* reply, const size_t reply_size) {});
74+
EXPECT_EQ(callback_called, true);
75+
}
76+
77+
// Tests that SetMessageHandler with a null handler unregisters the handler.
78+
TEST(BasicMessageChannelTest, Unregistration) {
79+
TestBinaryMessenger messenger;
80+
const std::string channel_name("some_channel");
81+
BasicMessageChannel channel(&messenger, channel_name,
82+
&flutter::StandardMessageCodec::GetInstance());
83+
84+
channel.SetMessageHandler([](const auto& message, auto reply) {});
85+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
86+
EXPECT_NE(messenger.last_message_handler(), nullptr);
87+
88+
channel.SetMessageHandler(nullptr);
89+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
90+
EXPECT_EQ(messenger.last_message_handler(), nullptr);
91+
}
92+
93+
} // namespace flutter

‎shell/platform/common/cpp/client_wrapper/include/flutter/basic_message_channel.h‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,16 @@ class BasicMessageChannel {
6060
}
6161

6262
// Registers a handler that should be called any time a message is
63-
// received on this channel.
63+
// received on this channel. A null handler will remove any previous handler.
64+
//
65+
// Note that the BasicMessageChannel does not own the handler, and will not
66+
// unregister it on destruction, so the caller is responsible for
67+
// unregistering explicitly if it should no longer be called.
6468
void SetMessageHandler(const MessageHandler<T>& handler) const {
69+
if (!handler) {
70+
messenger_->SetMessageHandler(name_, nullptr);
71+
return;
72+
}
6573
const auto* codec = codec_;
6674
std::string channel_name = name_;
6775
BinaryMessageHandler binary_handler = [handler, codec, channel_name](

‎shell/platform/common/cpp/client_wrapper/include/flutter/method_channel.h‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,16 @@ class MethodChannel {
6262
}
6363

6464
// Registers a handler that should be called any time a method call is
65-
// received on this channel.
65+
// received on this channel. A null handler will remove any previous handler.
66+
//
67+
// Note that the MethodChannel does not own the handler, and will not
68+
// unregister it on destruction, so the caller is responsible for
69+
// unregistering explicitly if it should no longer be called.
6670
void SetMethodCallHandler(MethodCallHandler<T> handler) const {
71+
if (!handler) {
72+
messenger_->SetMessageHandler(name_, nullptr);
73+
return;
74+
}
6775
const auto* codec = codec_;
6876
std::string channel_name = name_;
6977
BinaryMessageHandler binary_handler = [handler, codec, channel_name](

‎shell/platform/common/cpp/client_wrapper/method_channel_unittests.cc‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_channel.h"
6+
7+
#include <memory>
8+
#include <string>
9+
10+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/binary_messenger.h"
11+
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_method_codec.h"
12+
#include "gtest/gtest.h"
13+
14+
namespace flutter {
15+
16+
namespace {
17+
18+
class TestBinaryMessenger : public BinaryMessenger {
19+
public:
20+
void Send(const std::string& channel,
21+
const uint8_t* message,
22+
const size_t message_size) const override {}
23+
24+
void Send(const std::string& channel,
25+
const uint8_t* message,
26+
const size_t message_size,
27+
BinaryReply reply) const override {}
28+
29+
void SetMessageHandler(const std::string& channel,
30+
BinaryMessageHandler handler) override {
31+
last_message_handler_channel_ = channel;
32+
last_message_handler_ = handler;
33+
}
34+
35+
std::string last_message_handler_channel() {
36+
return last_message_handler_channel_;
37+
}
38+
39+
BinaryMessageHandler last_message_handler() { return last_message_handler_; }
40+
41+
private:
42+
std::string last_message_handler_channel_;
43+
BinaryMessageHandler last_message_handler_;
44+
};
45+
46+
} // namespace
47+
48+
// Tests that SetMethodCallHandler sets a handler that correctly interacts with
49+
// the binary messenger.
50+
TEST(MethodChannelTest, Registration) {
51+
TestBinaryMessenger messenger;
52+
const std::string channel_name("some_channel");
53+
const StandardMethodCodec& codec = StandardMethodCodec::GetInstance();
54+
MethodChannel channel(&messenger, channel_name, &codec);
55+
56+
bool callback_called = false;
57+
const std::string method_name("hello");
58+
channel.SetMethodCallHandler(
59+
[&callback_called, method_name](const auto& call, auto result) {
60+
callback_called = true;
61+
// Ensure that the wrapper recieved a correctly decoded call and a
62+
// result.
63+
EXPECT_EQ(call.method_name(), method_name);
64+
EXPECT_NE(result, nullptr);
65+
});
66+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
67+
EXPECT_NE(messenger.last_message_handler(), nullptr);
68+
// Send a test message to trigger the handler test assertions.
69+
MethodCall<EncodableValue> call(method_name, nullptr);
70+
auto message = codec.EncodeMethodCall(call);
71+
72+
messenger.last_message_handler()(
73+
message->data(), message->size(),
74+
[](const uint8_t* reply, const size_t reply_size) {});
75+
EXPECT_EQ(callback_called, true);
76+
}
77+
78+
// Tests that SetMethodCallHandler with a null handler unregisters the handler.
79+
TEST(MethodChannelTest, Unregistration) {
80+
TestBinaryMessenger messenger;
81+
const std::string channel_name("some_channel");
82+
MethodChannel channel(&messenger, channel_name,
83+
&flutter::StandardMethodCodec::GetInstance());
84+
85+
channel.SetMethodCallHandler([](const auto& call, auto result) {});
86+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
87+
EXPECT_NE(messenger.last_message_handler(), nullptr);
88+
89+
channel.SetMethodCallHandler(nullptr);
90+
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
91+
EXPECT_EQ(messenger.last_message_handler(), nullptr);
92+
}
93+
94+
} // namespace flutter

‎shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ class TestApi : public testing::StubFlutterApi {
2323
last_data_sent_ = message;
2424
return message_engine_result;
2525
}
26+
2627
bool MessengerSendWithReply(const char* channel,
2728
const uint8_t* message,
2829
const size_t message_size,
@@ -32,15 +33,27 @@ class TestApi : public testing::StubFlutterApi {
3233
return message_engine_result;
3334
}
3435

36+
// Called for FlutterDesktopMessengerSetCallback.
37+
void MessengerSetCallback(const char* channel,
38+
FlutterDesktopMessageCallback callback,
39+
void* user_data) override {
40+
last_callback_set_ = callback;
41+
}
42+
3543
const uint8_t* last_data_sent() { return last_data_sent_; }
44+
FlutterDesktopMessageCallback last_callback_set() {
45+
return last_callback_set_;
46+
}
3647

3748
private:
3849
const uint8_t* last_data_sent_ = nullptr;
50+
FlutterDesktopMessageCallback last_callback_set_ = nullptr;
3951
};
4052

4153
} // namespace
4254

43-
// Tests that the registrar returns a messenger that calls through to the C API.
55+
// Tests that the registrar returns a messenger that passes Send through to the
56+
// C API.
4457
TEST(MethodCallTest, MessengerSend) {
4558
testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique<TestApi>());
4659
auto test_api = static_cast<TestApi*>(scoped_api_stub.stub());
@@ -55,4 +68,28 @@ TEST(MethodCallTest, MessengerSend) {
5568
EXPECT_EQ(test_api->last_data_sent(), &message[0]);
5669
}
5770

71+
// Tests that the registrar returns a messenger that passes callback
72+
// registration and unregistration through to the C API.
73+
TEST(MethodCallTest, MessengerSetMessageHandler) {
74+
testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique<TestApi>());
75+
auto test_api = static_cast<TestApi*>(scoped_api_stub.stub());
76+
77+
auto dummy_registrar_handle =
78+
reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
79+
PluginRegistrar registrar(dummy_registrar_handle);
80+
BinaryMessenger* messenger = registrar.messenger();
81+
const std::string channel_name("foo");
82+
83+
// Register.
84+
BinaryMessageHandler binary_handler = [](const uint8_t* message,
85+
const size_t message_size,
86+
BinaryReply reply) {};
87+
messenger->SetMessageHandler(channel_name, std::move(binary_handler));
88+
EXPECT_NE(test_api->last_callback_set(), nullptr);
89+
90+
// Unregister.
91+
messenger->SetMessageHandler(channel_name, nullptr);
92+
EXPECT_EQ(test_api->last_callback_set(), nullptr);
93+
}
94+
5895
} // namespace flutter

0 commit comments

Comments
 (0)