Bug 520488 - [cocoa] If a child Shell disposes its parent, parent throws NPE
Summary: [cocoa] If a child Shell disposes its parent, parent throws NPE
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.7   Edit
Hardware: PC Mac OS X
: P3 normal (vote)
Target Milestone: 4.14 M3   Edit
Assignee: Karsten Thoms CLA
QA Contact: Sravan Kumar Lakkimsetti CLA
URL:
Whiteboard:
Keywords: helpwanted, triaged
: 466766 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-02 23:53 EDT by Ned Twigg CLA
Modified: 2020-12-01 12:50 EST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Twigg CLA 2017-08-02 23:53:22 EDT
The test code to reproduce this problem is very simple:

	Display display = Display.getDefault();
	Shell root = new Shell(display, SWT.SHELL_TRIM);
	Shell child = new Shell(root, SWT.SHELL_TRIM);
	child.addListener(SWT.Dispose, e -> root.dispose());
	root.dispose();

This results in the following NPE on Cocoa, but not win32 or gtk.  This bug is present in Oxygen, and also previous versions, I believe.

java.lang.NullPointerException
	at org.eclipse.swt.widgets.Composite._getChildren(Composite.java:95)
	at org.eclipse.swt.widgets.Composite.releaseChildren(Composite.java:957)
	at org.eclipse.swt.widgets.Canvas.releaseChildren(Canvas.java:348)
	at org.eclipse.swt.widgets.Decorations.releaseChildren(Decorations.java:403)
	at org.eclipse.swt.widgets.Shell.releaseChildren(Shell.java:1417)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:1345)
	at org.eclipse.swt.widgets.Control.release(Control.java:2943)
	at org.eclipse.swt.widgets.Widget.dispose(Widget.java:686)
        at (the root.dispose() line)

Currently, Widget.release looks like this:

    void release (boolean destroy) {
        if ((state & DISPOSE_SENT) == 0) {
            state |= DISPOSE_SENT;
            sendEvent (SWT.Dispose);
        }
        if ((state & DISPOSED) == 0) {
            releaseChildren (destroy);
        }
        ...

One possible fix, to ensure that releaseChildren is only called once, is this:

    void release (boolean destroy) {
        if ((state & DISPOSE_SENT) == 0) {
            state |= DISPOSE_SENT;
            sendEvent (SWT.Dispose);
            releaseChildren (destroy);
        }
        ...

But this is old deep code, so maybe I'm missing a bigger picture.
Comment 1 Eclipse Genie CLA 2019-09-30 17:08:37 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.
Comment 2 Ned Twigg CLA 2019-10-01 00:02:28 EDT
Retested with `4.13.0` aka `2019-09` aka `3.112.0`.  Same problem persists.  Here are the new line numbers:

java.lang.NullPointerException
	at org.eclipse.swt.widgets.Composite._getChildren(Composite.java:99)
	at org.eclipse.swt.widgets.Composite.releaseChildren(Composite.java:953)
	at org.eclipse.swt.widgets.Canvas.releaseChildren(Canvas.java:352)
	at org.eclipse.swt.widgets.Decorations.releaseChildren(Decorations.java:406)
	at org.eclipse.swt.widgets.Shell.releaseChildren(Shell.java:1420)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:1356)
	at org.eclipse.swt.widgets.Control.release(Control.java:2974)
	at org.eclipse.swt.widgets.Widget.dispose(Widget.java:694)
Comment 3 Karsten Thoms CLA 2019-10-01 03:55:56 EDT
Thanks for confirming that this bug is still valid. Maybe you could provide a Gerrit patch? This would also run all UI tests. I would assume that if this would introduce a regression that at least one UI test should fail. Even better, could you add a dedicated unit test?
Then we need an SWT expert to review this. (I'm not one ;-) )
Comment 4 Ned Twigg CLA 2019-10-01 11:49:14 EDT
If I could submit a PR on GitHub it would already be submitted.  As it is, the bug is pretty easy to workaround, and it's easier for me to keep our workaround in place than to navigate bugzilla and gerrit.

I've submitted patches before, but I feel that I spend more time futzing with the tools than the actual thing I'm attempting to contribute.  Would be awesome if there was a workflow for allowing GitHub issues and contributions.  Similar to git-svn, doesn't need to be a project switch, just a parallel universe where everything gets merged together eventually.  I realize that's out-of-scope here, just want to be honest and realistic about how I spend my time contributing upstream to open source projects.
Comment 5 Niraj Modi CLA 2019-10-04 10:50:52 EDT
I don't have a MAC setup, but going by the code I attempted a possible work-around for this NPE, will share a gerrit.
Comment 6 Eclipse Genie CLA 2019-10-04 10:53:01 EDT
New Gerrit change created: https://git.eclipse.org/r/150599
Comment 7 Niraj Modi CLA 2019-10-08 05:14:16 EDT
Thanks Karsten for the updated patch, we will look into this post M1.
Comment 9 Niraj Modi CLA 2019-10-16 06:23:44 EDT
(In reply to Eclipse Genie from comment #8)
> Gerrit change https://git.eclipse.org/r/150599 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=53d450854a768db924003aaead9d7df6e96f7fe8

Thanks Sravan for the review, resolving now.
Comment 10 Lakshmi P Shanmugam CLA 2019-11-20 06:14:37 EST
Verified on I20191119-1250 with JUnit test.
Comment 11 Lakshmi P Shanmugam CLA 2020-12-01 07:03:19 EST
*** Bug 466766 has been marked as a duplicate of this bug. ***