[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 8289208: Test DrawRotatedStringUsingRotatedFont.java occasionally crashes on MacOS
From:       Phil Race <prr () openjdk ! org>
Date:       2022-07-22 22:55:01
Message-ID: khQg82b9Vyg4tjyTlzLe0tWh90V7luDECEhFZzQStKc=.6d599101-cffb-4190-bdc6-3891eaaeb809 () github ! com
[Download RAW message or body]

On Mon, 11 Jul 2022 10:01:18 GMT, Maxim Kartashev <mkartashev@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


[prev in list] [next in list] [prev in thread] [next in thread]