search

flutter-tizen / plugins

Flutter plugins for Tizen
66 stars 47 forks source link

Invalid use of EventChannel in some plugins #131

Closed rwalczyna closed 3 years ago

rwalczyna commented 3 years ago

I analyzed some crash on TM1 recently and it turns out that there is a problem with use of EventChannel.

There are some places, where developers create EventChannel as local object which is deleted at the exit of the function. Unfortunately we can't do this, because EventChannel not only passes handlers to engine, but also keeps is_listening_ variable. This variable is used in binary_handler and also in SetStreamHandler to cancel listening - without it there might be some calls to handler even after removing the handler. But as far as I know, everything on platform channel is being called on one thread, so there should be no issue if we would just remove is_listening_ from EventChannel and make it local to binary_helper lambda. What do you think about this solution? One drawback is that we would need to make this change in upstream.

The second solution is to fix the code of plugins - wearable_rotary_plugin, video_player, sensors, connectivity and battery are affected.

I think that the second approach is better.

swift-kim commented 3 years ago

I think we need to first check if this is a genuine bug of the client_wrapper implemenation or not. There is no use case of EventChannel in the official plugins, but there are some in the plus plugins:

and also MethodChannel is already being used in a similar way in the first-party plugins.

@bbrto21 Didn't you say the channel instances were held by something when we talked about this matter before? I know you were not very confident then but we need to check this issue again.

bbrto21 commented 3 years ago

@rwalczyna To discuss the issue you reported, could you please share how to reproduce the crash and any known clues so far as detailed as possible?

rwalczyna commented 3 years ago

@swift-kim binary_handler in MethodChannel is not capturing this, so there is no problem with using it that way. I think it is more like a misuse of EventChannel than a bug in implementation.

@bbrto21 I found it while using MethodChannel and EventChannel in the same app. At the begining everything was working OK, but after registering EventChannel, application crashed. It turns out that is_listening_ is changed to true, what modifies the memory of method_channel handler and results in SIGSEGV:

Thread 1 "runner" received signal SIGSEGV, Segmentation fault.
0x90000000 in ?? ()
>>> bt
#0  0x90000000 in ?? ()
#1  0xb6c8c8f8 in std::function<void (unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>)>::operator()(unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>) const (this=0x2a162001, __args=..., __args=..., __args=...) at /home/r.walczyna/tizen-studio/tools/smart-build-interface/../arm-linux-gnueabi-gcc-9.2/lib/gcc/arm-tizen-linux-gnueabi/9.2.0/../../../../arm-tizen-linux-gnueabi/include/c++/9.2.0/bits/std_function.h:690
#2  0xb6c8b1d8 in flutter::(anonymous namespace)::ForwardToHandler (messenger=0x2a106620, message=0xbeffe47c, user_data=0x2a162001) at /home/r.walczyna/git/flutter/flutter-tizen/bin/cache/artifacts/engine/common/cpp_client_wrapper/core_implementations.cc:58
#3  0xb6d7ac74 in flutter::IncomingMessageDispatcher::HandleMessage(FlutterDesktopMessage const&, std::__1::function<void ()> const&, std::__1::function<void ()> const&) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_tizen_mobile.so
#4  0xb6d75cc6 in FlutterTizenEngine::RunEngine(FlutterDesktopEngineProperties const&)::$_3::__invoke(FlutterPlatformMessage const*, void*) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_tizen_mobile.so
#5  0xb4f7864a in std::__1::__function::__func<FlutterEngineInitialize::$_48, std::__1::allocator<FlutterEngineInitialize::$_48>, void (fml::RefPtr<flutter::PlatformMessage>)>::operator()(fml::RefPtr<flutter::PlatformMessage>&&) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_engine.so
#6  0xb4f8191a in flutter::PlatformViewEmbedder::HandlePlatformMessage(fml::RefPtr<flutter::PlatformMessage>) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_engine.so
#7  0xb52d2eb2 in std::__1::__function::__func<flutter::Shell::OnEngineHandlePlatformMessage(fml::RefPtr<flutter::PlatformMessage>)::$_38, std::__1::allocator<flutter::Shell::OnEngineHandlePlatformMessage(fml::RefPtr<flutter::PlatformMessage>)::$_38>, void ()>::operator()() () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_engine.so
#8  0xb4f800ec in flutter::EmbedderTaskRunner::PostTask(unsigned long long) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_engine.so
#9  0xb4f7a33c in flutter::EmbedderEngine::RunTask(FlutterTask const*) () from /opt/usr/globalapps/com.example.application_common_example/bin/../lib/libflutter_engine.so
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

I found it on TM1 with plugin that is located on internal f-project repo. It is not 100% reproducible, because it depends on it whether application reuses the memory after event_channel. I was not able to create minimal code with the crash.

swift-kim commented 3 years ago

Thanks. If the plus plugins are also misusing EventChannel, the second option (changing the plugin code) looks better for me too. Actually I don't know what the original author's intention about the use of EventChannel was.

bbrto21 commented 3 years ago

@rwalczyna I've reproduced it and seen crash, I carefully reviewed the EventChannel. Now I agree with you in general and I prefer your second opinion. One thing I wonder is that I think this(such as crash) should have happened much more often... but it didn't. As you mentioned, unlike MethodChannel, the BinaryMessageHandler of EventChannel captures the this pointer of an instance wrapped by a unique pointer and accesses its members(is_listening_) later after destroyed the unique pointer. so I think the crash should have occurred more often if I intentionally trigger "listen" and "cancel", but it didn't... Am I misunderstanding something? Why doesn't it happen more often?

rwalczyna commented 3 years ago

We have no influence to memory allocation inside an application. The memory previously used for EventChannel might be used for some 'not very important variable' or might be not reused at all. Also when we use OnCancel or OnListen then only one bit is changed (is_listening_). So overall I think that there is a very low probability it will change something important.