RFR: 8333374: Cannot invoke "com.sun.prism.RTTexture.contentsUseful()" because "this.txt" is null

[Kevin Rushforth]

On Sat, 5 Oct 2024 05:12:22 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Contrary to issue description, the problem was not because we referenced a texture after it was freed, but we referenced an RTT that we just tried to allocate and failed to do so because of lack of space in Prism's vram pool. In case of attached `WebViewAnimationTest.java` test app, the problem exists because WebView tries to allocate a new RTT for newly opened WebView scene and the Prism pool does not have 16MB needed while previous WebView is still being displayed. Only after the old WebView is disposed of, then the RTT allocation can proceed and WebView can continue as normal.
>> 
>> I looked around and did not find other solution than adding the null-checks to silence the NPE being thrown. NPE was originally in `RTImage.java`, however after adding a null check there triggered another NPE in `WCGraphicsPrismContext.java`, so I also added another null check there. Upper layers of web module seem to handle those just fine, the NPEs disappeared after that and the test still works properly once the pool gets enough room to display.
>> 
>> All of our tests run fine with this change (I do not expect any major regressions, as this happens only with a very limited memory pool scenario - it was extremely hard and time consuming to trigger this bug on the test app with default 512MB pool limit). For the test app, it might take a couple frames until the old WebView is disposed of and there is enough room for new WebView's RTT in the pool, after that the scene renders properly.
>
> modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/RTImage.java line 120:
> 
>> 118:             if (txt == null) {
>> 119:                 log.fine("RTImage::getTexture : return null because rt texture not allocated");
>> 120:                 return null;
> 
> If `RTImage.getTexture()` can return `null`, then L147 should probably also check for `null` before calling `readPixels()`.

Good question. Even before this change, `getTexture()` could have returned `null` if the resource factory is disposed (due to a lost surface). However, this check, along with a check for `txt == null`, is made in the `draw` method prior to ever calling `getTexture()`, which was why the call on L147 didn't previously needed to check. Ignoring a possible threading issue I see (more on this below), it still doesn't, since `getTexture()` only ever tries to allocate a new RTT if `txt == null`, and we will never call `getTexture()` in that case.

Regarding the possible threading issue, in the case of printing (which is the only way it gets to line 147), the draw method might not be called on the render thread. If it isn't, then we might have an issue with thread-safety (`txt` is not volatile, and I'm uncertain about the validity of checking whether the resource factory is disposed when not on the renderer thread). The safest thing to do would be to either redo those two checks inside the `runOnRenderThread` method right before calling `getTexture()` or, better still, wrap the two checks at the top of the draw method in `runOnRenderThread`. Either way, this could be done as a follow-up fix, since it is only loosely related. We should file a follow-up bug to check the thread-safety of the draw method in RTImage in the case of printing.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1590#discussion_r1792194207