Gnome shell 3.30 compatibility #299
Conversation
hedayat
commented
Dec 19, 2018
hedayat
commented
- Fix corrupting Gnome Shell 3.30 lock functionality
- Remove some warnings
|
I tested this on gnome-shell 3.30.2 on a Manjaro (Arch-Linux). After this patch it isn't crashing anymore and working stable again. |
|
I can confirm that this looks really good. I've been fighting with gnome-shell dying after blanking the screen for weeks until I found this. |
|
Thanks for testing my patch. @mwilck Sure, but lets see if there is anybody around willing to merge some PR! I'm afraid this repo is not maintained anymore. :( |
I can see that you're listed as hamster core developer yourself - can't you commit here? |
|
No, I'm a contributor, since there was previous PRs of mine which was merged. But I'm not a core developer and don't have access to merge PRs. |
|
I also just tried this repo on 3.30 (Ubuntu 18.10), with #302 applied to fix the compilation. I can confirm the problems that this PR fixes, and they are indeed fixed by this commit. The first commit, about accessing I haven't dug into the exact cause, but I suspect there is some race condition here (that might have been present for longer, dunno). The fix present in this PR is to just remove The second problem is a crash when the plugin is disabled, e.g. when blanking or locking the screen. This apparently tries to destroy two objects that are already destroyed, resulting in an error, stack trace and then a crash. See below for a full log. In the log, I see two different stacktraces, pointing to line 197 and 198: hamster-shell-extension/extension/extension.js Lines 197 to 198 in 0d815b2 The fix applied by this PR is to simply remove those two destroy calls, which indeed seems to solve the problem. However, I'm not so sure this is really the right fix. Why does this crash happening in the first place? Is the disable ran twice, or is it ran before the enable is ran perhaps? Why was this not broken in earlier gnome-shell versions? At the very least, we should wonder if removing these lines to fix 3.30 might be breaking earlier versions (or at least cause memory leaks)? @hedayat, did you investigate this further? I want to do a bit more debugging about what happens exactly, but since that involves crashing my gnome-shell to find out, I'm wrapping up this comment and will report in a follow-up later :-) Here's the log snippet about the crash: |
|
I had a (rather superficial) look into a similar problem for the TopIcons plus extension recently. I believe the point is that in GNOME 3.30, destroying a parent (widget?) object destroys also the children of that object, and if these children are then explicitly destroyed by the extension, it comes down to a "double free". In earlier GNOME versions, it was necessary to destroy the children expllicitly. At least this is my naïve understanding of the issue. IMO it's a severe design problem of the GNOME shell that this kind of backward incompatibility of an extension may actually crash the shell. |
|
@mwilck thanks for clarifying. That would suggest that we indeed need different handling for 3.30 and earlier versions... I did a little bit of testing, and it indeed seems that the problem is not in a weird enable/disable flow. At startup, I'm seeing enable, deferred_enable (with just one of apiProxy and WindowsProxy defined, so nothing happens), deferred_enable (with both defined, so initialization runs). When locking, I just see one disable (this is with the fix applied, though). What's interesting, though, is that the problem is in line 197/198, but the commit actually removes 196/197. Assuming that It's a bit weird that the It might be good to modify the |
I can't find it any more, sorry. I'd seen something similar to what I wrote above for some other extension. Must have been something other than TopIcons plus though. |
|
FTR, this is the local change I made to TopIcons plus. Something has changed in Gst wrt object desctruction. That's all can say. Documentation is hard to come by. |
This is for build environments that are offline and can't fetch convenience.js from the net.
|
Because GNOME shell 3.32 ff is incompatible with 3.30, we'll need to branch here. We should have a clean branch point if possible. @hedayat, could you rebase this PR, to #325 after that's been merged, and also merge in the first 3 commits of #312, in particular aae8340, which is necessary for clean 3.30 support? @projecthamster/gnome-shell-extension fellow maintainers, my strategy is to follow up with the other PRs one by one, so that we get a clean straight version history, with exactly one potential branch point on the |
|
OK, I will. :) |
Gnome 3.30 - update for PR projecthamster#299
|
@mwilck Merged, thanks. And sorry :P |
|
Thank you, @hedayat. As no other maintainer objected, and the two of us seem to agree that this PR is fine, let's get it merged. |
|
Do we have the setup to build and release this version of the extension in Gnome Extensions? #312 removes 3.30 support, so would be good to distribute this beforehand? |
