Bugfix JDK-8229902

[Kevin Rushforth]

Hi Dani,

Welcome, and thank you for taking a look at this problem.

I don't know enough yet to suggest the best course of action, but I took 
a quick look at it. I only see the one call to the native 
RenderingQueue::flush -- from freeSpace(int) if autoFlush is true. 
RenderingQueue::flush does a Java upcall to WCRenderQueue::fwkFlush 
which then calls WCRenderQueue::flush to decode and process the buffer. 
The native RenderingQueue::flushBuffer method does a Java upcall to 
WCRenderQueue::fwkAddBuffer which will add the buffer to a linked list 
of buffers, and then allocate a new BufferData wrapper object. It then 
calls WCRenderQueue::flush if the total size of the linked list of 
buffers it too large.

One thing that should be checked is whether the call to 
WCRenderQueue::flush from native RenderingQueue::flushBuffers and then 
again from RenderingQueue::flush is causing the problem. Instrumenting 
the code on the Java side (which is usually easier if it gives you the 
information you need), might show what's going on.

-- Kevin


On 10/20/2024 7:01 AM, dani-kurmann wrote:
> 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/20241021/5eb37ce5/attachment-0001.htm>