[ http://issues.apache.org/jira/browse/HARMONY-1751?page=comments#action_12453659 ]
Stepan Mishura commented on HARMONY-1751:
-----------------------------------------
Hi Ilya, Elena.
Sorry for delay with patches evaluation; I agree that Harmony implementation is not compatible
with RI and we should fix it. The problems I see here are that the class behavior is OS-depended,
not explicitly specified (we don't know how SPI methods were implemented by RI) and hard to
test the class implementation (we depend on backing store's state). I was going to fix patches
myself but I've realized that there are a lot of changes and it'd better to discuss them first.
1) Let's review the patch for RegistryPreferencesImpl.java first - it fixes the next methods:
- childSpi(String name);
- putSpi(String name, String value),
- removeNodeSpi()
The patch suggests ignoring error code returned by native API and printing a warning instead
of throwing SecurityException. I disagree with this approach.
The spec. for AbstractPreferences class says: "The remaining SPI methods putSpi(String,String),
removeSpi(String) and childSpi(String) have more complicated exception behavior. They are
not specified to throw BackingStoreException, as they can generally obey their contracts even
if the backing store is unavailable ... There is one circumstance under which putSpi, removeSpi
and childSpi should throw an exception: if the caller lacks sufficient privileges on the underlying
operating system to perform the requested operation. This will, for instance, occur on most
systems if a non-privileged user attempts to modify system preferences..."
So we should throw SecurityException if error code is ERROR_ACCESS_DENIED.
For example, you may wish to try the following test with RI:
- open registry with registry editor
- find HKEY_LOCAL_MACHINE / SOFTWARE / JavaSoft / Prefs entry
- deny all permission for you
- run a test that only invokes: Preferences.systemNodeForPackage(Object.class);
RI will throw SecurityException. And Harmony with the suggested patch applied will print
a warning only.
>From my point of view a native code should be fixed first. For example, childSpi(String)
methods invokes getNode() that has the following algorithm:
- invoke openRegKey() that in turn tries to open registry key and if it fails to open the
key it creates it.
- checks error code returned by openRegKey() and if err!=ERROR_SUCCESS returns
- the next step is confusing for me: so we have err==ERROR_SUCCESS now but the method invokes
RegCreateKeyEx again and returns dwDisposition==REG_CREATED_NEW_KEY value (that always is
false in this case)
I'd fix it in the next way: first try to create a registry key and check returned error code.
If the err== ERROR_ACCESS_DENIED just simply verify if the key exists (by invoking RegOpenKeyEx).
Otherwise return the error code.
A minor note: RI uses logging API for printing warnings instead of System.out. Frankly, I'm
now quite sure what we should use.
2) Now the patch for the test.
I agree that behavior depends on backing store's state so BackingStoreException can be thrown.
(BTW, I'd also catch SecurityException for systemNodeForPackage(Object.class) see example
above)
But I was thinking how to make the testing more predictable (I like automated testing and
dislike to edit registry keys by hands). For example, just crazy idea, to develop a native
library for testing that let the test verify registry permissions. So the test can check permissions
for registry keys first and them it can verify that a method does the right things. Is this
possible?
A minor note: I'd pass to systemNodeForPackage() PreferencesTest.class instead of Object.class.
So if the test fails to remove created key it is clear who put this garbage to the registry.
:-)
Thoughts?
Thanks,
Stepan.
> Classlib test org.apache.harmony.prefs.tests.java.util.prefs.PreferencesTest.testSystemNodeForPackage()
fails
> -------------------------------------------------------------------------------------------------------------
>
> Key: HARMONY-1751
> URL: http://issues.apache.org/jira/browse/HARMONY-1751
> Project: Harmony
> Issue Type: Bug
> Components: Classlib
> Reporter: Elena Semukhina
> Assigned To: Stepan Mishura
> Attachments: H-1751_PreferencesTest_updated.patch, Harmony-1751.patch, PreferencesTest.patch,
TestPref.java
>
>
> The test fails for me on Windows on both j9 and drlvm with the following assertion:
> ant -Dbuild.module=prefs -Dtest.case=PreferencesTest test
> testSystemNodeForPackage Error N/A
> (J9)
> java.lang.SecurityException at java.util.prefs.RegistryPreferencesImpl.childSpi(RegistryPreferencesImpl.java:116)
at java.util.prefs.AbstractPreferences.getNodeFromBackend(AbstractPreferences.java:645) at
java.util.prefs.AbstractPreferences.nodeImpl(AbstractPreferences.java:626) at java.util.prefs.AbstractPreferences.node(AbstractPreferences.java:597)
at java.util.prefs.Preferences.systemNodeForPackage(Preferences.java:767) at org.apache.harmony.prefs.tests.java.util.prefs.PreferencesTest.testSystemNodeForPackage(PreferencesTest.java:62)
at java.lang.reflect.AccessibleObject.invokeV(AccessibleObject.java:25)
> (drlvm)
> java.lang.SecurityException at java.util.prefs.RegistryPreferencesImpl.childSpi(RegistryPreferencesImpl.java:116)
at java.util.prefs.AbstractPreferences.getNodeFromBackend(AbstractPreferences.java:644) at
java.util.prefs.AbstractPreferences.nodeImpl(AbstractPreferences.java:625) at java.util.prefs.AbstractPreferences.node(AbstractPreferences.java:595)
at java.util.prefs.Preferences.systemNodeForPackage(Preferences.java:767) at org.apache.harmony.prefs.tests.java.util.prefs.PreferencesTest.testSystemNodeForPackage(PreferencesTest.java:62)
at java.lang.reflect.VMReflection.invokeMethod(Native Method)
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira