GNOME Bugzilla – Bug 691963
crash when alt+tab s slow and one press the mouse a few times
Last modified: 2013-01-18 23:34:29 UTC
Created attachment 233679 [details] [review] switcherpopup: handle the SwitcherPopup if it fails to show. Disclaimer: This is not a good patch. It only demonstrate the issue and how to fix it. To reproduce without heavy load and randomness one can: - start click on mouse - continue clicking on the mouse and hold "alt+tab" while doing so depending on how fast you click it is a matter of a few seconds before crash follows. This after two pushModal failures. Ie the timestamp we send in pushModal is getting out of the range accepted by XIGrabDevice (should be later than last grab device event and earlier than X server time). First is a move of this._createSwitcher after the Main.pushModal test. Ie the actor is attached to the stage after this test, move the _createSwitcher that set the _switcherList avoid this list from dangling. Then tabPopup.destroy (here tabPopup is an AppSwitcherPopup), does not call SwitcherPopup.destroy handler (which AppSwitcherPopup extendeds). This hack replace the call to AppSwitcherPopup.destroy by the content of the SwitcherPopup.destroy method. This prevent later calls to the AppSwitcherPopup._allocate which depends on _switcherList which is null as per the failure of the pushModal that aborted the SwitcherPopup show.
Created attachment 233685 [details] [review] switcherPopup: Hide the top level actor on _init destroy() checks if we are visible to decide wether to destroy the actor immediately or after a fade out animation. Since actors are visible by default it would always end up destroying only after the animation time.
Created attachment 233686 [details] [review] switcherPopup: Create the SwitcherList only after getting modal If pushModal() fails, SwitcherList.actor would never get parented but we would still try to call methods on it in _allocate() which would fail. -- Thanks for investigating. I think this is the correct fix here. The other patch also avoids the crash but is more of an optimization.
Review of attachment 233686 [details] [review]: I don't like this fix. http://git.gnome.org/browse/gnome-shell/tree/js/ui/windowManager.js#n628 If we can't construct the tab popup, we immediately destroy it. The issue is that destroy tries to fade, which calls _allocate. The correct fix is to not fade the actor if it's not parented, not to do things like this.
Review of attachment 233685 [details] [review]: ::: js/ui/switcherPopup.js @@ +55,3 @@ Main.uiGroup.add_actor(this.actor); + this.actor.hide(); Put visible: false in the actor props, so we don't cause a bunch of state transitions to mapped and back when we add the actor above.
Created attachment 233690 [details] [review] switcherPopup: Remove the needless _popModal() method popModal() gets called automatically for us when the actor is destroyed. We only need to call is explicitly when fading out. -- Here's another possible cleanup. Jasper pointed it on IRC.
Created attachment 233692 [details] [review] switcherPopup: Hide the top level actor on _init -- Rebased.
Created attachment 233693 [details] [review] switcherPopup: Remove the needless _popModal() method -- Rebased.
Review of attachment 233690 [details] [review]: I was talking about the hasModal variable, specifically. I think it makes sense to tie the modality of the actor to its visibility. I don't think we should be relying on popModal-on-destroy; we should always try to push and pop our modality explicitly.
Review of attachment 233693 [details] [review]: See other review.
(In reply to comment #8) > I was talking about the hasModal variable, specifically. I think it makes sense > to tie the modality of the actor to its visibility. That's what this patch does. If you get rid of _haveModal you also need to remove the call to _popModal from _onDestroy or you'll get 'incorrect pop'. And by then we might just as well fold it in the visible branch of destroy() and you get this patch. > I don't think we should be relying on popModal-on-destroy; we should always try > to push and pop our modality explicitly. I don't see why. It would clearly be a bug if the destruction of an actor being tracked by Main.pushModal() didn't result in a popModal().
At least with the move of _createSwitcher after the pusModal test fixing the plain crash but breaks on _allocate: the _allocate for all the existing AppSwitcherPopup happens on the first successfull show of one of them. if I manage to produce 4 failed show, the fifth will call _allocate for all of them on SwitcherPopup.show this.actor.get_allocation_box. Also the SwitcherPopup destroy I am still not understanding its behaviour. Ie it contains a fade but I put my log() before this tweener fade and still this destroy handler is never called (only the _onDestroy).
(In reply to comment #10) > I don't see why. It would clearly be a bug if the destruction of an actor being > tracked by Main.pushModal() didn't result in a popModal(). Because I think that every appearance of a pushModal should be paired by a popModal in the code. Relying on the destruction is fine to make sure things don't break, but not something we say "oh, we're OK, destruction will manage modality for us". With all of the very real breakage around pushModal/popModal and "incorrect pop" (like this bug), now is not the time to try and implement false-DWIM interfaces that will tie modality to an actor that we can pass around all over the place. We need to be sure we get it right, and explicitly calling pushModal/popModal and tracing the code path through is the best way to do that.
(In reply to comment #12) > (In reply to comment #10) > > I don't see why. It would clearly be a bug if the destruction of an actor being > > tracked by Main.pushModal() didn't result in a popModal(). > > Because I think that every appearance of a pushModal should be paired by a > popModal in the code. Relying on the destruction is fine to make sure things > don't break, but not something we say "oh, we're OK, destruction will manage > modality for us". > > With all of the very real breakage around pushModal/popModal and "incorrect > pop" (like this bug), now is not the time to try and implement false-DWIM > interfaces that will tie modality to an actor that we can pass around all over > the place. We need to be sure we get it right, and explicitly calling > pushModal/popModal and tracing the code path through is the best way to do > that. Ok, I understand your concern. But, look at the code paths here. If we are able to pushModal() then this.actor.visible is certainly set to true, thus, we only really need to popModal() in this single place.
Review of attachment 233692 [details] [review]: OK.
Attachment 233692 [details] pushed as f2cbc31 - switcherPopup: Hide the top level actor on _init