On Fri, 8 Jul 2022 22:58:08 GMT, Phil Race <p...@openjdk.org> wrote:
> The code that disposes on the rendering thread is invoked from a dispose()
> method
that was on the Disposer thread. It then waits for that to finish.
At that time the Disposer thread is blocked so not doing anything and the render
thread is free to add elements to deferredRecords.
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 and we're ready for the next iteration of
for (int i=0;i<deferredRecords.size(); i++) {
```
loop. The next call to `ArrayList.get()` here
DisposerRecord rec = deferredRecords.get(i);
will be executed on the disposer thread simultaneously with the array
relocation of `deferredRecords`. So some of the `get()` calls will return
`null` as they read from initialized, but yet-to-be-relocated chunks, and some
will read what essentially is garbage.
> Second if it is just about MT access to deferredRecords and pulling elements
> off
that queue then why aren't we getting a Java exception about concurrent
modification
instead of a crash.
The race was between `ArrayList.get()` called from `clearDeferredRecords()` on
a dedicated disposer thread and `ArrayList.add()` called from `pollRemove()` on
a _different_ thread. AFAIK there are no safeguards against concurrent
modification between those two methods (I check the implementation).
> And if we do somehow have two threads that end up trying to free the same
> data ie both executing the dispose() method of an instance
I didn't imply that `dispose()` was called twice on the same instance. I said
"probably due to double-free" (or "bad" free in the bug description) simply
because these are the checks that are usually implemented in the heap
alloc/dealloc routines, so "double-free" is likely with the second free called
from one of the `dispose()` methods; I have no idea who called the first
`free()` and when.
> BTW when we run this test since it is a headless test we would never have
> accelerated
glyphs .. and the same is true of your new tests .. so I imagine in our test
harness
and that of others too they'll all always pass.
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`. I can also simply drop it.
Be that as it may, `test/jdk/sun/java2d/Disposer/TestDisposerRace.java` doesn't
depend on any acceleration and is as removed from any 2D code as it can be. And
it also shows the problem on Linux, making the one mentioned above somewhat
superfluous.
-------------
PR: https://git.openjdk.org/jdk/pull/9362
