> ## Summary
> 
> This PR changes the behavior of `DataFormat` by allowing multiple instances 
> that contain the same set of mime types (ids).
> 
> 
> 
> ## Problem
> 
> There seems to be several issues with DataFormat API and implementation 
> discovered during review of the `Clipboard`-related code:
> 
> 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 bullet 2, a developer would typically 
> call `lookupMimeType()` to obtain the 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!
> 
> 5. The current implementation uses the `WeakReferenceQueue` which 
> theoretically might, under certain conditions, allow the application to 
> create mismatched `DataFormat`s.
> 
> 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.
> 
> Also, I could not find the origin of the multi-id requirement, or the origin 
> of the `java.file-list` id itself (it is not a valid mime type).
> 
> 
> 
> ## Solution
> 
> The proposed solution is to relax the constraint on the constructor to allow 
> multiple instances with the same set of mime types (ids).  This might require 
> changing the application code from identity compare (`==`) to ...

Andy Goryachev has updated the pull request incrementally with one additional 
commit since the last revision:

  review comments

-------------

Changes:
  - all: https://git.openjdk.org/jfx/pull/2197/files
  - new: https://git.openjdk.org/jfx/pull/2197/files/76b11d84..e5bc1fd4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jfx&pr=2197&range=02
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=2197&range=01-02

  Stats: 24 lines in 1 file changed: 2 ins; 5 del; 17 mod
  Patch: https://git.openjdk.org/jfx/pull/2197.diff
  Fetch: git fetch https://git.openjdk.org/jfx.git pull/2197/head:pull/2197

PR: https://git.openjdk.org/jfx/pull/2197

Reply via email to