Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 137398

Summary:

[Preferences] WorkingCopyPreferences attempts to set value of removed node

Product:

[Eclipse Project] Platform

Reporter:

Walter Harley <eclipse>

Component:

Resources

Assignee:

Szymon Brandys <Szymon.Brandys>

Status:

VERIFIED FIXED

QA Contact:

Severity:

major

Priority:

CC:

agrealis, benno.baumgartner, daniel_megert, dj.houghton, drews, jackie, martinae, Michael_Rennie, sieg, Szymon.Brandys, Tod_Creasey

Version:

3.2

Keywords:

helpwanted

Target Milestone:

3.4 M7

Hardware:

OS:

Windows XP

Whiteboard:

hasPatch

Bug Depends on:

Bug Blocks:

139284

Attachments:

Description	Flags
suggested fix	none
Tests update	none
Martin's patch with fixed tests	none
The patch is smaller now, but better	none

Description Walter Harley

2006-04-18 19:03:02 EDT

This is very closely related to bug 111144; that bug is marked "fixed" but I think that only a particular case was fixed and there are still other cases lurking.  This is one such case.

To repro: 
============

1. Create a new Java project, accepting all defaults.
2. In Project Properties / Java Compiler / Errors/Warnings, check "enable project specific settings" and change one of the settings.
3. Apply the change.  Say "no" when asked to rebuild (or say yes, doesn't matter).
4. No, press "Restore Defaults".  Again, Apply the change, and don't rebuild.
5. Again check "enable project specific settings".

At step 5, an Unhandled Loop Exception is caught, and an error "Preference node 
'org.eclipse.jdt.core' has been removed" is reported.  


NOTES:
=========

The call stack generating this error is, in part, as follows:
Thread [main] (Suspended (exception IllegalStateException))
	ProjectPreferences(EclipsePreferences).checkRemoved() line: 157
	ProjectPreferences(EclipsePreferences).internalGet(String) line: 513
	ProjectPreferences(EclipsePreferences).get(String, String) line: 364
	WorkingCopyPreferences.put(String, String) line: 156
	OptionsConfigurationBlock$Key.setStoredValue(IScopeContext, String, IWorkingCopyManager) line: 114
	ProblemSeveritiesConfigurationBlock(OptionsConfigurationBlock).useProjectSpecificSettings(boolean) line: 685
	ProblemSeveritiesPreferencePage.enableProjectSpecificSettings(boolean) line: 102

The basic problem is that when the Restore Defaults was applied, the Preferences code correctly discovered that there were no project-specific settings remaining in the node, so it removed the node.  But WorkingCopyPreferences never discovers that fact, so a timebomb is ticking from then on.

Notice that since the JDT options pages all share a WorkingCopyManager, but each page only modifies a subset of the jdt.core settings, there is no way that any single page can tell whether restoring defaults for that page's settings will or will not cause the node to be removed.  So it can't, e.g., cleverly call removeNode() on the WorkingCopyManager.

My personal opinion is that the "removed" state in EclipsePreferences is far too aggressively enforced.  Actions like flush() or clear() on a removed node should silently be ignored when a node is removed; actions like get() should return the default value.  Barring that, WorkingCopyPreferences/Manager need a better way of detecting and dealing with removed nodes.

Comment 1 Walter Harley

2006-05-01 19:50:54 EDT

*** Bug 139284 has been marked as a duplicate of this bug. ***

Comment 2 Yaron Mazor

2006-10-10 09:09:40 EDT

*** Bug 155413 has been marked as a duplicate of this bug. ***

Comment 3 Martin Aeschlimann

2006-11-03 02:56:26 EST

*** Bug 162924 has been marked as a duplicate of this bug. ***

Comment 4 Martin Aeschlimann

2007-05-11 04:20:09 EDT

*** Bug 186389 has been marked as a duplicate of this bug. ***

Comment 5 Martin Aeschlimann

2007-07-24 12:54:07 EDT

*** Bug 197599 has been marked as a duplicate of this bug. ***

Comment 6 Martin Aeschlimann

2007-10-31 11:02:02 EDT

*** Bug 208231 has been marked as a duplicate of this bug. ***

Comment 7 Martin Aeschlimann

2007-11-01 05:54:40 EDT

*** Bug 208368 has been marked as a duplicate of this bug. ***

Comment 8 Martin Aeschlimann

2007-11-06 04:22:04 EST

Created attachment 82171 [details]
suggested fix

Fix suggestion for core.resources: When the project preferences files in .settings is deleted, don't remove the node, just clear its settings as the nodes might still be referenced.

As Walter explains in comment 0, the preference framework internally deletes the .settings file when the last project specific setting has been cleared. By doing that it also removes the nodes by calling removeNode, making the preferences unusable for any clients still holding references on it.
Clients like the preference pages should still be able to read, modify and flush that preference. The fact that the .settings file has been removed as it isn't necessary anymore, is just an implementation detail to them.

Comment 9 Martin Aeschlimann

2007-11-06 04:25:38 EST

DJ, it would be great if you could have a look at my suggested fix.

We really need fix this problem. It's too easy to reproduce the failure.

Comment 10 Dani Megert

2007-11-06 09:15:05 EST

Note that the proposed fix is in Platform Resources.

Comment 11 DJ Houghton

2007-11-06 09:43:41 EST

The patch looks fine and it works well. One thing to note is that with the patch applied there are 2 JUnit test failures in the resources.tests. In both cases it looks like we are checking for the existence of a node after a deletion but with this patch the behavior has changed so the node still exists, but is empty.

Comment 12 Tod Creasey

2007-11-06 09:45:33 EST

Thanks Dani.

Comment 13 Krzysztof Daniel

2008-01-31 05:47:23 EST

Created attachment 88401 [details]
Tests update

I am not sure if I had make it right but I am attaching tests fix that reflects (in my opinion) changes introduces by the patch.

Comment 14 Szymon Brandys

2008-01-31 06:37:18 EST

Thanks Krzysztof. 
However I see more test that fails.
Other failures are in CharsetTest#testMovingProject and ProjectPreferencesTest#testProjectMove.

Comment 15 Szymon Brandys

2008-01-31 08:47:40 EST

Actually the issue is CharsetTest#testMovingProject and one of ProjectPreferenceSessionTest test is broken too. I'm investigating...

Comment 16 Szymon Brandys

2008-01-31 12:47:59 EST

I think that this should be solved on the UI level. 

WorkingCopyPreferences should be aware of the state of underlying ProjectPreferences. I asked Tod to look at the issue and we will try to work out the solution.

Comment 17 Martin Aeschlimann

2008-01-31 12:58:19 EST

Can you explain why you think the patch from comment 8 is not correct?

As explain in comment 8, the fact that the .settings file has been removed is just an implementation detail. You can't expect the UI to know about this and follow this.

Comment 18 Dani Megert

2008-02-01 03:33:47 EST

>I think that this should be solved on the UI level. 
-1 for that.

Comment 19 Martin Aeschlimann

2008-04-09 07:23:08 EDT

What's the state here? Any answers to the questions in comment 17.

I'd like to understand what's the problem with patch proposed in comment 8.

Comment 20 Szymon Brandys

2008-04-29 07:35:09 EDT

Created attachment 97949 [details]
Martin's patch with fixed tests

Comment 21 Szymon Brandys

2008-04-29 08:16:02 EDT

There is one more issue to handle when the patch is introduced.

The scenario is:
1) create "Project1" and "Project2" and set some project preferences on both, e.g. encoding
2) delete "Project2"
3) rename "Project1" to "Project2"
4) notice, that "Project2" uses now the OLD preferences of the removed project, it means "no preferences"
5) restart, fixes it

Comment 22 Szymon Brandys

2008-04-29 10:42:28 EDT

Created attachment 97991 [details]
The patch is smaller now, but better

Comment 23 Szymon Brandys

2008-04-29 12:57:21 EDT

The latest patch with a minor modification released to HEAD.

Comment 24 Szymon Brandys

2008-05-05 05:12:58 EDT

Verified in I20080502-0100.