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.
Hello Markus Mohrhard, Thank you for reporting the bug. Unfortunately without clear steps to reproduce it, I cannot track down the origin of the problem. Please provide a clearer set of step-by-step instructions on how to reproduce the problem. I have set the bug's status to 'NEEDINFO'. Please change it back to 'UNCONFIRMED' once the steps are provided Thanks