RFR: 8289208: Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS

[Phil Race]

On Mon, 11 Jul 2022 10:01:18 GMT, Maxim Kartashev <mkartashev at openjdk.org> wrote:

> Correct. Now let's suppose that the rendering thread - unblocked at the time - is adding more records such that
> ArrayList needs to grow. Now ArrayList starts relocating its underlying storage array at the time as dispose() returns

First, the only way that list grows is here :
share/classes/sun/font/StrikeCache.java
static void disposeStrike(final FontStrikeDisposer disposer) {
..

rq.flushAndInvokeNow(new Runnable() {
                    public void run() {
                        doDispose(disposer);
                        Disposer.pollRemove();
                    }
                });
..
}

where Disposer.pollRemove() is adding to the ArrayList deferredRecords

This disposeStrike method() is called from the dispose() method in
share/classes/sun/font/FontStrikeDisposer.java

So the ArrayList must have FINISHED "relocating its underlying storage array" BEFORE dispose() returns.

So all the following text you have implying concurrent adds / removes etc etc doesn't make sense to me.

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.

So the change to use the ConcurrentLinkedDeque would - if it does what I suppose - fix that.
it would ensure all the changes made on the render thread are immediately visible to the dispose thread.
So let's take that change since it can only make things safer

However I don't think you've explained how it leads to the crash.
Your comment " "double-free" is likely with the second free called from one of the dispose() methods"
and the surrounding text again doesn't add up to me. I read you as saying there are two dispose() methods
but there shouldn't be two operating on the same data and the  Strike one is synchronized and ensures
it only frees once .. 

My guess - and the stack trace in the bug perhaps bears this out since it implicates BufImgSurfaceData
- nothing to do with fonts  .. is that this is a non-synchronized dispose() method
allocated from here
0x0000000107afc4e0(0x00007ff636c574a0)
java.lang.Exception
    at java.desktop/sun.java2d.DefaultDisposerRecord.<init>(DefaultDisposerRecord.java:55)
    at java.desktop/sun.java2d.Disposer.addRecord(Disposer.java:107)
    at java.desktop/sun.awt.image.BufImgSurfaceData.initRaster(Native Method)


And DrawRotatedStringUsingRotatedFont.java does cycle through a lot of BufferedImage instances too ..

> Are you implying that test/jdk/java/awt/Graphics2D/DrawString/DisposerTest.java is useless in the headless mode?
> Then I can mark it as headful.

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

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.

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

PR: https://git.openjdk.org/jdk/pull/9362