Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate Carbon TTrayIcon leak #17

Closed
traumschule opened this issue Nov 29, 2017 · 8 comments
Closed

Investigate Carbon TTrayIcon leak #17

traumschule opened this issue Nov 29, 2017 · 8 comments

Comments

@traumschule
Copy link

traumschule commented Nov 29, 2017

To get the TrayIcon memory leak fixed in 1.8 the report should get more attention by adding more specific information like a trace or an example how to reproduce. Necessary information should be available via Leakview.

May leaks be ignored?
How to detect memory leaks in Lazarus
How to properly release memory
good example report with trace and patch

@traumschule traumschule mentioned this issue Nov 29, 2017
4 tasks
@davidbannon davidbannon added this to the v0.1 alpha release milestone Nov 30, 2017
@davidbannon
Copy link
Member

Some answers -

May leaks be ignored? - No, absolutely not.
How to detect memory leaks in Lazarus - easy, turn on heaptr, launch from console
How to properly release memory - depends on how it was allocated
good example report with trace and patch - I logged a very clear and simple example, indications are I cannot expect any help however, its a patch or be ignored. I'm sorry, I may have dragged Tomboy in to Pascal unadvisedly, I thought it was a more responsive project than that.

http://forum.lazarus.freepascal.org/index.php/topic,38601.msg267243.html#new

@traumschule
Copy link
Author

(the questions link to forum threads that answer them)
Although it may seem, as there is no support, I think there are still some things we could do to improve the speed it gets fixed (not by us, but by Carbon developers):

  • the bug report should appear in the Carbon bugtracker and should contain all necessary info for a developer to find the origin of the problem to trace it back to a line of code at best
  • the thread about the leak is in the Mac support area but not the Carbon folder - can you move it?

@davidbannon
Copy link
Member

Just to get on record ...
This particular problem is upsteam from tomboy-ng and has not, so far attracted any attention from people far more suited to the task than me. The difficulty is debugging RTL code.
In my install of Lazarus on the Mac, source code line numbers are not reported when an exception occurs or, importantly here, where a memory leak occurs. Advice in the forum suggests that this is because debug symbols are not included in the RTL and LCL binaries - makes sense but tat would also be the case under Linux and Windows too ?
Anyway, to get that debug info, seems I must rebuild RTL with debug info. As 1.8 is now just released, it makes sense to do it with that rather than sticking with a (not really suspect) RC. So, time .....

@davidbannon
Copy link
Member

For the record it was suggested that to get offending line numbers on the Mac, it was necessary to recompile the RTL with debugging info. http://forum.lazarus.freepascal.org/index.php/topic,38917.0.html
This proved to not be the case. https://forum.lazarus.freepascal.org/index.php/topic,39255.0.html

Its now agreed the problem (lack of suitable debugging info) is a deep seated one with the Mac FPC/RTL see https://bugs.freepascal.org/view.php?id=32775

Suggestions that the actual problem (leaks in TrayIcon) may never be fixed, its a Carbon issue and Carbon is headed fro the scrap heap. I have now made some changes to tomboy-ng so it builds and works under Cocoa but there are more leaks there. Same underlying issue, debugging tools don't report the source of leaks ! QT is reported to be better supported but our design goal here is to have a dependency free install.
There seems quite a lot of Cocoa activity there now so maybe it will get better soon.

@davidbannon
Copy link
Member

Some conclusions about how this leak affects this application.
The leak occurs when we call TrayIcon.InternalUpdate. It occurs whether or not we have made changes to the menu content so that implies there are some things we can do to minimise the problem

  • Only call InternalUpdate when we have made a change to menu
  • Don't update menu more than we must, so, perhaps Mac users can see a non sorted list of 10 most recently edited notes ? Less convenient but, depending on usage patterns, perhaps a substantial improvement.
  • Provide a setting that allows Mac users to turn off the recent menu, really solves the problem. There is an initial leak of 200bytes but that does not grow as the app is used.

Each call to InternalUpdate seems to leak about 80 bytes. Not a lot but it is a growing leak !

I don't like any of the above but do need a short term fix. Long term, the underlying Mac problem needs to be fixed but there does not appear to be a enthusiasm to do so, Apple's active discouragement of non endorsed products is an issue. After all, last thing Apple would support is a cross platform application...

1 similar comment
@davidbannon
Copy link
Member

Some conclusions about how this leak affects this application.
The leak occurs when we call TrayIcon.InternalUpdate. It occurs whether or not we have made changes to the menu content so that implies there are some things we can do to minimise the problem

  • Only call InternalUpdate when we have made a change to menu
  • Don't update menu more than we must, so, perhaps Mac users can see a non sorted list of 10 most recently edited notes ? Less convenient but, depending on usage patterns, perhaps a substantial improvement.
  • Provide a setting that allows Mac users to turn off the recent menu, really solves the problem. There is an initial leak of 200bytes but that does not grow as the app is used.

Each call to InternalUpdate seems to leak about 80 bytes. Not a lot but it is a growing leak !

I don't like any of the above but do need a short term fix. Long term, the underlying Mac problem needs to be fixed but there does not appear to be a enthusiasm to do so, Apple's active discouragement of non endorsed products is an issue. After all, last thing Apple would support is a cross platform application...

@davidbannon
Copy link
Member

The 0.12 release has some changes made, to mac only, that attempt to cut down on the number of times we update the RecentMenu items. Don't like it, its ugly and does not fully fix the problem but that might be all we see until we can move over to the Cocoa widget set. I see some progress happening there but its got a way to go.

Last time I checked, we could compile to Cocoa but there were even more leaks and several other issues. But, as I said, a lot of effort is going into it right now.

@davidbannon
Copy link
Member

OK, Mac now does not use the leaky TrayIcon and, it seems is now leak free. Bonus is Mac now has a much more Mac like feel. If you like that sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants