Ok, the flush() function and its business in WCRenderQueue definitively deserve a closer look:
I tried to do a proof-of-concept of how to test for this bug: https://github.com/CodeMonkeyIsland/Bugtest-JDK-8229902 Its in a "its not pretty, but it works... sometimes"-state. Not ready for anything. But I discovered something interesting: Bug.java is the adjusted test (not unit test) from https://bugs.openjdk.org/browse/JDK-8229902, I passed the function in the html-document, because executeScript wasnt working. You can see the Bug there. In CanvasRenderingTest.java, I was testing out the testing strategy. Not very gracefully, I just handed over the pixelarray to be tested as a String and then decoded it into an int array again. I had some Problem with gc, I think, so I did the whole painting and extracting the imagadata with engine.executeScript(). - And the Bug doesnt show up anymore! ... I tried with bigger canvas, but it seems to work, just my test breaks at some point ... gc again I'm guessing. So definitively, the code is working as intended under some circumstances. So understanding when it works and when it doesnt work, probably means understanding the actual problem. Ill definitively have a deeper look into this sometime next week. And hopefully define the problem better. kind regards Dani Sent with [Proton Mail](https://proton.me/mail/home) secure email. On Monday, October 21st, 2024 at 6:19 PM, Kevin Rushforth <kevin.rushfo...@oracle.com> wrote: > 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.
