Comment 1 Jonah Graham 2021-05-30 19:03:20 EDT

AFAICT this is a problem in SWT itself, an image is in TaskBar:

					Image image2 = Image.win32_new (display, SWT.BITMAP, item.hBitmap);
					data = image2.getImageData (DPIUtil.getDeviceZoom ());

but never disposed.

There may not actually be a leak here, but if so then SWT should avoid making an image that actually needs disposal.

Comment 2 Jonah Graham 2021-05-30 20:40:01 EDT

The issue here is that initNonDisposeTracking() is being called in the wrong place - initNonDisposeTracking() should only be called on resources requiring disposal and calls to win32_new methods should not be disposal tracked.

There appears to be workaround of setting org.eclipse.swt.graphics.Resource.ResourceTracker.ignoreMe - however that workaround isn't needed if initNonDisposeTracking is only called from init().

I will turn the above statement into a patch for review with appropriate test to make sure I am correct.

Comment 3 Eclipse Genie 2021-05-30 22:07:43 EDT

New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/181172

Comment 4 Eclipse Genie 2021-05-30 22:19:56 EDT

New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/181173

Comment 5 Jonah Graham 2021-05-30 22:20:20 EDT

AFAICT the stack trace / error log entry in Comment 0 affects all users on Windows that have Mylyn installed, which is many of the the EPP packages (for now).

The patch I proposed in Comment 3 is probably the correct patch, but may be too late to get something that requires so many eyes on it into RC2.

Therefore I propose for RC2 that the gerrit in Comment 4 is used which just squelches the immediate error to give time for properly considering the fuller fix.

Comment 6 Niraj Modi 2021-05-31 05:02:04 EDT

(In reply to Eclipse Genie from comment #4)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/181173

@jonah@kichwacoders.com
Since your targeting above patch for 4.20 RC2.
Can you create a separate bug for this part for tracked against 4.20 RC2 which we need for approval part as well. Thanks!

Comment 7 Alexandr Miloslavskiy 2021-05-31 05:24:25 EDT

Unfortunately it seems that SWT is rather confused about owning the handle in 'Image.win32_new()':

* In 'Display.createIcon()' it owns the handle and needs to be tracked
* In 'Program.getImageData()' it owns the handle, but code code be changed easily
* In 'Display.getSystemImage()' it owns the handle and needs to be tracked
* In 'TaskBar.createShellLink()' it doesn't own the handle and shall not free it
* In 'ImageTransfer.nativeToJava()' there seems to be a double free via
  1) DeleteObject() and
  2) image.dispose()
  Looks like a bug.

On average, I would say that SWT believes that 'Image.win32_new()' owns the handle. This unfortunately goes against the idea of both patches provided in this Bug.

Given that, I see the following potential fixes:
(1) Align all functions like '<Resource>.xxx_new()' so that they don't own the handle. Then apply patch 1 from this Bug. This would be the cleanest solution, probably.
(2) Assume that 'Image.win32_new()' owns its handle and fix 'TaskBar.createShellLink()' only by assigning 0 to 'handle'.

Jonah, would you check if 'ImageTransfer.nativeToJava()' is indeed a double free?

Comment 8 Alexandr Miloslavskiy 2021-05-31 05:28:24 EDT

Note: solution (1) is going to be a difficult one, because when Image is given away to unsuspecting caller, it's harder to impose any expectations about owning the handle.

When I was making the Resource Tracker, my first approach was to add another parameter to every '<Resource>.xxx_new()' that specifies whether Resource will own the handle or not. This sounded like a good and explicit contract to me, but resulted in quite some code changes, so eventually I dropped it in favor of declaring entire functions as owning or not owning. I now begin to think that explicit approach had its merits.

Comment 9 Niraj Modi 2021-05-31 06:27:37 EDT

(In reply to Alexandr Miloslavskiy from comment #7)
> Unfortunately it seems that SWT is rather confused about owning the handle
> in 'Image.win32_new()':
> 
> * In 'Display.createIcon()' it owns the handle and needs to be tracked
> * In 'Program.getImageData()' it owns the handle, but code code be changed
> easily
> * In 'Display.getSystemImage()' it owns the handle and needs to be tracked
> * In 'TaskBar.createShellLink()' it doesn't own the handle and shall not
> free it
> * In 'ImageTransfer.nativeToJava()' there seems to be a double free via
>   1) DeleteObject() and
>   2) image.dispose()
>   Looks like a bug.
> 
> On average, I would say that SWT believes that 'Image.win32_new()' owns the
> handle. This unfortunately goes against the idea of both patches provided in
> this Bug.
> 
> Given that, I see the following potential fixes:
> (1) Align all functions like '<Resource>.xxx_new()' so that they don't own
> the handle. Then apply patch 1 from this Bug. This would be the cleanest
> solution, probably.
> (2) Assume that 'Image.win32_new()' owns its handle and fix
> 'TaskBar.createShellLink()' only by assigning 0 to 'handle'.
> 
> Jonah, would you check if 'ImageTransfer.nativeToJava()' is indeed a double
> free?

Thanks Alexandr for the detailed feedback, based on above comments attempted an alternate fix patch(based on approach 2 above) specifically scoped for bug 573813.

Please have a look. Will share the gerrit shortly.

Comment 10 Eclipse Genie 2021-05-31 06:28:27 EDT

New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/181184

Comment 11 Eclipse Genie 2021-05-31 09:28:55 EDT

Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/181184 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=33df33ab19f98affef101785d4617302f3751922

Comment 12 Niraj Modi 2021-05-31 09:30:15 EDT

Hi Sravan,
For your review+ for RC2

Comment 13 Jonah Graham 2021-05-31 10:03:38 EDT

(In reply to Alexandr Miloslavskiy from comment #7)
> On average, I would say that SWT believes that 'Image.win32_new()' owns the
> handle. This unfortunately goes against the idea of both patches provided in
> this Bug.

I agree - and I will withdraw my patches.

However I don't know where this should be documented - for 20ish years[1] the documentation has been who calls the constructor is responsible for doing the free. The <platform>_new methods are workarounds that are only callable from within SWT, and I agree that changing that so that SWT's own leaks are detected is a good idea. The current use of ignoreNonDisposed still seems suspect to me, for example calling Image.win32_new is tracked, by Font.win32_new is not tracked.

> Jonah, would you check if 'ImageTransfer.nativeToJava()' is indeed a double free?

Yes it is a double DeleteObject. 



[1] https://www.eclipse.org/articles/swt-design-2/swt-design-2.html

Comment 14 Alexandr Miloslavskiy 2021-05-31 10:10:24 EDT

> The current use of ignoreNonDisposed
> still seems suspect to me, for example calling Image.win32_new is tracked,
> by Font.win32_new is not tracked.

It's not like I'm too confident myself. Back then, I verified the use of some 'xxx_new' functions, but I think that at the end I started declaring them as "not owning the handle" without full analysis if I saw that some use doesn't own.

So it's possible that some uses that are currently ignored in the tracker should actually be tracked.

This goes back to the idea of fine-tuning every 'xxx_new' with an additional parameter. That's a bit of work, though, and may introduce new bugs, so maybe the current solution is good enough. Still, if someone is willing to do such fine-tuning, I will support the idea.

Comment 15 Alexandr Miloslavskiy 2021-05-31 10:13:26 EDT

Note that if the parameter is introduced, then '.dispose()' shall not free the non-owned handle, and '.isDisposed()' ... hmm... can't really say what would be good here. It could also require similar changes in some other APIs.

Comment 16 Jonah Graham 2021-05-31 10:22:07 EDT

One option is to only track "user" created SWT resources, which means that initNonDisposed() should not be called until init() is called. That leaves pre-Bug 569752 status-quo for internally created handles, but provides the excellent new feature to SWT users. WDYT?

Comment 17 Alexandr Miloslavskiy 2021-05-31 13:56:54 EDT

This can be done, but will that be an worthy improvement compared to current situation? 

As far as I understand, most likely, everything is already fine. Or maybe there's one other problem in SWT where internal Resource is incorrectly reported as leaked, and this can again be band-aided similar to this patch.

It's not like I object really. If you really think that this needs to be done, I will repeat my opinion, but otherwise stay neutral.

Comment 18 Jonah Graham 2021-05-31 22:53:46 EDT

(In reply to Alexandr Miloslavskiy from comment #17)
> This can be done, but will that be an worthy improvement compared to current
> situation? 

I agree - it is not worth changing from where we are now. These corner cases of SWT allocating objects is not a worthy improvement and handling remaining false positives (if there are any) in a similar way to what has been done here is more than ok.

Thank you for your time on this!

Comment 19 Niraj Modi 2021-06-01 02:38:46 EDT

(In reply to Jonah Graham from comment #18)
> (In reply to Alexandr Miloslavskiy from comment #17)
> > This can be done, but will that be an worthy improvement compared to current
> > situation? 
> 
> I agree - it is not worth changing from where we are now. These corner cases
> of SWT allocating objects is not a worthy improvement and handling remaining
> false positives (if there are any) in a similar way to what has been done
> here is more than ok.
> 
> Thank you for your time on this!

Thanks Alexandr/Jonah, appreciate the detailed analysis on this problem.
Resolving this bug for RC2, open issues(if any) to be tracked via new bugs.