On Mon, 4 Jul 2022 09:31:05 GMT, Maxim Kartashev <[email protected]> wrote:
> Java2D's `Disposer` maintains a record of objects to dispose of with the help > of a collection that isn't thread safe. When `DisposerRecords` objects are > being added to it at the same time as others are being disposed on the > Toolkit thread, chaos ensues. > > This commit replaces the collection with a thread-safe one, more > consistently guards against exceptions in individual disposers, and adds > exception's stacktraces printing in order to facilitate said exceptions' > debugging, which are otherwise hard to pinpoint without code modification. > > Originally, the bug was caught on MacOS running an existing test > (`DrawRotatedStringUsingRotatedFont`) that would occasionally crash the VM > (probably due to double-free detected by libc that abort()'s in this case). > It may take many re-tries to reproduce and this wasn't observed on Linux. > The new test (`test/jdk/sun/java2d/Disposer/TestDisposerRace.java`) displays > the problem in a more reliable fashion and fails both on MacOS and Linux > without this fix. (as a side note: many thanks for such an in-depth review). > What I can imagine is that the storage change may not yet be visible to the > Disposer thread, or becomes visible as it is in the process of clearing the > deferred records. In the specific macOS case I'm sure that's the right explanation for the crash. But `Disposer` is a public class that is being widely used throughout JDK and as far as I can see nothing in its contract prevents it from being used in a way I described and illustrated with the second test. Perhaps I was unnecessarily generic in my description. > It is useless to test this in headless mode since there is no RenderQueue .. > and so this whole scenario never happens I think it sufficient to make the > EXISTING test headful .. the harness will report a failure automatically if > there's a crash Let me drop that test and make the existing one headful. > Both new tests don't seem needed and the 2nd one doesn't seem to be testing > the issue at all .. as I wrote above we do not run these concurrently and > your test does nothing to block the disposer thread. On the issue of the second test, I still believe it is useful: - it demonstrates a legitimate problem with `Disposer`, albeit admittedly a different one than reported in the bug, - it shows the problem more clearly as the problematic usage pattern of `Disposer` is contained in the test itself and not buried in JDK code, - it crashes more reliably, - it shows that the problem (at least potentially) exists on Linux also, - it does not have to be marked headful. ------------- PR: https://git.openjdk.org/jdk/pull/9362
