Bug 107792 - Introduce reference counting for GDI handles
Summary: Introduce reference counting for GDI handles
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.4.0.0.alpha1+
Hardware: All Windows (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.3.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks: GDI-Limit
  Show dependency treegraph
 
Reported: 2017-05-12 10:28 UTC by Markus Mohrhard
Modified: 2019-06-10 14:52 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Mohrhard 2017-05-12 10:28:13 UTC
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++.
Comment 1 Xisco Faulí 2017-05-12 10:34:13 UTC
Moving to NEW
Comment 2 Kowther Hassan 2018-04-07 23:52:45 UTC
(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?
Comment 3 Michael Meeks 2018-04-08 07:33:45 UTC
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 ? =)
Comment 4 Tomaz Vajngerl 2018-04-09 02:58:12 UTC
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
Comment 5 Gianluca 2018-07-13 11:32:41 UTC
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?
Comment 6 Xisco Faulí 2018-08-14 02:30:33 UTC Comment hidden (obsolete)
Comment 7 Gianluca 2018-08-14 21:06:30 UTC
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.
Comment 8 Gianluca 2018-09-02 21:50:53 UTC
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
Comment 9 Dmitriy Shilin 2018-11-28 07:00:55 UTC
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.
Comment 10 Xisco Faulí 2018-11-28 09:55:03 UTC
@Michael, @Noel, I'm adding you to this bugs so you can follow Dmitriy's work in the following weeks...
Comment 11 Dmitriy Shilin 2018-12-05 07:47:43 UTC
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
Comment 12 Michael Meeks 2018-12-05 09:11:45 UTC
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.
Comment 13 Noel Grandin 2018-12-05 09:14:26 UTC
(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>;
Comment 14 Commit Notification 2018-12-11 07:46:17 UTC
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.
Comment 15 Commit Notification 2018-12-12 08:04:01 UTC
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.
Comment 16 Commit Notification 2018-12-12 10:32:17 UTC
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.
Comment 17 Commit Notification 2018-12-13 05:17:15 UTC
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.
Comment 18 Commit Notification 2018-12-13 05:18:46 UTC
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.
Comment 19 Commit Notification 2018-12-24 09:16:52 UTC
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.
Comment 20 Commit Notification 2018-12-27 11:45:13 UTC
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.
Comment 21 Commit Notification 2019-01-08 10:13:00 UTC
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.
Comment 22 Commit Notification 2019-01-09 10:06:29 UTC
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.
Comment 23 Commit Notification 2019-01-13 10:06:28 UTC
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.
Comment 24 Commit Notification 2019-01-13 15:41:49 UTC
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.
Comment 25 Commit Notification 2019-01-18 14:22:09 UTC
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.
Comment 26 Commit Notification 2019-01-18 14:37:29 UTC
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.
Comment 27 Commit Notification 2019-01-18 15:39:27 UTC
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.
Comment 28 Commit Notification 2019-01-20 10:13:12 UTC
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.
Comment 29 Commit Notification 2019-01-22 16:10:22 UTC
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.
Comment 30 Commit Notification 2019-01-30 06:15:44 UTC
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.
Comment 31 Commit Notification 2019-02-01 11:46:58 UTC
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.
Comment 32 Commit Notification 2019-02-06 07:30:40 UTC
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.
Comment 33 Commit Notification 2019-02-06 10:15:15 UTC
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.
Comment 34 Xisco Faulí 2019-03-09 03:39:40 UTC Comment hidden (obsolete)
Comment 35 Xisco Faulí 2019-03-10 03:17:14 UTC Comment hidden (obsolete)
Comment 36 Xisco Faulí 2019-06-10 14:52:46 UTC
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.