Fix #109770: WM: Memory leak of timers for file browser report #109603

Iliya Katushenock · 2023-07-01T23:45:11+02:00

Iliya Katushenock commented

2023-07-01 23:45:11 +02:00

After opening a new project through the file browser, memory leaks appear.

Error: Not freed memory blocks: 2, total unfreed memory 0.000183 MB
window timer len: 96 000002C62956A5D8
window timer len: 96 000002C658A0C678

After opening a new project through the file browser, memory leaks appear. ``` Error: Not freed memory blocks: 2, total unfreed memory 0.000183 MB window timer len: 96 000002C62956A5D8 window timer len: 96 000002C658A0C678 ```

Iliya Katushenock added 1 commit 2023-07-01 23:45:23 +02:00

a6f8a66fcd init

Iliya Katushenock added this to the User Interface project 2023-07-01 23:47:03 +02:00

Iliya Katushenock requested review from Campbell Barton 2023-07-01 23:47:13 +02:00

Harley Acheson commented

2023-07-03 18:22:53 +02:00

Note that I haven't looked deeply into this but reports->reporttimer is a pointer to a wmTimer that is owned by a wmWindowManager. So freeing an item like this could cause unrelated issues with the wm->timers list.

So if this leak is from the call to BKE_reports_clear in block_create__close_file_dialog you could remove that timer there like:

WM_event_remove_timer(wm, nullptr, reports->reporttimer);

Note that I haven't looked deeply into this but `reports->reporttimer` is a pointer to a `wmTimer` that is owned by a wmWindowManager. So freeing an item like this could cause unrelated issues with the `wm->timers` list. So if this leak is from the call to `BKE_reports_clear` in `block_create__close_file_dialog` you could remove that timer there like: ``` WM_event_remove_timer(wm, nullptr, reports->reporttimer); ```

Campbell Barton commented

2023-07-04 02:36:06 +02:00

My sense in the same as Harley's, can you confirm the timer isn't referenced elsewhere? - if so, note this in the code comments, else - remove any references to the timer before freeing.

Campbell Barton commented

2023-07-04 04:59:18 +02:00

After opening a new project through the file browser, memory leaks appear.

Can you give steps to redo the bug, tested with 5c4510d39ecf84d71d6afd15329dafda4fcc8556 and I can't redo this leak.

> After opening a new project through the file browser, memory leaks appear. Can you give steps to redo the bug, tested with `5c4510d39ecf84d71d6afd15329dafda4fcc8556` and I can't redo this leak.

Campbell Barton requested changes 2023-07-04 04:59:59 +02:00

Campbell Barton left a comment

The ownership of reports->reporttimer needs to be clarified.

The ownership of `reports->reporttimer` needs to be clarified.

Harley Acheson commented

2023-07-04 18:30:43 +02:00

Looking a bit deeper I think I would also need a way to reproduce the error.

This PR is only freeing a timer and those are all owned by a wmWindowManager (wm->timers). My guess is that you are seeing a leak caused by the WM_report_banner_show in wm_block_file_close_save. Timers are only in that list while they are active and they generally don't live long. So my gut feeling is that you might have a situation where a banner is being shown when window_manager_blend_read_data is called. wm_close_and_free later calls wm_reports_free

But it looks like wm_reports_free makes the same mistake as I did earlier in this thread. It calls WM_event_remove_timer on wm->reports.reporttimer but that function only marks the timer for deletion (adds a WM_TIMER_TAGGED_FOR_REMOVAL flag). It should also call wm_window_delete_removed_timers to actually free it. Note that WM_event_remove_timer does however free timer-customdata if it exists, something missing from this PR.

So I'd love a sample blend and instructions on how to recreate this leak. But it if it is hard to catch and reproduce you could consider adding wm_window_delete_removed_timers to the end of wm_reports_free and see if it fixes it.

Looking a bit deeper I think I would also need a way to reproduce the error. This PR is only freeing a timer and those are all owned by a wmWindowManager (wm->timers). My guess is that you are seeing a leak caused by the `WM_report_banner_show` in `wm_block_file_close_save`. Timers are only in that list while they are active and they generally don't live long. So my gut feeling is that you might have a situation where a banner is being shown when `window_manager_blend_read_data` is called. `wm_close_and_free` later calls `wm_reports_free` But it looks like `wm_reports_free` makes the same mistake as I did earlier in this thread. It calls `WM_event_remove_timer` on wm->reports.reporttimer but that function only marks the timer for deletion (adds a WM_TIMER_TAGGED_FOR_REMOVAL flag). It should also call `wm_window_delete_removed_timers` to actually free it. Note that `WM_event_remove_timer` does however free timer-customdata if it exists, something missing from this PR. So I'd love a sample blend and instructions on how to recreate this leak. But it if it is hard to catch and reproduce you could consider adding `wm_window_delete_removed_timers` to the end of `wm_reports_free` and see if it fixes it.

Iliya Katushenock commented

2023-07-04 23:51:40 +02:00

More complete playback steps:

Disable all addons.
Open any project with a file browser.

How I see the leak stack:
WM_event_add_timer -> WM_report_banner_show -> wm_file_read_report -> wm_file_read_post.

More complete playback steps: 1. Disable all addons. 2. Open any project with a file browser. How I see the leak stack: `WM_event_add_timer` -> `WM_report_banner_show` -> `wm_file_read_report` -> `wm_file_read_post`.

Iliya Katushenock added 2 commits 2023-07-05 00:08:16 +02:00

54c1d70739 Merge branch 'main' into tmp_fix_mem_leak

7f51735ff7 progress

Iliya Katushenock commented

2023-07-05 00:08:43 +02:00

is owned by a wmWindowManager

Okay, that simplifies things then. I've updated the fix to be much more correct.

> is owned by a wmWindowManager Okay, that simplifies things then. I've updated the fix to be much more correct.

Iliya Katushenock requested review from Campbell Barton 2023-07-05 00:10:07 +02:00

Harley Acheson commented

2023-07-05 00:29:16 +02:00

More complete playback path:

Disable all addons.

Open any project with a file browser.

How I see the leak stack:
WM_event_add_timer -> WM_report_banner_show -> wm_file_read_report -> wm_file_read_post.

You are definitely missing steps to create this, as this does not include anything that would cause WM_report_banner_show to be called.

Note that your list is in backwards order. You were probably looking at a stack list with the last item on top. so it was in wm_file_read_post that wm_file_read_report was called, which called WM_report_banner_show, which called WM_event_add_timer.

If you look at the source in wm_file_read_post where it calls wm_file_read_report only addons are loaded, which conflicts with your "Disable all addons" step. And you wouldn't get to WM_report_banner_show from there unless there is an error to show. Do you see an error message on the status bar?

> More complete playback path: > 1. Disable all addons. > 2. Open any project with a file browser. > > How I see the leak stack: > `WM_event_add_timer` -> `WM_report_banner_show` -> `wm_file_read_report` -> `wm_file_read_post`. You are definitely missing steps to create this, as this does not include anything that would cause `WM_report_banner_show` to be called. Note that your list is in backwards order. You were probably looking at a stack list with the last item on top. so it was in wm_file_read_post that wm_file_read_report was called, which called WM_report_banner_show, which called WM_event_add_timer. If you look at the source in wm_file_read_post where it calls wm_file_read_report only addons are loaded, which conflicts with your "Disable all addons" step. And you wouldn't get to WM_report_banner_show from there unless there is an error to show. Do you see an error message on the status bar?

Harley Acheson commented

2023-07-05 00:37:35 +02:00

Okay, that simplifies things then. I've updated the fix to be much more correct.

If your PR as it is now fixes the problem, then try replacing your

BLI_freelistN(&wm->timers);

with

wm_window_delete_removed_timers(wm);

The wm_reports_free line above these clears the reports, sets the wm->reports.reporttimer as deletable, but does not free it. wm_window_delete_removed_timers will then free it.,

> Okay, that simplifies things then. I've updated the fix to be much more correct. If your PR as it is now fixes the problem, then try replacing your ``` BLI_freelistN(&wm->timers); ``` with ``` wm_window_delete_removed_timers(wm); ``` The `wm_reports_free` line above these clears the reports, sets the wm->reports.reporttimer as deletable, but does not free it. wm_window_delete_removed_timers will then free it.,

Iliya Katushenock commented

2023-07-06 02:36:07 +02:00

You are definitely missing steps to create this, as this does not include anything that would cause WM_report_banner_show to be called.

To me, commenting wm_file_read_report' call resolve leak.

You are definitely missing steps

Rechecked. Addons don't seem to affect this.
Also, I will attach an archive with a folder and a file. Opening the file through the file browser results in a leak for me.

Note that your list is in backwards order.

I'm not sure if this matters, so I listed the stack in order of leak up, not call order..

> You are definitely missing steps to create this, as this does not include anything that would cause WM_report_banner_show to be called. To me, commenting `wm_file_read_report`' call resolve leak. > You are definitely missing steps Rechecked. Addons don't seem to affect this. Also, I will attach an archive with a folder and a file. Opening the file through the file browser results in a leak for me. > Note that your list is in backwards order. I'm not sure if this matters, so I listed the stack in order of leak up, not call order..

okn.rar

93 KiB

Campbell Barton commented

2023-07-06 03:46:44 +02:00

Could you please open a report with steps to redo this issue, I tried multiple times and couldn't redo it & having to figure out why in the review makes things more difficult to follow.

Iliya Katushenock changed title from ~~Fix: WM: Memory leak of timers for file browser report~~ to Fix #109770: WM: Memory leak of timers for file browser report

2023-07-09 14:28:11 +02:00

Iliya Katushenock commented

2023-07-09 14:28:26 +02:00

@Harley @ideasman42 #109770

Campbell Barton requested changes 2023-07-10 13:03:40 +02:00

source/blender/windowmanager/intern/wm.c

						
				@ -606,6 +606,7 @@ void wm_close_and_free(bContext *C, wmWindowManager *wm)

				  WM_drag_free_list(&wm->drags);

				  wm_reports_free(wm);

				  BLI_freelistN(&wm->timers);

Campbell Barton commented

2023-07-10 13:02:47 +02:00

Timers can themselves have allocated data, this should be replaced by a function which frees timer custom data (see: WM_TIMER_NO_FREE_CUSTOM_DATA).

Suggest adding WM_event_timer_free_data(...) which WM_event_remove_timer can call too.

Timers can themselves have allocated data, this should be replaced by a function which frees timer custom data (see: `WM_TIMER_NO_FREE_CUSTOM_DATA`). Suggest adding `WM_event_timer_free_data(...)` which `WM_event_remove_timer` can call too.

Campbell Barton referenced this issue from a commit

2023-07-11 04:54:43 +02:00

Fix #109953: Timers without an associated window leak memory on exit

Campbell Barton commented

2023-07-11 05:00:16 +02:00

Committed a fix 6be0c90eab.

Note that the memory leak is quite harmless as it was only a leak on exit and freeing the memory just suppresses the warnings.
There was however a bug where reports which didn't set the window were suppressed (resolved by 8a73c36643).

There is a more general issue that timers without windows aren't freed while Blender runs.
It's up to the owner to free them (which may be acceptable) but seems error prone as add-ons could for e.g. add timers without windows and never remove them. We might for e.g. want timers to be freed when a new file is loaded for e.g.
This is more an API design issue though.

Committed a fix 6be0c90eab2ad8e59a2f59d0fd9168f2fd9488ca. Note that the memory leak is quite harmless as it was only a leak on exit and freeing the memory just suppresses the warnings. There was however a bug where reports which didn't set the window were suppressed (resolved by 8a73c36643c8e29ab250e367436536a976a1b753). There is a more general issue that timers without windows aren't freed while Blender runs. It's up to the owner to free them (which may be acceptable) but seems error prone as add-ons could for e.g. add timers without windows and never remove them. We might for e.g. want timers to be freed when a new file is loaded for e.g. This is more an API design issue though.

👍 1

Campbell Barton closed this pull request

2023-07-11 05:00:18 +02:00

Iliya Katushenock deleted branch tmp_fix_mem_leak

2023-07-11 11:32:41 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #109770: WM: Memory leak of timers for file browser report #109603

Pull request closed