P4
There seems to be several issues with DataFormat API and implementation discovered during a Clipboard-related code review:
1. static DataFormat::lookupMimeType(String) is not thread safe: while iterating over previously registered entries in the DATA_FORMAT_LIST another thread might create a new instance (DataFormat L227)
2. public DataFormat(String...) constructor might throw an IllegalArgumentException if one of the given mime types is already assigned to another DataFormat. The origin of this requirement is unclear, but one possible issue I can see is if the application has two libraries that both attempt to create a DataFormat for let's say "text/css". Then, depending on the timing or the exact code path, an exception will be thrown for which the library(-ies) might not be prepared. The constructor is also not thread safe.
3. To avoid a situation mentioned in #2, a developer would is typically call lookupMimeType() to obtain an already registered instance, followed by a constructor call if such an instance has not been found. An example of such code can be seen in webkit/UIClientImpl:299 - but even then, despite that two-step process being synchronized, the code might still fail if *some other* library or the application attempts to create a new instance of DataFormat, since the constructor itself is not synchronized.
4. DataFormat(new String[] { null }) is allowed but makes no sense!
Why do we need to have the registry of previously created instances? Unclear. My theory is that the DataFormat allows to have multiple mime-types (ids) - example being DataFormat.FILES = new DataFormat("application/x-java-file-list", "java.file-list"); - and the registry was added to prevent creation of a DataFormat with just one id for some reason.
What should be done?
- find out why we need this registry in the first place i.e. what could happen if we have multiple DataFormat instances with overlapping ids.
- if the registry is needed add a new factory method, something like DataFormat::of(String ...) which is properly synchronized. This method will be called by the constructor to retain the backward compatibility.
- deprecate (possibly for removal) DataFormat::lookupMimeType(String), or keep it but have it properly synchronized
Dangers.
Adding synchronization might lead to deadlocks if the application or library has existing code synchronized around some other object and not DataFormat.class.
1. static DataFormat::lookupMimeType(String) is not thread safe: while iterating over previously registered entries in the DATA_FORMAT_LIST another thread might create a new instance (DataFormat L227)
2. public DataFormat(String...) constructor might throw an IllegalArgumentException if one of the given mime types is already assigned to another DataFormat. The origin of this requirement is unclear, but one possible issue I can see is if the application has two libraries that both attempt to create a DataFormat for let's say "text/css". Then, depending on the timing or the exact code path, an exception will be thrown for which the library(-ies) might not be prepared. The constructor is also not thread safe.
3. To avoid a situation mentioned in #2, a developer would is typically call lookupMimeType() to obtain an already registered instance, followed by a constructor call if such an instance has not been found. An example of such code can be seen in webkit/UIClientImpl:299 - but even then, despite that two-step process being synchronized, the code might still fail if *some other* library or the application attempts to create a new instance of DataFormat, since the constructor itself is not synchronized.
4. DataFormat(new String[] { null }) is allowed but makes no sense!
Why do we need to have the registry of previously created instances? Unclear. My theory is that the DataFormat allows to have multiple mime-types (ids) - example being DataFormat.FILES = new DataFormat("application/x-java-file-list", "java.file-list"); - and the registry was added to prevent creation of a DataFormat with just one id for some reason.
What should be done?
- find out why we need this registry in the first place i.e. what could happen if we have multiple DataFormat instances with overlapping ids.
- if the registry is needed add a new factory method, something like DataFormat::of(String ...) which is properly synchronized. This method will be called by the constructor to retain the backward compatibility.
- deprecate (possibly for removal) DataFormat::lookupMimeType(String), or keep it but have it properly synchronized
Dangers.
Adding synchronization might lead to deadlocks if the application or library has existing code synchronized around some other object and not DataFormat.class.