GNOME Bugzilla – Bug 683986
MessageTray: fix reentrancy when calling out to the grab helper
Last modified: 2013-02-14 17:05:47 UTC
Similar to 683546. I'm not sure where this bug lives, but let's make the code robust.
Created attachment 224276 [details] [review] MessageTray: fix reentrancy when calling out to the grab helper
Btw, the error this bug is attempting to fix is JS ERROR: !!! Exception in callback for signal: notify JS ERROR: !!! message = '"this._notification is null"' JS ERROR: !!! fileName = '"/opt/gnome/share/gnome-shell/js/ui/messageTray.js"' JS ERROR: !!! lineNumber = '2289' JS ERROR: !!! stack = '"()@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:2289 wrapper()@/opt/gnome/share/gjs-1.0/lang.js:212 ()@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:1973 wrapper()@/opt/gnome/share/gjs-1.0/lang.js:212 ([object Object],[object Object])@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:1777 wrapper([object Object],[object Object])@/opt/gnome/share/gjs-1.0/lang.js:212 _emit("notify",[object Object])@/opt/gnome/share/gjs-1.0/signals.js:124 ([object Object])@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:1168 wrapper([object Object])@/opt/gnome/share/gjs-1.0/lang.js:212 _parent([object Object])@/opt/gnome/share/gjs-1.0/lang.js:175 ()@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:693 wrapper()@/opt/gnome/share/gjs-1.0/lang.js:212 ()@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:678 wrapper()@/opt/gnome/share/gjs-1.0/lang.js:212 which is caused by _notificationState begin 2 (SHOWN) while _notification is null, and indeed the notification is hidden. I now added some debug to my tree, I hope to get to the root of this problem soon.
*** Bug 682533 has been marked as a duplicate of this bug. ***
Any chance to have this patch merged soon? I'm using chat bubbles a lot and this bug is really annoying.
Review of attachment 224276 [details] [review]: "I'm not sure where this bug lives" Yeah, I'm not either. Let's just push this hack for now.
Unless you found the problem, of course.
(In reply to comment #6) > Unless you found the problem, of course. I wish I had. I'll push the hack with a comment pointing here.
Attachment 224276 [details] pushed as 8095448 - MessageTray: fix reentrancy when calling out to the grab helper Heh, let's call this fixed...
Found it! And it had nothing to do with poor GrabHelper...
Created attachment 229391 [details] [review] MessageTray: restore opacity when expanding a notification MessageTray tweens both opacity and y to hide or show _notificationWidget, but only y when expanding it. This means that an existing tween to hide the notification will continue running, clearing the notification state. If the hiding one completes before the showing one, the onComplete handler will throw an exception (because the notification was nullified) and therefore break the state tracking.
Review of attachment 229391 [details] [review]: You go, girl! Nice job. Feel free to revert the HACK patch for now, too.
Attachment 229391 [details] pushed as b121c25 - MessageTray: restore opacity when expanding a notification
And here it is again... Note how it moved to a slighlty different place JS ERROR: !!! Error calling onComplete JS ERROR: !!! message = '"this._notification is null"' JS ERROR: !!! fileName = '"/opt/gnome/share/gnome-shell/js/ui/messageTray.js"' JS ERROR: !!! lineNumber = '2303' JS ERROR: !!! stack = '"()@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:2303 wrapper()@/opt/gnome/share/gjs-1.0/lang.js:204 ("_notificationState",2,wrapper,[object Object],(void 0))@/opt/gnome/share/gnome-shell/js/ui/messageTray.js:2120 wrapper("_notificationState",2,wrapper,[object Object],(void 0))@/opt/gnome/share/gjs-1.0/lang.js:204 ("_notificationState",2,wrapper,[object Object],(void 0))@/opt/gnome/share/gnome-shell/js/ui/tweener.js:109 _callOnFunction((function () {oldHandler.apply(eventScope, oldParams);handler(target);}),"onComplete",[object Object],[object GObject_Object],[object Array])@ /opt/gnome/share/gjs-1.0/tweener/tweener.js:202 _updateTweenByIndex(0)@/opt/gnome/share/gjs-1.0/tweener/tweener.js:332 _updateTweens()@/opt/gnome/share/gjs-1.0/tweener/tweener.js:344 _onEnterFrame([object Object])@/opt/gnome/share/gjs-1.0/tweener/tweener.js:359 _emit("prepare-frame")@/opt/gnome/share/gjs-1.0/signals.js:124 (421)@/opt/gnome/share/gnome-shell/js/ui/tweener.js:249 wrapper(421)@/opt/gnome/share/gjs-1.0/lang.js:204 ([object GObject_Object],421)@/opt/gnome/share/gnome-shell/js/ui/tweener.js:224
Created attachment 229865 [details] [review] MessageTray: remove all tweens when tweening for state parameters MessageTray._tween sets the state variable to the in-progress value, so it must be sure that at the end of the animation the value will be the corresponding final and nothing else will happen in between. Let's fix this once and for all.
Review of attachment 229865 [details] [review]: Sigh. Go.
Attachment 229865 [details] pushed as 1a27d7d - MessageTray: remove all tweens when tweening for state parameters
(In reply to comment #15) > Review of attachment 229865 [details] [review]: > > Sigh. Go. You'll sigh more: it's not over yet. I readded logging for now.
https://bugs.launchpad.net/bugs/1088759 says that the 1a27d patch is causing problems when we tried applying it in Ubuntu 13.04 Alpha "Raring". Here's the diff from the older version without the problems: http://launchpadlibrarian.net/124090291/gnome-shell_3.6.2-0ubuntu2_3.6.2-0ubuntu3.diff.gz
Uh, yes, you need to backport 41f933b89e275d06936bba2df8796a16e02baf41 too.
Created attachment 232118 [details] [review] MessageTray: fix reentrancy problem in hideNotificationCompleted Hiding notificationWidget with a telepathy notification causes unfocused to be emitted, which causes a reentrant updateState. If another notification is queued, it is shown before the old one is cleared.
*** Bug 687318 has been marked as a duplicate of this bug. ***
Created attachment 233826 [details] [review] MessageTray: fix reentrancy problem in hideNotificationCompleted Hiding notificationWidget with a telepathy notification causes unfocused to be emitted, which causes a reentrant updateState. If another notification is queued, it is shown before the old one is cleared. Rebased. Mind adding yet another hack to fix this bug? :D
*** Bug 692096 has been marked as a duplicate of this bug. ***
I just hit this stacktrace with all but the last patch: (gnome-shell:23635): Clutter-WARNING **: Actor '<unnamed>[<StBin>:0x23e67a0]' should not be mapped if parent '<notification-container>[<StWidget>:0x20f59c0]'is not visible JS ERROR: !!! Error calling onComplete JS ERROR: !!! message = '"this._notification is null"' JS ERROR: !!! fileName = '"/usr/share/gnome-shell/js/ui/messageTray.js"' JS ERROR: !!! lineNumber = '2242' JS ERROR: !!! stack = '"()@/usr/share/gnome-shell/js/ui/messageTray.js:2242 wrapper()@/usr/share/gjs-1.0/lang.js:204 ("_notificationState",2,wrapper,[object Object],(void 0))@/usr/share/gnome-shell/js/ui/messageTray.js:2065 wrapper("_notificationState",2,wrapper,[object Object],(void 0))@/usr/share/gjs-1.0/lang.js:204 ("_notificationState",2,wrapper,[object Object],(void 0))@/usr/share/gnome-shell/js/ui/tweener.js:109 _callOnFunction((function () {oldHandler.apply(eventScope, oldParams);handler(target);}),"onComplete",[object Object],[object _private_St_Widget],[object Array])@/usr/share/gjs-1.0/tweener/tweener.js:202 _updateTweenByIndex(0)@/usr/share/gjs-1.0/tweener/tweener.js:332 _updateTweens()@/usr/share/gjs-1.0/tweener/tweener.js:344 _onEnterFrame([object Object])@/usr/share/gjs-1.0/tweener/tweener.js:359 _emit("prepare-frame")@/usr/share/gjs-1.0/signals.js:124 (431)@/usr/share/gnome-shell/js/ui/tweener.js:249 wrapper(431)@/usr/share/gjs-1.0/lang.js:204 ([object _private_Clutter_Timeline],431)@/usr/share/gnome-shell/js/ui/tweener.js:224 Can I assume adding the final patch will fix this?
It was my assumption, but I was proven wrong, once again...
Review of attachment 233826 [details] [review]: OK. Why won't you just rewrite the MessageTray at some point? Given how this was originally written for a completely different design, I'm sure we can find tons of dead code.
(In reply to comment #26) > Review of attachment 233826 [details] [review]: > > OK. > > Why won't you just rewrite the MessageTray at some point? Given how this was > originally written for a completely different design, I'm sure we can find tons > of dead code. Because rewriting the MessageTray from scratch would introduce new bugs, and we'd need to debug them again. And because, at the end of the day, I'm not paid to code for GNOME. I code for fun, and the tray code surely doesn't belong in that category... (No offense to the original authors, of course)
Attachment 233826 [details] pushed as 85743ed - MessageTray: fix reentrancy problem in hideNotificationCompleted
Any chance to have this fix merged to 3.6 as well? It's my most annoying Shell issue on F18.
Idem #29. any chance?
I don't know, I'm not very confident, given the number of patches that piled up over time... Jasper?
Tbh, it can't really be worst than the current situation where the message tray disappeared at least once per day on F18.
Indeed, this is a major issue with the gnome-shell in 3.6. This should be fixed in a stable release as for anyone using notifications this is causing a lot of trouble. The tray disappears many times a day and if you're receiving empathy messages you don't know about them at all unless you restart the shell.
(In reply to comment #29) > Any chance to have this fix merged to 3.6 as well? It's my most annoying Shell > issue on F18. I've added a scratch build in https://bugzilla.redhat.com/show_bug.cgi?id=877762, testing appreciated. In case feedback is positive, I'll push the backported patches to the gnome-3-6 and prepare a 3.6.3 release (though I'd like it to include the fix for bug 689106 as well)
(In reply to comment #34) > (In reply to comment #29) > > Any chance to have this fix merged to 3.6 as well? It's my most annoying Shell > > issue on F18. > > I've added a scratch build in > https://bugzilla.redhat.com/show_bug.cgi?id=877762, testing appreciated. In > case feedback is positive, I'll push the backported patches to the gnome-3-6 > and prepare a 3.6.3 release (though I'd like it to include the fix for bug > 689106 as well) I used this version all day yesterday and it works fine. It's nice to finally not having to restart the Shell every couple of hours to get the message tray back. :)
Confirming that in new version message tray does not disappear. Great!
OK, I pushed all patches to the stable gnome-3-6 branch. Thanks everyone for testing!
Created attachment 235980 [details] screenshot of hidden notification with just the x button visible Hi! I just installed 3.6.3 and now face a new problem relating to the message tray which was not there earlier, and was wondering if it might have to do with the patches from here. With 3.6.3, when I insert a removable disk (external HD, etc.) the notification pop-up does not pop-up all the way but sits below the screen with just the x button visible. Perhaps the attached screenshot makes things clearer. Clicking on the barely visible x button causes this notification to close after which the message tray can be used as usual. This used to work just fine with 3.6.2. All other notifications seem to work without any issues with the new version as well. Please excuse me if this does not belong here, in which case I will open a new bug for this issue. Thanks!
commit 41f933b89e275d06936bba2df8796a16e02baf41 fixes that. https://bugzilla.gnome.org/show_bug.cgi?id=689295 Florian, I guess you missed that one.
(In reply to comment #39) > Florian, I guess you missed that one. Yes. There are over 600 commits on master since 3-6 was branched, I didn't go through all of them. I've pushed the missing patch to the 3-6 branch, but I won't be rolling another release before UI freeze.
So, for packagers, what to do ? Skip this release because I had to downgrade to gnome-shell 3.6.2 to have working notifications !
I would hardly expect you to go through all the commits in master, however it was mentioned above in this bug report https://bugzilla.gnome.org/show_bug.cgi?id=683986#c19 Either way do drama, and anyone packaging this could just pull it in themselves.
s/do/no/
(In reply to comment #41) > So, for packagers, what to do ? The fix is on the gnome-3-6 branch, packagers can grab it from there. I will do a follow-up release eventually, but (1) it's very much possible that I missed other regressions caused by those patches, so let's get this some exposure for testing first before piling up releases, and (2) we are five days away from 3.8 beta, so 3.6 is not very high on the priority list (to say the least)
(In reply to comment #44) > (In reply to comment #41) > > So, for packagers, what to do ? > > The fix is on the gnome-3-6 branch, packagers can grab it from there. I will do > a follow-up release eventually, but (1) it's very much possible that I missed > other regressions caused by those patches, so let's get this some exposure for > testing first before piling up releases, and (2) we are five days away from 3.8 > beta, so 3.6 is not very high on the priority list (to say the least) Ok ! I reported a bug on archlinux tracker related to this issue. And I can understand that as 3.8 beta is coming, these bugs are very low level priority. Thanks for your kind answer.
(In reply to comment #44) >(1) it's very much possible that I missed > other regressions caused by those patches, so let's get this some exposure for > testing first before piling up releases, I think that is all re regressions as far as the messagetray fixes (we have had these in Ubuntu for some time). Can't speak for the other fixes you pulled in however.
(In reply to comment #46) > Can't speak for the other fixes you pulled in however. Everything else (not that it is much) is far less intrusive, so should be fairly safe.
*** Bug 693823 has been marked as a duplicate of this bug. ***
*** Bug 693827 has been marked as a duplicate of this bug. ***