Protect NPE in CMRI connection preferences #2734
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simple protection against the tc or adapter being null. What I've not checked into is if those shouldn't have been null in the first place. Was related to a user issue.
bobjacobsen
reviewed
Dec 20, 2016
| String value = ""; | ||
| for (int i = 0; i < node.getLocSearchLightBits().length; i++) { | ||
| value = value + Integer.toHexString(node.getLocSearchLightBits()[i] & 0xF); | ||
| if (tc != null) { |
There was a problem hiding this comment.
I think that if this code runs with tc==null, you're going to store without the node definitions, thereby losing the node definitions. Is that really what you want?
bobjacobsen
reviewed
Dec 20, 2016
| @@ -38,7 +38,9 @@ public String name() { | |||
|
|
|||
| public void loadDetails(JPanel details) { | |||
|
|
|||
| b.addActionListener(new NodeConfigAction((CMRISystemConnectionMemo)adapter.getSystemConnectionMemo())); | |||
| if (adapter != null) { | |||
| b.addActionListener(new NodeConfigAction((CMRISystemConnectionMemo)adapter.getSystemConnectionMemo())); | |||
There was a problem hiding this comment.
If this runs with adapter==null, it won't crash right away, but it will behave oddly later on (because the listener won't be present). I'm much rather see a Large Amount of information being logged here so we have some chance of tracking down the real problem, which is the null adapter value.
bobjacobsen
reviewed
Dec 20, 2016
| String value = ""; | ||
| for (int i = 0; i < node.getLocSearchLightBits().length; i++) { | ||
| value = value + Integer.toHexString(node.getLocSearchLightBits()[i] & 0xF); | ||
| if (tc != null) { |
There was a problem hiding this comment.
Similar to the other comment on this commit, I think this will write a configuration file that doesn't contain, hence loses for all time, the node information.
|
Bob,
Valid point, it skips saving the node data. But that seems a curse of us
using the same place for nodes as connections. I've lots nodes a number of
times over the years when something happened trying to take a serial to
simulator or back. Or other things that caused it. But I don't see how this
can be avoided. If there isn't a TC, there isn't anything to attach the data
to. But I'm thinking at this point the node data is already a lost cause. Or
is the node stored somewhere other than the TC?? If so, I'd say this should
get the node from there instead.
But I suspect the future will be moving the node data out of the connection
anyway. Nodes are not dependent on the type of connection. At least not in
most of the other systems. I've always wondered about why the nodes are
here.
…-Ken Cameron, Member JMRI Dev Team
www.jmri.org
www.fingerlakeslivesteamers.org
www.cnymod.com
www.syracusemodelrr.org
|
|
BTW I got into this from a case where things were not working right for the
user, and the log message didn't get floated up to the user, it just sat in
the log.
So the real question is why the different actions lead to things like
adapter or tc being null/missing??
…-Ken Cameron, Member JMRI Dev Team
www.jmri.org
www.fingerlakeslivesteamers.org
www.cnymod.com
www.syracusemodelrr.org
|
|
I'm thinking that to track down why we end up with the tc or adapter null would be setting a trap in the code where those live and throw exception if something tries to set them to null. That would help find when a 'good' configuration is going bad. But the case of a 'new' configuration not getting setup right in the first place, that's another story. |
|
So the real question is why the different actions lead to things like adapter or tc being null/missing??
Right. That’s the thing we have to fix. When I get a bit of time (after Thursday), I’m going to look into the race condition you saw. Perhaps that’s involved. But there might be other things too. It would be great if we had some more examples of where the TrafficController (or other link) was null. Getting more logging when == null happens would help.
|
|
Flipping between simulator and serial or network seems to be a good way to get things null. Also things like an invalid port (or no ports) or blank/bad network address. At least flipping these things around generated the nulls I found. Yes, sometimes leaving a blank or 'no ports found' and trying to save anyway. |
|
I haven't been able to get this to reproduce, sorry. Maybe I'm not understanding what "Flipping between simulator and serial or network seems to be a good way" means? Happy to try to track down something that's intermittent, just need a recipe that will let me recreate this at least occasionally. |
|
Situation:
System running on CMRI simulator. I start to change it to a serial port, but
since it wasn't plugged in before starting, I'm showing 'no ports found'.
Trying to save throws this and I can't get out plus only error in the log,
no visible for why the 'save' hung. At least it acts that way.
2017-01-02 15:05:00,765 jmrix.AbstractSerialPortController ERROR - No
usable ports returned [AWT-EventQueue-0]
2017-01-02 15:05:06,588 ptionhandler.UncaughtExceptionHandler ERROR -
Uncaught Exception caught by
jmri.util.exceptionhandler.UncaughtExceptionHandler [AWT-EventQueue-0]
java.lang.NullPointerException
at
jmri.jmrix.cmri.serial.serialdriver.configurexml.ConnectionConfigXml.extendE
lement(ConnectionConfigXml.java:40)
at
jmri.jmrix.configurexml.AbstractSerialConnectionConfigXml.store(AbstractSeri
alConnectionConfigXml.java:64)
at
jmri.jmrix.configurexml.AbstractConnectionConfigXml.store(AbstractConnection
ConfigXml.java:32)
at
jmri.configurexml.ConfigXmlManager.elementFromObject(ConfigXmlManager.java:4
96)
at
jmri.jmrix.ConnectionConfigManager.lambda$0(ConnectionConfigManager.java:142
)
at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(Unknown
Source)
at java.util.stream.ReferencePipeline$Head.forEach(Unknown Source)
at
jmri.jmrix.ConnectionConfigManager.savePreferences(ConnectionConfigManager.j
ava:140)
at
jmri.jmrix.ConnectionConfigManager.savePreferences(ConnectionConfigManager.j
ava:132)
at
jmri.jmrix.swing.ConnectionsPreferencesPanel.savePreferences(ConnectionsPref
erencesPanel.java:296)
at
apps.gui3.TabbedPreferences.invokeSaveOptions(TabbedPreferences.java:261)
at apps.gui3.TabbedPreferences.lambda$0(TabbedPreferences.java:155)
at javax.swing.AbstractButton.fireActionPerformed(Unknown Source)
at javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source)
at javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source)
at javax.swing.DefaultButtonModel.setPressed(Unknown Source)
at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(Unknown
Source)
at java.awt.Component.processMouseEvent(Unknown Source)
at javax.swing.JComponent.processMouseEvent(Unknown Source)
at java.awt.Component.processEvent(Unknown Source)
at java.awt.Container.processEvent(Unknown Source)
at java.awt.Component.dispatchEventImpl(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.LightweightDispatcher.retargetMouseEvent(Unknown Source)
at java.awt.LightweightDispatcher.processMouseEvent(Unknown Source)
at java.awt.LightweightDispatcher.dispatchEvent(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Window.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
at java.awt.EventQueue.access$500(Unknown Source)
at java.awt.EventQueue$3.run(Unknown Source)
at java.awt.EventQueue$3.run(Unknown Source)
at java.security.AccessController.doPrivileged(Native Method)
at
java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivileg
e(Unknown Source)
at
java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivileg
e(Unknown Source)
at java.awt.EventQueue$4.run(Unknown Source)
at java.awt.EventQueue$4.run(Unknown Source)
at java.security.AccessController.doPrivileged(Native Method)
at
java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivileg
e(Unknown Source)
at java.awt.EventQueue.dispatchEvent(Unknown Source)
at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown
Source)
at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown
Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.run(Unknown Source)
…-ken c
|
|
I should also note that while the port list will update after plugging in
the right interface, it keeps with the same NPE. Also I can't exit the
system, it may blink but still generates a NPE instead of saving or exiting.
…-Ken Cameron, Member JMRI Dev Team
www.jmri.org
www.fingerlakeslivesteamers.org
www.cnymod.com
www.syracusemodelrr.org
|
Seems the properties got updated and removed the trailing spaces from the keys, which would be confusing anyway.
9 tasks
|
Replaced by overtaken by PR #3233 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User was tripping on one of the spots, checking for similar, I found 2 other places nulls were showing and causing NPE's.