8373452: DataFormat threading and API issues

[andy-goryachev-oracle]

Summary

This PR makes deprecates the DataFormat constructor (for removal):

public DataFormat(@NamedArg("ids") String... ids)

and replaces it with

public static DataFormat of(String ... ids)

Problem

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 bullet 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

1. adding synchronization might lead to deadlocks if the application or library has existing code synchronized around some other object and not DataFormat.class.

Alternatives

We could possibly prevent the application code creating DataFormats with multiple ids by

• creating a public DataFormat(String) constructor
• allowing multiple instances with the same id by removing the registry or using the registry only for DataFormats with multiple ids

/reviewers 2
/csr

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion jfx27 to be approved (needs to be created)
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8373452: DataFormat threading and API issues (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2006/head:pull/2006
$ git checkout pull/2006

Update a local copy of the PR:
$ git checkout pull/2006
$ git pull https://git.openjdk.org/jfx.git pull/2006/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2006

View PR using the GUI difftool:
$ git pr show -t 2006

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2006.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[openjdk]

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8373452 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[mlbridge]

Webrevs

• 01: Full - Incremental (77ae53ba)
• 00: Full (f8a5552a)

[kevinrushforth]

Removing the constructor is an incompatible change.

[kevinrushforth]

This is an incompatible change. You will need to go through the usual process for such changes where it is deprecated for removal in one release and removed in a subsequent release.

[andy-goryachev-oracle]

should we also print a warning to stderr?

[andy-goryachev-oracle]

resolved offline: no, since stderr will likely be noticed by the user rather than the developer.