Community
Participate
Working Groups
Either throw an error or do nothing - need to decide.
Our customers are hit by this regularly... Is there any chance for a fix? Caused by: java.lang.NullPointerException at org.eclipse.swt.accessibility.AccessibleObject.addRelation(AccessibleObject.java:3514) at org.eclipse.swt.accessibility.Accessible.addRelations(Accessible.java:423) at org.eclipse.swt.accessibility.AccessibleFactory.atkObjectFactory_create_accessible(AccessibleFactory.java:342) at org.eclipse.swt.internal.gtk.OS._g_main_context_iteration(Native Method) at org.eclipse.swt.internal.gtk.OS.g_main_context_iteration(OS.java:2276)
Interesting. I didn't expect anyone to run into this problem - I just wanted to think about it for API cleanup. Do your customers truly make calls similar to the following? control.getAccessible().addRelation(ACC.RELATION_LABELLED_BY, null);
The problem is that we have no idea how this can happen. In our application we never call addRelation and it is unclear what leads to the NPE. These NPEs are happening about 3-5 times a day (Linux userbase about 2000 daily). In the past it happened once or twice to the same user but then worked fine again. Since yesterday we have a user where it happens all the times. According to him he did not install/run any accessibility tool. I guess that about 80% are gtk-32bit, the rest gtk-64bit. Our customers can not extend our application, they just use it.
> In our application we never call addRelation and it is unclear what leads to the NPE. Thanks, Marcel - useful information. I will see if I can find a possible path that could lead to this NPE even if addRelation is not called.
There are a couple of possible paths I can follow, but it would help to narrow it down if you can tell me if you call any of these methods: Change the Z-order of a control: org.eclipse.swt.widgets.Control.moveAbove(Control) org.eclipse.swt.widgets.Control.moveBelow(Control) Change the parent of a control: org.eclipse.swt.widgets.Control.setParent(Composite)
I added a null-check (target != null) to AccessibleObject, but it still happens. It seems that the target is non-null, but target.getAccessibleObject() returns null in some cases. We use (if this is still relevant) - moveAbove: in some places, but the NPE happens sometimes right after startup, where this is not called yet (the respective control not created yet) - moveBelow: no - setParent: no
See also AERI https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/579ca690e4b04403a3cbb7c0 and https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/574d7cebe4b0d39ec5919e33
*** Bug 418164 has been marked as a duplicate of this bug. ***
*** Bug 495515 has been marked as a duplicate of this bug. ***
New Gerrit change created: https://git.eclipse.org/r/78700
(In reply to Eclipse Genie from comment #10) > New Gerrit change created: https://git.eclipse.org/r/78700 I added null checks around the public API that could potentially lead to a null target. This way we could tell what adds null objects before they lead to a crash. This also allows clients to put the addRelation() and removeRelation() calls into try/catch clauses to prevent crashes. I'm not sure if this can be classified as api change. This may be an *implicit* api change, in that it throws an unchecked exception: https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html I.e, previous code doesn't have to be modified, but new code can add try/catch around a call to that api. Does anyone know if this is considered api change?
As discussed on mailing list, as per: http://help.eclipse.org/neon/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fmisc%2Fapi-usage-rules.html "Null parameters. Do not pass null as a parameter to an API method unless the parameter is explicitly documented as allowing null. This is perhaps the most frequently made programming error." I.e on the grounds that API doesn't allow null to begin with, we can consider this as non-api change. Thus can go ahead with merge afaik.
Gerrit change https://git.eclipse.org/r/78700 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=d5aaf6787e634fc7205b27242335ef52d1b330a1
(In reply to Eclipse Genie from comment #13) > Gerrit change https://git.eclipse.org/r/78700 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=d5aaf6787e634fc7205b27242335ef52d1b330a1 In master now, thanks for the patch Leo!
This is a good candidate for 4.6.2
Leo, https://dev.eclipse.org/mhonarc/lists/platform-swt-dev/msg08006.html and comment 6 indicate that the problem is not that the 'target' passed to AccessibleObject.addRelation(..) is null, but that 'target.getAccessibleObject()' is null. Gurmeen, do you have steps to reproduce?
I am not sure as I see it as part of existing UI when expanding tree item or selecting text to copy to clipboard. The gtk version that I see the crash: Version: 3.18.9-1ubuntu3.1 OS version- Ubuntu Description: Ubuntu 16.04.1 LTS
Its on unity desktop version on unix.
(In reply to Markus Keller from comment #16) > Leo, https://dev.eclipse.org/mhonarc/lists/platform-swt-dev/msg08006.html > and comment 6 indicate that the problem is not that the 'target' passed to > AccessibleObject.addRelation(..) is null, but that > 'target.getAccessibleObject()' is null. > > Gurmeen, do you have steps to reproduce? Hello Markus, The logic here was that in order to stop getAccessibleObject() from returning a null, the user should not be permitted to add a null-object to begin with. Theoretically that should fix things. (Unless something sets an added an accessibleObject to null after it was added?, in which case we could add more null-checks) Thoughts?
I backported null checks here: https://git.eclipse.org/r/#/c/80507/ @ Markus/other, if the patch doesn't fix the problem or I missed something, please kindly re-open this bug and let me know response to #19. Thank you
(In reply to Leo Ufimtsev from comment #20) I've reverted that commit with https://git.eclipse.org/r/80508. R4_6_maintenance is closed until 4.6.1 is declared, and you would have required an independent code review for the backport anyway. See https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_6_1.php
(In reply to Leo Ufimtsev from comment #19) > The logic here was that in order to stop getAccessibleObject() from > returning a null, the user should not be permitted to add a null-object to > begin with. The Accessible#accessibleObject field is not set by the user. SWT is supposed to create and store this object when necessary. The bug is that the object either didn't get created or that it got released too early.
(In reply to Markus Keller from comment #21) > (In reply to Leo Ufimtsev from comment #20) > I've reverted that commit with https://git.eclipse.org/r/80508. > > R4_6_maintenance is closed until 4.6.1 is declared, and you would have > required an independent code review for the backport anyway. See > https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_6_1.php Thank you, I should have read the freeze plan more attentively. (In reply to Markus Keller from comment #22) > (In reply to Leo Ufimtsev from comment #19) > > The logic here was that in order to stop getAccessibleObject() from > > returning a null, the user should not be permitted to add a null-object to > > begin with. > > The Accessible#accessibleObject field is not set by the user. SWT is > supposed to create and store this object when necessary. The bug is that the > object either didn't get created or that it got released too early. I see. I submitted a tennative patch to check for a null. It checks for null in Accessible.addRelations() that is simmilar to Accessible.addRelation(): https://git.eclipse.org/r/#/c/80520/ Those are the only two methods were AccessibleObject:addRelation() is called. A snippet or steps to reproduce would help with finding the root cause, but this patch should at least prevent crashes. I added you as reviewer, would you be able to take a look and let me know your thoughts? Thank you.
New Gerrit change created: https://git.eclipse.org/r/80507
Gerrit change https://git.eclipse.org/r/80507 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=e6349257e05a89e01f81a9400e925d17523a77fc
New Gerrit change created: https://git.eclipse.org/r/80508
Gerrit change https://git.eclipse.org/r/80508 was merged to [R4_6_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a707757677b3f2f85d89c66edbc9334ea4a15f7e
New Gerrit change created: https://git.eclipse.org/r/80520
(In reply to Leo Ufimtsev from comment #23) > (In reply to Markus Keller from comment #21) > > (In reply to Leo Ufimtsev from comment #20) > > I've reverted that commit with https://git.eclipse.org/r/80508. > > > > R4_6_maintenance is closed until 4.6.1 is declared, and you would have > > required an independent code review for the backport anyway. See > > https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_6_1.php > Thank you, I should have read the freeze plan more attentively. > > (In reply to Markus Keller from comment #22) > > (In reply to Leo Ufimtsev from comment #19) > > > The logic here was that in order to stop getAccessibleObject() from > > > returning a null, the user should not be permitted to add a null-object to > > > begin with. > > > > The Accessible#accessibleObject field is not set by the user. SWT is > > supposed to create and store this object when necessary. The bug is that the > > object either didn't get created or that it got released too early. > I see. > I submitted a tennative patch to check for a null. It checks for null in > Accessible.addRelations() that is simmilar to Accessible.addRelation(): > https://git.eclipse.org/r/#/c/80520/ > Those are the only two methods were AccessibleObject:addRelation() is called. > > A snippet or steps to reproduce would help with finding the root cause, but > this > patch should at least prevent crashes. > > I added you as reviewer, would you be able to take a look and let me know > your thoughts? > > Thank you. If I have access to a drop of swt 4.5 then I can verify it. I first saw the problem in 4.3 then upgraded to 4.5 and then to 4.7 drop and still see the problem. I see it with one of SWT GUIs in many scenarios but not with others so not sure of the steps. I will try and if I am able to get the steps - shall post it. I am happy to test the drops on latest package though. Thanks
New Gerrit change created: https://git.eclipse.org/r/80609
(In reply to Leo Ufimtsev from comment #23) > https://git.eclipse.org/r/#/c/80520/ Exactly the same check is already made three lines earlier (line 430), so if this would change anything, then it would indicate a race condition and the "fix" would only work by pure chance. If we just wanted to get rid of the NPE without trying to understand the actual reason, then the check would have to go into AccessibleObject#addRelation(int, Accessible) like this: https://git.eclipse.org/r/80609
(In reply to comment #31) > (In reply to Leo Ufimtsev from comment #23) > > https://git.eclipse.org/r/#/c/80520/ > > Exactly the same check is already made three lines earlier (line 430), so if > this would change anything, then it would indicate a race condition and the > "fix" would only work by pure chance. If we just wanted to get rid of the NPE > without trying to understand the actual reason, then the check would have to go > into AccessibleObject#addRelation(int, Accessible) like this: > > https://git.eclipse.org/r/80609 I see. I've miss-read the stack trace, I thought AccessibleObject was the null object. Thank you for correction. Given that we can't really reproduce the issue, it's hard to perform a deep investigation. I think fix would work for now.
Gerrit change https://git.eclipse.org/r/80609 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a31e66b6450374405a04b72f094dc49ee2c0846f
If you could provide us with a drop on 4.5 or latest, I shall be happy to verify the fix.
@Gurmeen, Yesterday's build contained the fix: http://download.eclipse.org/eclipse/downloads/ -> Nightly -> N2016xxxxxxxxxx (8th sept). To tell: http://download.eclipse.org/eclipse/downloads/drops4/N20160908-2000/directory.txt contains Markus's commit: "a31e66b6450374405a04b72f094dc49ee2c0846f eclipse.platform.swt (R4_6-158-ga31e66b)" Let us know how it goes.
Thanks for the fix - it seems to work. However, I noted a similar problem in removeRelation: java.lang.NullPointerException at org.eclipse.swt.accessibility.AccessibleObject.removeRelation(Unknown Source) at org.eclipse.swt.accessibility.Accessible.removeRelation(Unknown Source) at org.eclipse.swt.widgets.Control.removeRelation(Unknown Source) at org.eclipse.swt.widgets.Control.release(Unknown Source) at org.eclipse.swt.widgets.Widget.dispose(Unknown Source) at com.isode.ddm.editor.EEF_DN.deleteRow(EEF_DN.java:627) Adding a similar check in removeRelation seems to fix the problem, so it might be good to add this to removeRelation as well. It would also help if this fix can be backported to old releases.
It would be helpful to know if the remove relation fix will be included as part of this fix ? Also it would be useful to know the timescale of when this fix intended to be released and whether it will be backported to 4.3 as well.
(In reply to Gurmeen Bindra from comment #37) > It would be helpful to know if the remove relation fix will be included as > part of this fix ? > Also it would be useful to know the timescale of when this fix intended to > be released and whether it will be backported to 4.3 as well. Sorry for the dealy. I'll try to add those next week sometime.
New Gerrit change created: https://git.eclipse.org/r/82292
New Gerrit change created: https://git.eclipse.org/r/82294
(In reply to Eclipse Genie from comment #40) > New Gerrit change created: https://git.eclipse.org/r/82294 Ignore #39. #40 adds a nullcheck to the method similar to previous patches. @Gurmeen Bindra Awaiting review of patch and update on if it fixes things (at least for now) for you..
Thanks for fixing this Leo. could you point me to the swt.jar that has the fix ?
I think I can pick up a nightly build: http://download.eclipse.org/eclipse/downloads/drops4/N20161002-2000/download.php?dropFile=swt-N20161002-2000-gtk-linux-x86_64.zip Will test this and get back to you. If this gets fixed, will be very very helpful if this could be ported back to 4.3.
(In reply to comment #43) > I think I can pick up a nightly build: > http://download.eclipse.org/eclipse/downloads/drops4/N20161002-2000/download.php?dropFile=swt-N20161002-2000-gtk-linux-x86_64.zip > > Will test this and get back to you. If this gets fixed, will be very very > helpful if this could be ported back to 4.3. Ok. Let me know. Also, once testing is done: By 4.3 do you mean 4.3 or the latest 4.6 maintenance branch? (R4_6_maintenance)
I tested and it did not work because the change is still in gerrit and has not been committed yet ! So its not in the nightly builds. By 4.3 , I meant swt/eclipse version 4.3.2.
Gerrit change https://git.eclipse.org/r/82294 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=bad8a49cf9af35f6428be1c814c91fc9bb250c5f
I think the downloads link is not working: download.eclipse.org/eclipse/downloads/drops4/N20161004-2000/
(In reply to Gurmeen Bindra from comment #47) > I think the downloads link is not working: > download.eclipse.org/eclipse/downloads/drops4/N20161004-2000/ The build is generated, but there is an issue with a proxy and thus web links are not working since monday. They're working on it. Please see: Bug 503388 – Unable to access eclipse update sites from releng hipp https://bugs.eclipse.org/bugs/show_bug.cgi?id=503388 Hopefully should work by tomorrow.
The workaround in removeRelation(..) has been pushed and is in http://download.eclipse.org/eclipse/downloads/drops4/I20161004-1000/ . We're currently having trouble with generating the download page and running tests, but the produced build is usable. We'll backport the fix to the 4.6.2 branch after it has been tested in at least 2 successful weekly I-builds. We don't produce builds for older maintenance branches any more. If you need it in 4.3, you'll have to build it yourself or go via https://lts.eclipse.org/ .
(In reply to Markus Keller from comment #49) > The workaround in removeRelation(..) has been pushed and is in > http://download.eclipse.org/eclipse/downloads/drops4/I20161004-1000/ . We're > currently having trouble with generating the download page and running > tests, but the produced build is usable. > > We'll backport the fix to the 4.6.2 branch after it has been tested in at > least 2 successful weekly I-builds. > > We don't produce builds for older maintenance branches any more. If you need > it in 4.3, you'll have to build it yourself or go via > https://lts.eclipse.org/ . I could potentially backport the few patches to R4_3_maintenance? (@Markus thoughts? any problems with that?). However, you'd still have to build Eclipse/SWT bits yourself thou.. If that is of any use to you, let me know.
that would be nice if it is backported to 4.3 - we could build it without modifying the code and hence avoid licensing issues if any.
Tested - the fix works fine for me. Thank you.
(In reply to comment #51) > that would be nice if it is backported to 4.3 - we could build it without > modifying the code and hence avoid licensing issues if any. Ok, kewl. I'll look into backporting.
(In reply to Leo Ufimtsev from comment #53) > (In reply to comment #51) > > that would be nice if it is backported to 4.3 - we could build it without > > modifying the code and hence avoid licensing issues if any. > > Ok, kewl. > > I'll look into backporting. Hmm, it seems like we normally don't backport all the way back to older versions. I've asked on the swt mailing list if we can make a one-off exception for this bug. If there won't be any objections by Oct 11 th, I'll go ahead with 4.3 backport.
thanks !
(In reply to Leo Ufimtsev from comment #50) > I could potentially backport the few patches to R4_3_maintenance? (@Markus > thoughts? any problems with that?). We only do that rarely, and only after the fix has been backported and tested in the active maintenance branch for a while. To backport to out-of-service branches, a few things need to be done carefully: - Create a new bug, set its target milestone to the oldest x.y.z+ version, and make sure it depends on the original bug - Also backport the fix to *all* maintenance branches that are newer than the one you're interested in. This is important for consumers. E.g. moving from 4.3.2+ to 4.4.2+ must not lose any backported fixes. - Make sure you correctly update all necessary bundle and feature version numbers (i.e. all features that contain SWT). E.g. adding 0.0.1 everywhere would be wrong. - Test the fix in all branches. For the concrete use case, an easier way to avoid any and all licensing issues is to create a patch with the changes for the 4.3 branch and attach it here.
(In reply to Markus Keller from comment #56) > (In reply to Leo Ufimtsev from comment #50) > > I could potentially backport the few patches to R4_3_maintenance? (@Markus > > thoughts? any problems with that?). > > We only do that rarely, and only after the fix has been backported and > tested in the active maintenance branch for a while. > > To backport to out-of-service branches, a few things need to be done > carefully: > - Create a new bug, set its target milestone to the oldest x.y.z+ version, > and make sure it depends on the original bug > - Also backport the fix to *all* maintenance branches that are newer than > the one you're interested in. This is important for consumers. E.g. moving > from 4.3.2+ to 4.4.2+ must not lose any backported fixes. > - Make sure you correctly update all necessary bundle and feature version > numbers (i.e. all features that contain SWT). E.g. adding 0.0.1 everywhere > would be wrong. > - Test the fix in all branches. > > For the concrete use case, an easier way to avoid any and all licensing > issues is to create a patch with the changes for the 4.3 branch and attach > it here. Also got a reply from Dani.M: "you will need a +1 from an SWT project lead in the bug report." @Markus Thank you very much for taking the time to do this write up. I made a note of it in the swt wiki: https://wiki.eclipse.org/SWT/Devel/Workflow#Note_about_backporting_to_versions_earlier_than_n-1 I see. Backporting so far back is much more demanding than I had anticipated. @Gurmeen Given the complexity involved, I won't have the time to go through all of that. At the moment the wayland migration ( Bug 496923 – [Wayland] Improve support for Wayland in 4.7 ) and the webkit2 port are more pressing. However, I can backport these to R4_6_Maintenance. (I'll do next week). If you can get a +1 from the SWT project lead and are willing to submit relevant patches, then I could review/merge them for you thou.
Backporting to 4.6 wont be much help to us. When is the next eclipse release with this fix expected ?
(In reply to Gurmeen Bindra from comment #58) > Backporting to 4.6 wont be much help to us. > When is the next eclipse release with this fix expected ? If I'm not mistaken: https://wiki.eclipse.org/Simultaneous_Release .2 will be released in: December 21, 2016 If of use, and things work well, let me know and I'll backport current patches to R4_6_Maintenance.
If there is no use for backporting, should we close the bug?
yes, please.
I've backported the relevant changes (squashed into a single commit) to 4.6.2 via http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=R4_6_maintenance&id=83d34538078cfcb43e1bd9344d645859ecc8e2ae
