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

[Maxim Kartashev]

On Wed, 13 Jul 2022 08:58:38 GMT, Sergey Bylokhov <serb at 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.
>
>> 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.
> 
> Don't we have a similar issue in the usage of `records `and `queue`? Is it possible that the `target` object in the `add `method will be deallocated before `ref `will be added to the `records`? In that case, the next code in `run` method will fail:
> 
>     Reference<?> obj = queue.remove();
>     obj.clear();
>     DisposerRecord rec = records.remove(obj);
>     rec.dispose();
> 
> Do we need Reference.reachabilityFence at the end of the `add` or some kind of synchronization?
> 
> Note that pollRemove tries to solve that:
> 
>     DisposerRecord rec = records.remove(obj);
>     if (rec == null) { // shouldn't happen, but just in case.
>          continue;
>     }
> 
> But for sure synchronization should solve that in some better way.

@mrserb Are you OK with the current state of the PR?

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

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