Conversation
751f704 to
6515c18
Compare
6515c18 to
8d45964
Compare
|
It will most likely cause crashes here Try to open wcm and change some settings here, open-close windiows etc. And it will crash, as data is no longer passed to wayland by wroots, data pointer is simply dropped and wlroots pass NULL to wayland instead of this. I do revert those changes in local copy of wlroots (in multiple locations) - so it s stable now , but with vanilla wlroots it crashes. Not immediately - you need to do something, so it can run for a while and it compiles like a charm. But it is a stinker bomb under wayfire. I have no idea why this shitty commit landed in wlroots and how to fix it ob wayfire without reverting wlroots, but maybe you or ammen99 have better idea. Bad thing - it is not always crashes. so tests can pass
|
|
wlr_input_method_manager_v2.events.commit & wlr_input_method_manager_v2.events.destroy signals now use NULL sources as data instead of the wlr_input_method_v2 owner. That's the change causing the trouble. |
Can you possibly send a PR targeting this branch? |
|
Here is the troublemaking merge. |
|
#3003 could you please try this patch |
|
does not help much. |
|
wayfire: ../wayfire-0.10.1.1/src/core/seat/input-method-relay.cpp:426: wf::text_input::text_input(wf::input_method_relay*, wlr_text_input_v3*)::<lambda(void*)>: Assertion `input == wlr_text_input' failed. One more place where this one is called, and what is worse - after assertion this pointer is used. So just removing the assertion will not help from crash. To reproduce crash - run wcm, try to do some change in text field, then close wcm. Crash will follow. |
|
@mark-herbert42 The first thing that jumps out at me in your log is the first line.
Apparently you are not even using the branch in this pull request, nor the released version in the wlroots subproject. Please do not report issues unless you are actually using the wayfire |
|
wayfire.log if you apply this over your wlroots version it will work, but how I understand the idea Wayfire need to work with wlroots release how it is done by wlroots team, not using some special wlroots version |
|
@mark-herbert42 Thank you for testing this. I was unable to produce the crash and I am unsure about the exact mechanics of this plugin, but does this help the crash while preserving functionality? |
|
The patch is exactly what I was trying myself - if assert fails, remove assert. So i did - but seems that assert was doing something on purpose, and simply patching that out caused segfaults and a lot of trouble. But the issue is - if you do not have crash, maybe I have something enabled that you dont. Seems you might have input-method-v2 disabled in workarounds settings. When I uncheck it and restart wayfire - no crashes, and that's also understandable as the bad commint is in input v2 code. |
|
@mark-herbert42 Thanks, I was able to reproduce the crash indeed by enabling the input-method-v2 workaround option. This seems to help here, are you able to confirm it doesn't crash and does whatever it is expected to do? index ca26291f..5e25b867 100644
--- a/src/core/seat/input-method-relay.cpp
+++ b/src/core/seat/input-method-relay.cpp
@@ -329,6 +329,10 @@ void wf::input_method_relay::set_focus(wlr_surface *surface)
{
for (auto & text_input : text_inputs)
{
+ if (!text_input->input->current_enabled)
+ {
+ continue;
+ }
if (text_input->pending_focused_surface != nullptr)
{
assert(text_input->input->focused_surface == nullptr);
@@ -372,9 +376,6 @@ wf::text_input::text_input(wf::input_method_relay *rel, wlr_text_input_v3 *in) :
{
on_text_input_enable.set_callback([&] (void *data)
{
- auto wlr_text_input = static_cast<wlr_text_input_v3*>(data);
- assert(input == wlr_text_input);
-
if (relay->input_method == nullptr)
{
LOGI("Enabling text input, but input method is gone");
@@ -388,9 +389,6 @@ wf::text_input::text_input(wf::input_method_relay *rel, wlr_text_input_v3 *in) :
on_text_input_commit.set_callback([&] (void *data)
{
- auto wlr_text_input = static_cast<wlr_text_input_v3*>(data);
- assert(input == wlr_text_input);
-
if (!input->current_enabled)
{
LOGI("Inactive text input tried to commit");
@@ -414,30 +412,16 @@ wf::text_input::text_input(wf::input_method_relay *rel, wlr_text_input_v3 *in) :
on_text_input_disable.set_callback([&] (void *data)
{
- auto wlr_text_input = static_cast<wlr_text_input_v3*>(data);
- assert(input == wlr_text_input);
-
relay->disable_text_input(input);
});
on_text_input_destroy.set_callback([&] (void *data)
{
- auto wlr_text_input = static_cast<wlr_text_input_v3*>(data);
- assert(input == wlr_text_input);
-
- if (input->current_enabled)
- {
- relay->disable_text_input(wlr_text_input);
- }
-
set_pending_focused_surface(nullptr);
on_text_input_enable.disconnect();
on_text_input_commit.disconnect();
on_text_input_disable.disconnect();
on_text_input_destroy.disconnect();
-
- // NOTE: the call destroys `this`
- relay->remove_text_input(wlr_text_input);
});
on_pending_focused_surface_destroy.set_callback([&] (void *data) |
|
unfortunately not - same bad crash |
|
@mark-herbert42 I see you have enabled asan, but somehow it is not printing line numbers. Have you passed |
|
fix2.patch I've not used Dsanitize - I am actualy emerging gento ebuild, just feeding some patches to it. Will try to change ebuild and revert my fix. |
|
How are you applying the patch(es) I am sending? Because it seems that you do not need to do this if you applied the suggested patch correctly. |
|
adding the to ebuild here are my meson options for wayfire. But wlroots build separately. |
I'm guessing it builds with debug symbols but then strips the binary. Can you tell it to not strip? |
|
So far my little workaround without the last patch applied it works so far. replacng this with this allows all the further code to run correctly. My guess is - before wlroots was sending the pointer to wayland, so in this pointer we were receiving the "result" of wayland call. So the assert was actually comparing the cached input with the data pointer contents to make sure it allright. But now wlroots is doing call blindly returning this NULL so assert between NULL and cached input is failing and it dumps before actual dump. So I assume that wlroots call always good so simply passing the input instead of data into the further code, hoping that it is allright. It works so far - anyway in old code if data not equal to input the assert will fail and crash. So now it also will crash if the wlroots call failed.... But that's a guess - I am not an expert in this and it iw beyound my knowledge/ here is the log with your last patch applied and nostrip. indeed it was stripping that removed the line numbers etc, so your guess is 100% right - here are the code details. It is just your last patch with asserts and some other operations removed completely. |
|
@mark-herbert42 The whole point of having the line numbers with a specific patch is so that I can have eyes on the code where it's failing. If you change other things, the line numbers might change too, and then I am left in the dark. Please apply this patch with no other patches or changes, and let me know if it works for you. We need to try and get this pushed to the branch so that it works for everyone, which is the ultimate goal here. |
|
okay - so applied this PR over clean wayfire git master + the last patch. And this one works like a charn - no crashes. tried all the tricks that cases crashes before and still up and running. WCM, Telegram, firefox - all working well. Hope to see this patch as part of this PR to merge it all cleanly over master branch , and better to see the whole PR merged there. |
No description provided.