Currently we manually keep track of the lifecycle of GDI resources and delete them manually. Instead we should use a reference counted object that releases the handle as soon as we don't need it any more. Tomasz already introduced such a concept to fix one of our GDI handle leaks with https://cgit.freedesktop.org/libreoffice/core/commit/?id=dae61482df7ae540a1fb8feefbb92b5e7238444d That commit introduces a scoped version and we need additionally a reference counted version. These correspond to scoped_ptr and shared_ptr in C++.
Moving to NEW
(In reply to Markus Mohrhard from comment #0) > Currently we manually keep track of the lifecycle of GDI resources and > delete them manually. Instead we should use a reference counted object that > releases the handle as soon as we don't need it any more. > > Tomasz already introduced such a concept to fix one of our GDI handle leaks > with > https://cgit.freedesktop.org/libreoffice/core/commit/ > ?id=dae61482df7ae540a1fb8feefbb92b5e7238444d > That commit introduces a scoped version and we need additionally a reference > counted version. These correspond to scoped_ptr and shared_ptr in C++. Hi I am an undergraduate student new to open source development and Libre Office and an intermediate knowledge of C++ (I applied for GSoC). I want to tackle this bug but have a few questions. I have not used shared_ptr, scoped_ptr or anything to do with reference counting before. - The example mentioned introduced the class ScopedHDC that deletes the object for you.Is this fix just for HDC objects? Or are there other gdi objects? I am not familiar with gdi. I see other objects such as HBITMAP, HBRUSH, HFONT that are all manually destroyed using DeleteObject(). - Is this fix asking for an alternative to ScopedHDC class using shared pointers or scoped pointers? - For shared_ptr, my understanding is that you could create a shared_ptr object using: std::shared_ptr<HDC> hNewDC = std::make_shared<HDC>( CreateCompatibleDC(hDC) ); then use it as you would a regular pointer without having to call DeleteDC(hNewDC) once your done. Is my understanding correct?
I suspect you will need to create a class that does the DeleteDC in its destructor to make this work =) Actually the ScopedHDC from that patch would work just fine I think when wrapped in a shared_ptr - but perhaps it should be renamed - Tomaz - thoughts ? =)
std::shared_ptr<ScopedHDC> should work, so I would just use that where appropriate. Anyway - chromium uses similar code to ScopedHDC [1] - it also uses same principle for other resources. [1] https://github.com/mozilla/gecko-dev/blob/86897859913403b68829dbf9a154f5a87c4b0638/ipc/chromium/src/base/scoped_handle_win.h
I can take care of this. I am new to the codebase, where can I get a more precise idea or a list of the GDI resources?
A polite ping, still working on this bug?
I was hoping for more information on it, i.e. a list of these GDI resources, but I can still do it. I am on holidays now but if it is not urgent I'll take care of it in the next few weeks.
I won't have time to deal with this anymore for a while. I unassigned myself in case someone else wants to pick it up
Hi Guys, My turn to fight with this task! This week: * [ ] dig a little bit into codebase * [ ] explore the source of the leaking part Next week: * [ ] compile LO for Win * [ ] introduce shared_ptr * [ ] realize how to test the solution: * [ ] add some unit tests? * [ ] test on the target platform? I'm very appreciate any advises about the testing part.
@Michael, @Noel, I'm adding you to this bugs so you can follow Dmitriy's work in the following weeks...
Hi Guys! Last week I didn't have a lot of time for coding. As a result I spent some time for reading the vcl/win/gid code base. While reading I made some notes that I want to share with you. # Initial task The initial task was about introducing some kind of shared_ptr for managing the resources created via CreateCompatibleDC, similar to ScopedHDC [1]. # Preparation patches * Reduce the scope of manual resource handling by extracting the low-level stuff from the high-level functions: * Extract SysColorList deletion [2] * ImplFreeSalGDI into smaller low-level routines. * Maybe something else. * Reduce copy-paste of low-level stuff spread across multiple files. These patches should be very stupid and contain only mechanical changes. It allows me to carefully replace new/delete, new[]/delete[] and don't break something. # Rework smart pointers * Remove ScopedHDC. I understand why ScopedHDC was introduced. But it's very limited and allows to manage only resources allocated via CreateCompatibleDC. I want to replace it via std::unique_ptr via custom deleter. More over, we have a lot of other places where we need scoped_ptr but with another delete method in the descructor. * Use std::unique_ptr instead of std::shared_ptr. I saw some places where std::shared_ptr is used as some kind of scoped_ptr. * I won't introduce any new SharedPtrDC. I'd rather use std::shared_ptr wherever possible. [1] https://cgit.freedesktop.org/libreoffice/core/commit/?id=dae61482df7ae540a1fb8feefbb92b5e7238444d [2] https://gerrit.libreoffice.org/#/c/64591/ FYI: @Michael, @Noel CC: @Xisco
Seems like a reasonable plan; one issue: > * I won't introduce any new SharedPtrDC. I'd rather use std::shared_ptr > wherever possible. Having a wrapper class potentially makes this simpler and less error prone. If we pass around std::shared_ptr<Foo> everywhere, then quite probably some call sites forget to set the extra custom deleter inside the shared_ptr at construction time. If we have a wrapper class - then we have compile-time type-checking to help enforce that. Then again - if most of the resources are constructed at one place - then, perhaps it's only a few call sites - so it might work. Then again - it is un-expected that releasing a shared_ptr calls a custom deletion method assigned at construction time - most people assume its just a 'delete' I think. Anyhow - I look forward to seeing the patch =) We can also use make_shared so each is as efficient as the other - but up to you really.
(In reply to Michael Meeks from comment #12) > > Having a wrapper class potentially makes this simpler and less error prone. Note that a wrapper class can be as simple as a typedef: using UniquePtrDC = std::unique_ptr<DC, DCDeletor>;
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/216cbcec6b0361986593e49837f8420e84032d6f%5E%21 tdf#107792 vcl: simplify SetLineColor() It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/1f399944ab9d2d46b372a259374cb1c2d26f107c%5E%21 tdf#107792 vcl: simplify SetFillColor() It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/773a29a3fa188b0a1290781d305d2df868e1600c%5E%21 tdf#107792 vcl: split MakeBrush into functions It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/007396b294c9ecdd7468c5dd7bc01a30905e904d%5E%21 tdf#107792 vcl/win/gdi: reduce number of return paths It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/399af2e8cb3b80555194b4a6186fe9deabeac95d%5E%21 tdf#107792 vcl/win/gdi: shrink WinSalGraphicsImpl::MakePen API It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/af9493419e103cff3b8b006c6d41613c42df8a49%5E%21 tdf#107792: vcl/win/gdi: extract Get50PercentBrush It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/71f02295f2dca557fddd7dbdd19bfd464de06508%5E%21 tdf#107792 vcl/win/gdi: introduce ScopedHBRUSH It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/bcbcfa7927c575ddb1379b3cd8ef1b2df709ba66%5E%21 tdf#107792 vcl/win/gdi: introduce ScopedHRGN It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/8addcffd4c4b1ce8d7e4c8dee01c312b68f1de71%5E%21 tdf#107792 vcl/win: replace ScopedHDC with type alias It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/60e308e60a623bd45b7a30aa7f538ee6ed3d42cb%5E%21 tdf#107792 vcl/win: move scoped_gdi.hxx to vcl/inc/win It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/889b57e48d94d11cd76578f421ef27534300a894%5E%21 tdf#107792 vcl/win: introduce ScopedSelectedHPEN It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/f00fc7ee17153ff70fa4fb2052a5b555af0c054c%5E%21 tdf#107792 vcl/win: introduce ScopedSelectedHFONT It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/a40f12c3f18e4262336fcd51d26dd099eae1e070%5E%21 tdf#107792 vcl/win: introduce ScopedSelectedHBRUSH It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/643edcb11d635e09042d82d191279b6b1c2f25a9%5E%21 tdf#107792 vcl/win: introduce ScopedHBITMAP It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/b940eb5903f583089e99042d60ceae635ae2af83%5E%21 tdf#107792 vcl/win: do not afraid to delete stock objects It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Xisco Faulí committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/81ab8e736fafbe826008939568e2d446204ae080%5E%21 Revert "tdf#107792 vcl/win: do not afraid to delete stock objects" It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/c2c60eb9969716ef91a83952203948b5e334ec85%5E%21 tdf#107792 vcl/win: introduce ScopedCachedHDC It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/cd520894e7092349f1a7aad07066376bb28571fc%5E%21 tdf#107792 vcl/win: use ScopedGDI in WinSalVirtualDevice It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/d3e888cf9e053259076671e8bea0d667c1f92357%5E%21 tdf#107792 vcl/win: simplify WinSalInstance::CreateVirtualDevice It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dmitriy Shilin committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/48f1451407443450e49b1253e95671d9513021b8%5E%21 tdf#107792 vcl/win: simplify WinSalVirtualDevice::~WinSalVirtualDevice() It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Dear Dmitriy Shilin, This bug has been in ASSIGNED status for more than 3 months without any activity. Resetting it to NEW. Please assigned it back to yourself if you're still working on this.