Bugzilla – Full Text Bug Listing
| Summary: | [prefs] EclipsePreferences.toString() changes the state of object under debug | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||||
| Component: | Compendium | Assignee: | equinox.compendium-inbox <equinox.compendium-inbox> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | blocker | ||||||||
| Priority: | P1 | CC: | daniel_megert, dj.houghton, krzysztof.daniel, markus.kell.r, tjwatson, tomasz.zarna | ||||||
| Version: | 3.6.1 | ||||||||
| Target Milestone: | 3.7 M5 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 207510 | ||||||||
| Attachments: |
|
||||||||
Shouldn't cachedPath become final and initialized in the constructor of EclipsePreferences? The proposed patch would always return null from toString if absolutePath is never called. Better fix would be to change the constructor:
protected EclipsePreferences(EclipsePreferences parent, String name) {
super();
this.parent = parent;
this.name = name;
cachedPath=null;//make sure cache is cleared after setting parent
}
I agree with John. Fixed in HEAD. (In reply to comment #2) > Better fix would be to change the constructor: > > protected EclipsePreferences(EclipsePreferences parent, String name) { > super(); > this.parent = parent; > this.name = name; > cachedPath=null;//make sure cache is cleared after setting parent > } This can be very misleading when run in debug mode. If we first enter the constructor and see in Variables view that "this" is represented by "/" and then after stepping over simple assignment cachedPath=null we get confused because now Variables view shows something totally different although nothing happened in the constructor. toString() returns cachedPath after initialization of cachedPath but it happens more than once if initialization of cachedPath is done also in toString() and we are in debug mode. Debugging should be absolutely transparent for the execution path and in this case it is not until we change implementation of toString(). I think we should also change its implementation so that it does not alter any fields of the object. (In reply to comment #5) > This can be very misleading when run in debug mode. Constructors must not call overridable method. When the debugger calls toString on partially constructed object, you are effectively doing this and can get undefined results. The debugger really should not be calling toString on a partially constructed object. But given that the debugger does this, classes should be coded to avoid entering a bad state if this happens. But the class does not have to be coded to provide meaningful output for toString when called while partially constructed. Well, as far as I can see, the problem is not that toString is overridable. What causes trouble here is the fact that calling toString may change object state. I agree with BJ that the class "should be coded to avoid entering a bad state if this happens", which is accomplished with John's fix, but I think it would be much safer if we altered the toString method which leads to the bad state. We know the cause, why not fix it? (In reply to comment #7) > Well, as far as I can see, the problem is not that toString is overridable. That is exactly the problem. Someone (the debugger) is calling an overridable method on an object which is partially constructed. Since the subtype state of the object is not properly set, how can you expect proper behavior of the subtype's implementation of the overridden method? This is just one example of odd behavior because of the debugger calling toString on a partially constructed object. If you are debugging the initialization of an object, you really can't always expect toString to display meaningful information. I am sure there are many other types in the system which will have similar issues with toString when debugging object construction. (In fact I encountered one in my own code the other day while debugging :-). Just about any toString implementation is going to be affected by the execution of the object's constructor. I.e., if you set a breakpoint in the constructor of any object, you will typically get different toString outputs as you proceed through the constructor. Although it needs to be done carefully, I also think its fine for a toString implementation to alter a field. We are lazily caching the string representation of the object for performance reasons (it is not always needed and is somewhat expensive to compute). That cache will naturally get populated by invoking toString. The bug was that we didn't clear that cache in all places where fields affecting the cache value were changed. (In reply to comment #9) I agree that there is nothing wrong with toString() being affected by the constructor's execution. But I'm not sure why toString() changes the object's state, if it doesn't have to. Created attachment 185222 [details]
Alternate fix
I attached an alternate patch. What do you think? (In reply to comment #0) > This is a blocker as we are not able to debug the code safely [..] I agree that the underlying problem should be fixed, but this is not a blocker. To work around the problem for now, just add a Detail Formatter for the affected type (in Preferences) and use a harmless snippet like "workaround...". (In reply to comment #12) > I attached an alternate patch. What do you think? It seems rather absurd to go to all this length to solve a problem that does not exist. The only "problem" is that, when debugging the constructor with a debugger that calls toString on the partially constructed object, the debugger displays undefined values for the object until the constructor completes. (In reply to comment #14) It's just my proposal. Since people are not sure whether changing the object state in toString() is fine or not, I attached the patch. If you think it is not worth of looking at it, feel free to close the bug again. There are many examples where a toString caches the result as long as the result is constant. In this case it is based on two final fields that are constant. It just happens that absolutePath and toString both return the same constant result. It makes sense to me that both methods would do the exact same thing. The one line fix John provided is adequate in my opinion. |
Created attachment 185116 [details] Patch v.0.1 org.eclipse.core.internal.preferences.EclipsePreferences.toString() initializes cachedPath if toString() is called before the object is initialized by constructor. This happens only when running in debug mode if you add a breakpoint at the beginning of the constructor to see current values of fields and then open Variables view to see those values. Variables view calls Object.toString() to show some nice string representation of objects and it changes not initialized cachedPath to "/". Then it is no longer possible for the constructor to set correct value of cachedPath as absolutePath() initializes this field once only. In my case I was able to add preferences with path "/" as a child of preferences "/project". This is a blocker as we are not able to debug the code safely as it may change unexpectedly state of objects showing inappropriate results. toString() should not initialize the object in any way as it is mainly used for debug purposes. Adding a breakpoint inside toString() will not help as this method is called by host Eclipse instance. You may add some System.out.println() calls to see that this method is in use. You will not see the problem when running normally (not in debug mode) as you will not have any reference to the object before the constructor returns so you will not be able to call toString() for the object. Attached patch solves the problem.