Bugfix JDK-8229902

[dani-kurmann]

Hi everyone, nice to be here.

I don't want to be overzealous, but I did sign up for a bugfix :)
So here is me, socializing:

I ran into the bug in a similiar scenario as described in the Bug report: https://bugs.openjdk.org/browse/JDK-8229902

It's a buffer-problem. The culprit is the flush()-call on line 64 of /modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/RenderingQueue.cpp (in the freeSpace(int size) function)

if (m_buffer && !m_buffer->hasFreeSpace(size)) {
flushBuffer();

if (m_autoFlush) {
flush();
}

}

this gets called, before a shape(or possibly something else) is added to the bytebuffer. If there is not enough space left in the buffer, here we go:

The flushBuffer()-Call works fine. It sets up new Buffer Space as intended and, I guess, flushes the buffer.
The flush()-call however leads to the described Bug. Just comment it out to test it. But thats not a bugfix, of course.

Its very interesting where the two calls lead, this is right in the sweet spot between java and c++ and openjdk and webkit. I dont claim to understand half of it. But I do find it very interesting.

This is the point where I ask for help: I can propose a bugfix, but I want to understand the issue deeper. Maybe this is a newbie-problem, but I have problems understanding the above mentioned sweet spot. So any advice is very welcome!

Please DO correct me, and show me that other approach that I dont see, but the way I see it, there's two ways to approach this thing:
A) Treat it as an initialisation-problem. e.g. There is nothing wrong with RenderingQueue, it just needs to be initialised with m_autoFlush=false. (in this case, at least)
B) There seems to be something strange going on in RenderingQueue, understand it and fix it.

A) is a lot easier: just follow back to where the RenderingQueue Constructor gets called: /modules/javafx.web/src/main/native/Source/WebCore/platform/graphics/java/ImageBufferJavaBackend.cpp, line 82:

auto context = makeUnique<GraphicsContextJava>(new PlatformContextJava(wcRenderQueue, true));

The second Argument for the PlatformContext-constructor sets the m_autoFlush-Variable upon which the flush()-call in the freeSpace-function in RenderingQueue.cpp is conditional. Set it to false.
as a git commit: https://github.com/CodeMonkeyIsland/jfx/commit/96f23307c7b6f324bac416b90c0eac4ad40b13a8

pro:
-simple fix
-no need to mess around in more than one file
-looks like this is a normal way to use RenderingQueue
con:
-more and more feels like a hack

B) is can of worms: You can comment out not only the flush()-call in the freespace-function, but the flush() function itself. At first, I thought, that the flush()-function is still needed for a flush-call during flushBuffer() outside RenderingQueue. But that was not the same flush()-function. I just tested that, and it works. (havent checked yet, if this breaks something else)
So assuming, the flush()-function is really defunct and not needed(and I would have to dive into this a lot deeper to make that statement as a fact), this is a chance to do some spring-cleaning in RenderingQueue. Again, I only see two main ways of approaching this:
- make m_autoFlush obsolete. Since (if the assumption above is right) it already flushes regardless of how m_autoFlush is set, lets stop pretending. Remove m_autoFlush, adjust the constructors and the constructor calls outside RenderingQueue.
- make m_autoFlush great again :) It would basically mean separating flushBuffer() into two functions, one that does the addBuffer-part and one that does the flushing. Then do the addBuffer unconditional and the flushing conditional on m_autoFlush().
The "problem" here being, that other parts of the codebase seem to have gone the way of approach A). So another piece of code initializing RenderingQueue with m_autoFlush=false doesnt mean that they dont want to autoFlush. So every call of the constructor has to be analysed, if the intention is to autoflush or not. I dont see the use-case for not flushing. So this approach might very well lead to a situation, where every single constructor call sets m_autoFlush to true and m_autoFlush is kind of obsolete again. Or not. Anyone has an example, where it shouldnt flush? Maybe there is runtime to be optimised by planning when to flush? I dont know...

pro:
-if there is a problem in RenderingQueue, then thats the place to fix it! No matter the consequences.
con:
-even if its an easy fix in RenderingQueue, it will change its behaviour. Or at the very least make autoFlush obsolete. This has consequences.

Approach B) seems a lot scarier than A), but if it must be done, it should be done. If you guys think this is the rabbit hole to be, I would be willing to go there. However, I don't feel competent to overview all the consequences of this. I could really need some advice and help on that road, I guess thats why Im here.
I could imagine doing A) as a temporary fix and an exercise. Then exploring B) deeper with some help.

As for testing: At first glance, this seems like a non-automated Mk.1 eyeball-test. On a closer look however, you could do something like the Bug-Class in the bug-report, extract the RenderingContext-imagedata, pass it from js to java and check the color of the pixel, to see if a shape has been drawn. A window would have to be opened for this test, I dont know if thats allowed. (But why not, I guess)

So what do you guys think? Does that sound like a plan? Does this break something, that I don't see?
I am new here. Anyone willing to have a look into this and maybe point me in the right direction sometimes?

kind regards

Dani Kurmann

p.s. the Bug class in the bug report does not work anymore.(at least for me, forked or unforked) the script in executeScript doesn't get executed. As a quick fix, you can pass the function in the html document like so:

WebEngine engine = webView.getEngine();
String content="<html><body><canvas id=\"canvas\" width=\"600\" height=\"600\" style=\"border: 1px solid red;\"></canvas><script>window.onload = function() {";
content=content
+ " var canvas = document.getElementById('canvas');"
+ " var c = canvas.getContext('2d');"
+ " var size = 4;"
+ " var step = size + 2;"
+ " for (var y = 0; y <= canvas.height; y = y + step) {"
+ " for (var x = 0; x <= canvas.width; x = x + step) {"
+ " c.fillRect(x, y, size, size);"
+ " }"
+ " }";
content=content+"}</script></body></html>"; engine.loadContent(content);

Sent with [Proton Mail](https://proton.me/mail/home) secure email.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241020/04af0ca2/attachment-0001.htm>