Community
Participate
Working Groups
Build id: I20120814-0800 When starting org.eclipse.jdt.debug.ui plugin during exception inspector initialization a new project preference node /project/org.eclipse.jdt.debug is created. This conflicts with the purpose of project preferences where the hierarchy is /<scope>/<project_in_workspace>/<plugin_that_stores_preferences> and this is somewhat confusing how it is possible that without any project in workspace we get a node that corresponds to org.eclipse.jdt.debug project stored in workspace. See the output from the Preferences Spy view to observe changing preferences hierarchy.
Is there any real/visible impact?
(In reply to comment #1) > Is there any real/visible impact? I have not found any apart from broken hierarchy.
What is the value of the preference? We have only one reference to ProjectScope in all of JDT debug and it is only used to look up a builder setting.
(In reply to comment #3) > What is the value of the preference? We have only one reference to > ProjectScope in all of JDT debug and it is only used to look up a builder > setting. There is no value set, only empty node is created. To reproduce: 1. Open new workspace. 2. Open Preferences Spy view and verify there is no child node for /project. 3. Open Run > Debug configurations... 4. Create new Java Application configuration. 5. Close the dialog. 6. Refresh Preferences Spy view by using "Collapse All". => Verify there are two new nodes added under /project: org.eclipse.debug.core and org.eclipse.jdt.debug.
(In reply to comment #4) > (In reply to comment #3) > > What is the value of the preference? We have only one reference to > > ProjectScope in all of JDT debug and it is only used to look up a builder > > setting. > > There is no value set, only empty node is created. To reproduce: > 1. Open new workspace. > 2. Open Preferences Spy view and verify there is no child node for /project. > 3. Open Run > Debug configurations... > 4. Create new Java Application configuration. > 5. Close the dialog. > 6. Refresh Preferences Spy view by using "Collapse All". > => Verify there are two new nodes added under /project: > org.eclipse.debug.core and org.eclipse.jdt.debug. These are coming from calls to the preference service to ask for preferences. The debug.core one comes from: String preferred = Platform.getPreferencesService().getString(DebugPlugin.getUniqueIdentifier(), LaunchManager.PREF_PREFERRED_DELEGATES, IInternalDebugCoreConstants.EMPTY_STRING, null); and the jdt.debug one comes from: boolean doit = Platform.getPreferencesService().getBoolean( JDIDebugPlugin.getUniqueIdentifier(), IJDIPreferencesConstants.PREF_OPEN_INSPECT_POPUP_ON_EXCEPTION, false, null); debugging through the code and it looks like if you ask for a node that does not exist a new empty node is created - in the platform resources code, not debug. Moving to platform resources for comment. Is there a way we should be asking for preferences that will not create empty nodes?
(In reply to comment #5) > The debug.core one comes from: > > String preferred = > Platform.getPreferencesService().getString(DebugPlugin.getUniqueIdentifier(), > LaunchManager.PREF_PREFERRED_DELEGATES, > IInternalDebugCoreConstants.EMPTY_STRING, null); > > and the jdt.debug one comes from: > > boolean doit = Platform.getPreferencesService().getBoolean( > JDIDebugPlugin.getUniqueIdentifier(), > IJDIPreferencesConstants.PREF_OPEN_INSPECT_POPUP_ON_EXCEPTION, > false, > null); > > debugging through the code and it looks like if you ask for a node that does > not exist a new empty node is created - in the platform resources code, not > debug. > > Moving to platform resources for comment. Is there a way we should be asking > for preferences that will not create empty nodes? The #node method must create new node as this is part of preferences API. The point is that you should not be looking for your preference in project scope, because it does not seem to be project related. In other words, passing 'null' as the last argument in the calls above does not seem to be right.
As Szymon said, the context should be something like: new IScopeContext[]{Instance.INSTANCE, Configuration.INSTANCE} unless you really want to look int the project scope. Then the context would be: new IScopeContext[] { new ProjectScope(project)}
(In reply to comment #7) > As Szymon said, the context should be something like: > new IScopeContext[]{Instance.INSTANCE, Configuration.INSTANCE} > unless you really want to look int the project scope. Then the context would > be: > new IScopeContext[] { new ProjectScope(project)} I made a test change to supply the instance scope which makes no difference, the project node is created anyway - see PreferencesService#getNodes: if (!found) { Preferences node = getRootNode().node(scopeString).node(qualifier); if (childPath != null) node = node.node(childPath); result.add(node); } We are using the preferences API as is allowed, I am inclined to mark this as wontfix, unless resources wants to make some changes to prevent the service from creating dead nodes.
(In reply to comment #8) > We are using the preferences API as is allowed, I am inclined to mark this > as wontfix, unless resources wants to make some changes to prevent the > service from creating dead nodes. I guess we can help here, but the actual fix needs to be done in the preferences bundle. Moving to Equinox/Compendium. Patch will follow.
Created attachment 224928 [details] Patch v.0.1
Created attachment 224929 [details] Test v.0.1
John, can you review?
Looks good, thanks Szymon. Pushed: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=a7d93237b86540767274bc3f8ba79f92b3cbe562 http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=19d6f419af8e26620ef478378218c189427d16c8
The fix is not good. It causes preferences to be 'null' now, see e.g. 397290. I verified that all works fine with the old version of PreferenceService.
*** Bug 397290 has been marked as a duplicate of this bug. ***
Yes the fix is bad because in the case of default scope, the node is only initialized at the moment it is created. So just because the node does not exist does not mean it will not have any values once created. I have reverted the change: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=82bd6fb64c976d01579a027852569486ce721df3 This commit reverts Szymon's tests, and introduces a new regression test to verify that the new regression is fixed: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=22612df8dbfd9c7ef978673f1c69549037f26871
Verified in N20130103-2000 that the NPE is gone.
The root cause of regression is that DefaultPreferences#nodeExists does not take into consideration possibility that node may get be created programatically in initializer. We had similar problem with #nodeExists in bug 388004, so I guess the right fix would be to extend DefaultPreferences#nodeExists to cover nodes created in runtime. Since initializers can be 3rd party code, all we can do is to try to run them inside #nodeExists and then check if the node got created. I tried this approach and it fixes this bug. The new regression test added by John also passes with this approach. I will attach new cumulative patches shortly.
Created attachment 225289 [details] Patch v.0.2
Created attachment 225290 [details] Test v.0.2
Created attachment 225292 [details] Patch v.0.2 Previously attached patch was not the one I wanted to upload.
Moving this to M6. John is this something you could review and release after M5?
DefaultPreferences#nodeExists needs some more love because it still misses preferences that can be set via preferences.ini file inside bundles and we definitely must not break those cases. I will adjust the patch.
Created attachment 226931 [details] Patch v.0.3 There are no additional changes in PreferencesService class compared to the fix that was previously released. The only additional modification is in the DefaultPreferences#nodeExists method.
Created attachment 226934 [details] Test v.0.3 Apart from changes that were previously released PreferencesServicesTest contains additional test for the case described in comment 23. I added new "empty" plugin for this test to make sure that the new test does not depend on other plugins and nodes. If you agree with adding this new plugin, it will require pom.xml for CBI compatibility. For now, I have skipped it so that patches contain only relevant changes.
Resetting review flag since previous fix was reverted. The new fix contains previous fix and additional changes that cover regression from comment 14 and comment 23.
Where are we at with this bug? I assume it is not addressed in M6. I am marking for M7 for now.
(In reply to comment #27) > Where are we at with this bug? Patches are up to date and waiting for review.
I get two test failures with this patch applied: org.eclipse.core.tests.internal.preferences.TestBug388004 testBug(org.eclipse.core.tests.internal.preferences.TestBug388004) junit.framework.AssertionFailedError: This node exists in pluginCustomization file. org.eclipse.core.tests.internal.preferences.TestBug380859 testBug(org.eclipse.core.tests.internal.preferences.TestBug380859) junit.framework.ComparisonFailure: expected:<[v1]> but was:<[not_found]> I'm having a hard time understanding why we actually want to fix this. Preference initialization is a complicated piece of code and this patch alters the init order based on when a client calls nodeExists. There is no user visible bug or other apparent benefit of changing this from what I can see.
(In reply to comment #29) > I get two test failures with this patch applied: > > org.eclipse.core.tests.internal.preferences.TestBug388004 > testBug(org.eclipse.core.tests.internal.preferences.TestBug388004) > junit.framework.AssertionFailedError: This node exists in > pluginCustomization file. > > org.eclipse.core.tests.internal.preferences.TestBug380859 > testBug(org.eclipse.core.tests.internal.preferences.TestBug380859) > junit.framework.ComparisonFailure: expected:<[v1]> but was:<[not_found]> > > I'm having a hard time understanding why we actually want to fix this. > Preference initialization is a complicated piece of code and this patch > alters the init order based on when a client calls nodeExists. There is no > user visible bug or other apparent benefit of changing this from what I can > see. I agree. Suggest to close as WONTFIX, but definitely -1 for Kepler.
(In reply to comment #29) > I get two test failures with this patch applied: > > org.eclipse.core.tests.internal.preferences.TestBug388004 > testBug(org.eclipse.core.tests.internal.preferences.TestBug388004) > junit.framework.AssertionFailedError: This node exists in > pluginCustomization file. > > org.eclipse.core.tests.internal.preferences.TestBug380859 > testBug(org.eclipse.core.tests.internal.preferences.TestBug380859) > junit.framework.ComparisonFailure: expected:<[v1]> but was:<[not_found]> Bug 380859 was fixed last Friday so it was not there when the patch was prepared. I have just applied the patch on current master branch and run the whole runtime suite and I don't see any failures. John, can you double-check that?
Deferring to Luna.
Bug 418046 shows an example where creating empty preference nodes may result in removing some of the preferences. The attached patches still apply on the master branches. Could we review/release for Luna early in the cycle?
Is there any chance to have this reviewed and pushed for Luna? The bug is assigned to me but I cannot push it by myself.
CC'ing John since I don't think he has seen this review request.
Moving to Mars, would be good to get a review by John.
Converted to Gerrit change: https://git.eclipse.org/r/#/c/30246/ https://git.eclipse.org/r/#/c/30247/
M1 has left the station.
(In reply to Dani Megert from comment #38) > M1 has left the station. and M2, and M3 on Friday.
From John: " Hi Szymon, My opinion hasn't changed since my comment two years ago in the bug: "I'm having a hard time understanding why we actually want to fix this. Preference initialization is a complicated piece of code and this patch alters the init order based on when a client calls nodeExists. There is no user visible bug or other apparent benefit of changing this from what I can see." I think we should close as WONTFIX and move on to something with more visible impact and user value. John " I completely agree with that. Closing as WONTFIX.
(In reply to Dani Megert from comment #40) This is a good way to tell your contributors to stop contributing. Well done.
Gerrit change https://git.eclipse.org/r/30246 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=92ae5195ee7a7685524798f74948b8fcbfeb7ccd
(In reply to Eclipse Genie from comment #42) > Gerrit change https://git.eclipse.org/r/30246 was merged to [master]. > Commit: > http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/ > ?id=92ae5195ee7a7685524798f74948b8fcbfeb7ccd Seems we have decided to fix this. Reflecting the decision in the bug resolution.
Reopening because the bug is not done yet - you need to push both changes but only the first one was merged. Two changes are needed because tests live in a different repository than the code being tested. (In reply to Szymon Ptaszkiewicz from comment #37) > Converted to Gerrit change: > > https://git.eclipse.org/r/#/c/30246/ > https://git.eclipse.org/r/#/c/30247/
(In reply to Szymon Ptaszkiewicz from comment #44) > Reopening because the bug is not done yet - you need to push both changes > but only the first one was merged. Two changes are needed because tests live > in a different repository than the code being tested. > > (In reply to Szymon Ptaszkiewicz from comment #37) > > Converted to Gerrit change: > > > > https://git.eclipse.org/r/#/c/30246/ > > https://git.eclipse.org/r/#/c/30247/ I rebased the tests, but now it seems we have a consistent failure (not related to the test changes in the review since the failure is in master). See bug 506447.
Gerrit change https://git.eclipse.org/r/30247 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=9f2e003e61774d56adf1716c8c0faa293240c19a
(In reply to Eclipse Genie from comment #46) > Gerrit change https://git.eclipse.org/r/30247 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/ > ?id=9f2e003e61774d56adf1716c8c0faa293240c19a Finally should be done now.
The change made in https://git.eclipse.org/r/#/c/30246/5/bundles/org.eclipse.equinox.preferences/src/org/eclipse/core/internal/preferences/DefaultPreferences.java is invalid since the preference node can be left in a state when runtime defaults are applied but product and command line customizations that are supposed to override the runtime defaults are not applied. This is causing bug 511138.
The changes made in this bug have been reverted: Gerrit change https://git.eclipse.org/r/89637 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=23f9b744c1083eb4fcba8e45da70fa10d2670dd3 Gerrit change https://git.eclipse.org/r/89635 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.runtime.git/commit/?id=ad0e8c85475d4df04dd3aa0068923cfac2ad3e87
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.
