revert changes to the destruction of systray from the mainframe by seanbudd · Pull Request #12243 · nvaccess/nvda

seanbudd · 2021-03-29T00:57:44Z

Link to issue number:

Summary of the issue:

System tray icons are not cleaned properly when exiting/restarting NVDA. This is due to an incorrect assumption in #12183 that the manual destruction of the system tray icon was no longer necessary.

Description of how this pull request fixes the issue:

Revert the changes in #12183 that manually destroyed the system tray icon and add logging to the process.

Garbage collection is not enough with wx; Destroy() must eventually be called on all objects. wx does handle destruction of children, but the system tray icon and its menu are not children, so we need to handle them explicitly. 8ab9d48

Testing strategy:

Manually test the steps in #12238

Ensure the silent installation process doesn't crash (ie a regression on what #12183 fixed for #12153)

Known issues with pull request:

None

Change log entry:

None, fixes regression

Code Review Checklist:

feerrenrut · 2021-03-29T01:59:47Z

It would be good if we had a handle on when it was necessary to manually call destroy on children in wx. Have you seen this in the docs?

feerrenrut · 2021-03-29T02:09:49Z

So the Window deletion overiview says:

Child windows are deleted from within the parent destructor. This includes any children that are themselves frames or dialogs, so you may wish to close these child frame or dialog windows explicitly from within the parent close handler.

seanbudd · 2021-03-29T03:50:50Z

It would be good if we had a handle on when it was necessary to manually call destroy on children in wx. Have you seen this in the docs?

It isn't documented anywhere officially as far as I can tell, just forums/blogs like the following:

The original commit which introduced this had a good message 8ab9d48:

Garbage collection is not enough with wx; Destroy() must be called on all objects. wx does handle destruction of children, but the system tray icon and its menu are not children, so we need to handle them explicitly. The application cannot terminate if objects are not destroyed. This fixes the issue where NVDA refuses to exit. There are still many cases in settingsDialogs that don't call Destroy() that will cause this bug now that they are not children of the main frame, so these need to be fixed also.

The cases mentioned surrounding Dialogs should be fixed with #12183

feerrenrut · 2021-03-29T04:39:46Z

Right, thanks for looking into this. It seems the key is:

wx does handle destruction of children, but the system tray icon and its menu are not children,

Would it make sense to ensure that they are children?

seanbudd · 2021-03-29T04:51:06Z

wx.adv.TaskBarIcon can't be a child of our main frame window as it doesn't inherit from wx.Window and the children of Windows must inherit from wx.Window. This is the same reason why we can only call Destroy.

This is also all the same for our taskbar menu wx.Menu https://wxpython.org/Phoenix/docs/html/wx.Menu.html

feerrenrut · 2021-03-29T04:55:37Z

Ah, good find. In this case, perhaps we can add these to a list of "must be explicitly destroyed on exit" controls on the App of somewhere appropriate and have it iterated over calling destroy? I'm thinking that if we call this out on the app that there are certain types of controls that need special handling, how to differentiate them from the others, and a mechanism for destroying them on shutdown it will be easier to understand in the future.

seanbudd · 2021-03-29T05:02:22Z

I think the reason why they were being destroyed from where they are currently is because the same class is initialising them.

I could move it all to safeAppExit, but you can see the dilemma of where this code belongs.

AppVeyorBot · 2021-03-29T05:11:25Z

System test failure
Build (for testing PR)

See test results for failed build of commit bf23c9e0a2

AppVeyorBot · 2021-03-29T05:35:25Z

System test failure
Build (for testing PR)

See test results for failed build of commit bf23c9e0a2

AppVeyorBot · 2021-03-30T00:17:58Z

Install process crashed
Build (for testing PR)

See test results for failed build of commit 7c462fa831

AppVeyorBot · 2021-03-30T00:42:32Z

Install process crashed
Build (for testing PR)

See test results for failed build of commit c44c28a0d6

feerrenrut

Could you also add a note into the "safe exit" function you added previously to point out that wx objects that don't inherit from wx.Window (eg sysTrayIcon, Menu) need to be manually destroyed. That function is a logical place to look for an explanation if NVDA has problems during exit in the future.

feerrenrut · 2021-03-30T01:34:55Z

 	def _onDestroy(self, evt):
-		evt.Skip()  # Allow other handlers to process this event.


Why should evt.Skip() be removed? There shouldn't be any other parent controls, but if there were?

I've added a comment that makes the reason for this more explicit

AppVeyorBot · 2021-03-30T01:59:44Z

System test failure
Build (for testing PR)

See test results for failed build of commit 1687ecb41b

AppVeyorBot · 2021-03-30T02:28:13Z

Install process crashed
Build (for testing PR)

See test results for failed build of commit 1687ecb41b

AppVeyorBot · 2021-03-30T03:56:18Z

System test failure
Build (for testing PR)

See test results for failed build of commit cbf3bc3037

feerrenrut · 2021-03-31T05:54:33Z

+		# wx destroys child Windows automatically but `wx.adv.TaskBarIcon` is not a window
+		# so it must be set to be destroyed when destroying our main frame window (#12243)
+		log.debug(f"destroying systray icon")
+		wx.CallAfter(self.sysTrayIcon.Destroy)


Why is callAfter necessary here?

I can't find this in any official documentation, but when doing the original safeAppExit PR I found several recommendations in forums to use this method as it queues destruction events safer. Calling Destroy directly here causes a crash on the installation process.

feerrenrut · 2021-03-31T05:54:35Z

 				self.Hide()

+	def _onDestroy(self, evt: wx.EVT_WINDOW_DESTROY):
+		self.Skip(skip=False)  # we should block the destruction events until the sysTrayIcon is destroyed


"until" makes it sound like this logic will change at some point.

Hopefully I'm not alone in finding skip hard to memorize, I wish they had named it in the affirmative eg handled.

The docs say:

This method can be used inside an event handler to control whether further event handlers bound to this event will be called after the current one returns.

Without Skip (or equivalently if Skip(false) is used), the event will not be processed any more. If Skip(true) is called, the event processing system continues searching for a further handler function for this event, even though it has been processed already in the current handler.

In general, it is recommended to skip all non-command events to allow the default handling to take place. The command events are, however, normally not skipped as usually a single command such as a button click or menu item selection must only be processed by one handler.

This sounds like it will do the opposite of what the comment here says.

I've just noticed that this calls self.Skip rather than evt.Skip I assume this is a mistake?

I did find this hard to parse. I'm not sure what the default handler does for a wx.EVT_WINDOW_DESTROY event, but this is a command event so this behaviour is recommended and crashes otherwise. This event fires just before self.Destroy - which is what the comment was trying to allude to in a misleading way. I think the comment where the event handler is being bound should makes this clear anyway.

And yes, the self/evt typo was a mistake.

AppVeyorBot · 2021-04-06T00:35:52Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit a4fd79176c

AppVeyorBot · 2021-04-06T01:19:10Z

Install process crashed
Build (for testing PR)

See test results for failed build of commit 324f5a1724

lukaszgo1 · 2021-04-06T19:59:30Z

+		evt.Skip(skip=False)  # blocks other handlers as wxCommandEvents must only be processed by one handler
+		# wx destroys child Windows automatically but `wx.adv.TaskBarIcon` is not a window
+		# so it must be set to be destroyed when destroying our main frame window (#12243)
+		log.debug(f"destroying systray icon")


Why you're using f-string here?

lukaszgo1 · 2021-04-06T19:59:59Z

 			wx.CallAfter(window.EndModal, wx.ID_CLOSE_ALL)
 		if isinstance(window, MainFrame):
-			log.info(f"destroying main frame during exit process")
+			log.debug(f"destroying main frame during exit process")


Same here w.r.t f-string being unnecessary.

lukaszgo1 · 2021-04-06T20:00:20Z

+	def Destroy(self):
+		# wx.Windows destroy child Windows automatically but wx.Menu and TaskBarIcon don't inherit from
+		# wx.Window. The menu must be manually destroyed when destroying our system tray icon (#12243)
+		log.debug(f"destroying systray menu during exit process")


And one more

AppVeyorBot · 2021-04-09T09:32:53Z

Install process crashed
Build (for testing PR)

See test results for failed build of commit c127e088d9

seanbudd · 2021-04-11T23:48:12Z

closing as it appears this can't be fixed without inducing a crash

revert changes to the destruction of systray from the mainframe

b5547cb

seanbudd self-assigned this Mar 29, 2021

seanbudd added this to the 2021.1 milestone Mar 29, 2021

seanbudd requested a review from feerrenrut March 29, 2021 01:14

add explanation comments

03c6d3d

seanbudd added 2 commits March 29, 2021 17:03

Merge branch 'master' into fix-12238

2888f55

add comments at untracked object creation

9e9c6f1

feerrenrut suggested changes Mar 29, 2021

View reviewed changes

Comment thread source/gui/__init__.py Outdated

Comment thread source/gui/__init__.py Outdated

use safer event bind where possible

9f3240d

remove evt skip

a84dece

feerrenrut reviewed Mar 30, 2021

View reviewed changes

seanbudd added 2 commits March 30, 2021 12:42

stop and resume event propagation

7290f17

add comment to safeappexit

a60cf4d

seanbudd marked this pull request as draft March 30, 2021 02:09

use callafter to prevent crash

362d05e

seanbudd force-pushed the fix-12238 branch from c4ed68c to 362d05e Compare March 30, 2021 03:43

seanbudd marked this pull request as ready for review March 30, 2021 05:11

feerrenrut suggested changes Mar 31, 2021

View reviewed changes

seanbudd added 4 commits April 1, 2021 15:50

Merge branch 'master' into fix-12238

b063e57

fix typo and comment for evt skip

6e2b3c7

add better comments

593d89f

Merge branch 'master' into fix-12238

b5caece

fix formatter issues

4537051

seanbudd marked this pull request as draft April 6, 2021 01:49

lukaszgo1 reviewed Apr 6, 2021

View reviewed changes

seanbudd added 2 commits April 8, 2021 15:37

Merge branch 'master' into fix-12238

4c5de0a

Merge branch 'master' into fix-12238

aac13ad

seanbudd closed this Apr 11, 2021

seanbudd deleted the fix-12238 branch April 11, 2021 23:48

		def _onDestroy(self, evt):
		evt.Skip() # Allow other handlers to process this event.

Uh oh!

Uh oh!

Conversation

seanbudd commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

Uh oh!

feerrenrut commented Mar 29, 2021

Uh oh!

feerrenrut commented Mar 29, 2021

Uh oh!

seanbudd commented Mar 29, 2021

Uh oh!

feerrenrut commented Mar 29, 2021

Uh oh!

seanbudd commented Mar 29, 2021

Uh oh!

feerrenrut commented Mar 29, 2021

Uh oh!

seanbudd commented Mar 29, 2021

Uh oh!

AppVeyorBot commented Mar 29, 2021

Uh oh!

AppVeyorBot commented Mar 29, 2021

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Mar 30, 2021

Uh oh!

AppVeyorBot commented Mar 30, 2021

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Mar 30, 2021

Uh oh!

AppVeyorBot commented Mar 30, 2021

Uh oh!

AppVeyorBot commented Mar 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 6, 2021

Uh oh!

AppVeyorBot commented Apr 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Apr 9, 2021

Uh oh!

seanbudd commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seanbudd commented Mar 29, 2021 •

edited

Loading