On Thu, 27 Aug 2026 17:47:09 GMT, Andy Goryachev <[email protected]> wrote:
>> ## 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 chan...
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains eight additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/master' into 8373452.dataformat.2
> - Merge branch 'master' into 8373452.dataformat.2
> - review comments
> - Merge branch 'master' into 8373452.dataformat.2
> - review comments
> - review comments
> - nulls
> - data format
> The main question is whether this PR solves the problem sufficiently, or
> should we ban multi-id data formats altogether? I could not find an origin or
> any use of `java.file-list` id, it's not even a valid mime type.
In my opinion, banning multi-ids makes the model more reasonable. You can
register all the IDs you want 1-by-1 (unless there are conflicts). I would even
say that removing this class makes the model simpler because identifiers can be
registered directly as strings, but I have a feeling that this will break a bit
of code...
Whatever we can get away with is a plus as I see it.
I had an AI go over the fix because concurrency is tricky. Its findings, in
short (these things are very verbose):
* It agrees that the concurrent access issue is resolved.
* It also came up with the scenario of having duplicate identifiers: `new
DataFormat("a", "a")`, but they are handled by the contract of `Set.of`. I'm
not sure what we want to do with such an oddity, but it might be worth a test
case.
* It found the equals-inconsistent hash function that I commented about.
* It reminded that now the registered IDs will be held forever because we're
not using a weak reference anymore. I don't think that's a problem, although
what we have is first-come-first-serve, which is another argument for exposing
the `Map` methods directly where you can use `putIfAbsent` or override with
`put`.
Regarding the tests, if we're fixing a concurrency issue, there should be a
concurrent construction test.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
42:
> 40: // A static registry of DataFormats for the purposes of checking
> against constructing DataFormats
> 41: // that contain mismatched mime types.
> 42: private static final HashMap<String,DataFormat> registry = new
> HashMap<>();
Suggestion:
private static final Map<String, DataFormat> registry = new HashMap<>();
We usually code to an interface, not an implementation.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
89:
> 87: * and breaking our drag view implementation.
> 88: */
> 89: private static final DataFormat DRAG_IMAGE_OFFSET = new
> DataFormat("application/x-java-drag-image-offset");
`DRAG_IMAGE` and `DRAG_IMAGE_OFFSET` are unused (and were unused before this
PR). I don't know where they were supposed to be used, maybe there's some
missing wiring somewhere. They could possibly be removed.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
194:
> 192:
> 193: for (String id : identifiers) {
> 194: hash = 31 * hash + id.hashCode();
The hash should be `identifiers.hashCode()` to match the `equals` check.
modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java line
220:
> 218: */
> 219: public static DataFormat lookupMimeType(String mimeType) {
> 220: if (mimeType == null || mimeType.length() == 0) {
Suggestion:
if (mimeType == null || mimeType.isEmpty()) {
Minor and preexisting, so up to you.
-------------
Changes requested by nlisker (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/2197#pullrequestreview-5078276924
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3904297347
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3904294726
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3906007513
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3906093818
