search

mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
197 stars 87 forks source link

Crash fixes #601

Closed rcaridade145 closed 4 years ago

rcaridade145 commented 4 years ago

1 - window: fix crash if workspace is null 2- compositor: fix possible crash closing/destroying window

muktupavels commented 4 years ago

https://github.com/mate-desktop/marco/search?q=compositor%3A+fix+possible+crash+closing%2Fdestroying+window&type=Commits

raveit65 commented 4 years ago

How often we should revert this commit to fix https://github.com/mate-desktop/marco/issues/251 ?

rcaridade145 commented 4 years ago

Was not aware of this bug. Everything seemed OK from my testing. Will have a look at it

rcaridade145 commented 4 years ago

Did a short screen capture . Everytime i switch between workspaces the windows maintain their focus. Was this not the bug? bug.ogv.zip

raveit65 commented 4 years ago

@rcaridade145 A good description of the issue is here https://github.com/mate-desktop/marco/issues/251#issuecomment-261713147 Or this post where i could reproduce the issue the first time. https://github.com/mate-desktop/marco/issues/251#issuecomment-271672013 In short you can't click in a foreground window, you always match the window behind the foreground window. It doesn't happen all the time, and when using auto-raise the window with mouse pointer you don't run into this issue. You need to run marco several days or a week to run into this issue, when compositor: fix possible crash closing/destroying window is used. I biscet marco and found the culprit commit here at this post. https://github.com/mate-desktop/marco/issues/251#issuecomment-298713114

raveit65 commented 4 years ago

Any way, i start testing your PR. The daily crashes of marco going on my nerves.

joakim-tjernlund commented 4 years ago

I added the patches to 1.22.4 here and so far I haven't seen anything bad.

raveit65 commented 4 years ago

@joakim-tjernlund Can you please test https://github.com/mate-desktop/marco/issues/251 with compositor: fix possible crash closing/destroying window We don't want to introduce this issue the third time. And as i said you need to test several days or a week to see this bug if it exists.

joakim-tjernlund commented 4 years ago

@joakim-tjernlund Can you please test #251 with compositor: fix possible crash closing/destroying window We don't want to introduce this issue the third time. And as i said you need to test several days or a week to see this bug if it exists.

Well, clicking through all the Windows many times, Evolution sometimes gets sent to the back instead of the front. I can see how the selected Window sometimes changes to Evo just by moving the mouse around the Window List. Evo is my first item in Windows List. This is not new to me though, just moving the mouse around in the Window List sometimes changes active window even before this change. I prefer that over SEGV though.

joakim-tjernlund commented 4 years ago

I do need to have the Window list completely full and then some more. Thinking about it, it is probably due to me having focus follows mouse and I happen do a mouse over before reaching the Window list

rcaridade145 commented 4 years ago

I could not reproduce the error on my side as shown on the short screen capture i did . However i came across this bug https://bugzilla.gnome.org/show_bug.cgi?id=620744 and commit https://gitlab.gnome.org/GNOME/metacity/-/commit/f628d8f8901f46fa9e00707ae9d7ccfd1e85f427 As of now i've only backported from Metacity one commit which probably won't fix this issue. Needs a bit more work.

Edit: Can you please test now.

raveit65 commented 4 years ago

Did you noticed that PR doesn't build anymore? And what should last commit fix? I am really concerned that marco becomes more and unstable when we add more commits from metacity without a clear reproducer and saying why it is needed. This was exactly the problem when https://github.com/mate-desktop/marco/commit/768fdd8d3852e67555a585da28b6404a97853cbd was added the first time in 2015, and causes a serious regression/bug.

vkareh commented 4 years ago

@rcaridade145 thanks, now if you can please fixup that last commit and reword the commit messages to reflect the original description, then this will be ready for testing

rcaridade145 commented 4 years ago

@vkareh @raveit65 Sorry. I did fix it locally but forgot the commit. I thought i had done it :-1: Ok i will change the commits message.

rcaridade145 commented 4 years ago

@vkareh changed the messages

vkareh commented 4 years ago

Thanks, testing now

raveit65 commented 4 years ago

I can confirm that using the first 2 commits fixes the crashes and i didn't run in https://github.com/mate-desktop/marco/issues/251 Starting now with testing the whole PR.

joakim-tjernlund commented 4 years ago

I have been using all 4 patches for a while now and so far NP.

raveit65 commented 4 years ago

@mate-desktop/core-team Can another reviewer please explain author of PR how to edit existing commits?

rcaridade145 commented 4 years ago

I am sorry

raveit65 commented 4 years ago

Here is a tutorial about editing a commit with git rebase -i ..... and using git --amend to edit a commit. https://www.atlassian.com/git/tutorials/rewriting-history

In short (all in a terminal):

  1. git rebase -i 3d65c8b7f57ab6b9b99e39303e069c40eee91502 Press i (insert) to enable text editing in vim editor. In vim editor choose edit a commit, eg. change pick f2a873c compositor: fix possible crash closing/destroying window . Fixes to previous commit. to edit f2a873c compositor: fix possible crash closing/destroying window . Fixes to previous commit. Press esc to finish editing in vim. Close vim with :wq

  2. Edit eg. src/core/window.c, save it, and mark the file with git add src/core/window.c as edited.

  3. git commit --amend --> opens the vim editor for changing commit message if you want. Close vim with :wq

  4. Finish rebase command with git rebase --continue

Repeat that with other commits. Sorry, i can't help you more without doing the work for myself ;)

PS: There are a lot of git tutorials in the web....

raveit65 commented 4 years ago

The resulting diff is OK https://patch-diff.githubusercontent.com/raw/mate-desktop/marco/pull/601.diff but you edited the wrong commit to fix the format issues. I am still wondering that simply copying ready commits from gnome can be so problematic. Any way that needs to be reviewed and tested by more reviewers. @mate-desktop/core-team ping

vkareh commented 4 years ago

cherry-picked to 1.24:

raveit65 commented 4 years ago

Today i ran again in https://github.com/mate-desktop/marco/issues/251 So this isn't fixed. It was with deadbeef player window.