JDK-8095787 : [WebView] HTML canvas clip() functionality broken
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8u5
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2014-08-13
  • Updated: 2015-06-12
  • Resolved: 2014-09-10
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8
8u40Fixed
Related Reports
Relates :  
JDK-8096612 - [WebView] "outstanding resource locks detected" when using clip in canvas
Description
The canvas clipping doesn't work as expected: 

<html>
<head>
</head>
<body onload="draw();">
<script type="text/javascript">
  function draw() {
  
      var canvas = document.getElementById("canvas");
      var ctx = canvas.getContext("2d");

      ctx.fillStyle = "blue";
      ctx.fillRect(0,0,200,200);

//      ctx.save();

      ctx.beginPath();
      ctx.moveTo(30, 30);
      ctx.lineTo(30, 70);
      ctx.lineTo(70, 70);
      ctx.lineTo(70, 30);
      ctx.lineTo(30, 30);
      ctx.clip();

      ctx.fillStyle = "green";
      ctx.fillRect(0,0,200,200);

//      ctx.restore();
  }

</script>
Canvas:<br>
<canvas id="canvas" width="200" height="200"></canvas>
</body>
</html>


if 'ctx.save()/restore()' is uncommented the script works fine. 

The reason is WCGraphicsPrismContext creates a new ClipLayer when clipping is needed and pushes the current state to the stack. The same machinery is used for context save/restore functionality. When the drawing is finished, the clip layer is not rendered on the 'primary' layer until the restore() is called.
Comments
+1
11-09-2014
Fixed: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/114951fd1301
10-09-2014
Looks good to me as well.
08-09-2014
Ok, looks fine to me. Thanks!
03-09-2014
I see your concerns and proposals. It seems to me this will not make the code much more clear, but is the subject to introduce a number of regression. So I propose to return to the original conservative fix: http://cr.openjdk.java.net/~anashaty/RT-38290/webrev.01/
02-09-2014
Oh, sorry I missed that. So, you're creating a new array... This probably won't be a perf bottle neck, however you could avoid that by introducing kind of a "fake" pop mode. Moreover, you could create a new internal StatesStack class and encapsulate all the logic into it. That would look like: states.setFakePopMode(true); while_do; states.setFakePopMode(false); The last call would restore the "currentState" ref to the last element and the pop mode to "delete". And also you could call states.get(), states.pop() etc. which looks better. This is what came to my mind, but I'll be pretty fine if you prefer to leave it as is or return to the previous version.
02-09-2014
In the new version after popping all the states, I'm restoring the whole stack. (While flushing the layers we need to pop the states one-by-one to correctly render each layer)
02-09-2014
Anton, in the previous version of the fix, flushAllLayers only peeked into the stack, but now it pops the layers out of it. So, looks like you've changed the behavior. Is that done on purpose?
02-09-2014
I think we should leave the previous version to avoid additional risks to break something. As I understand Anton already approved the fix. Peter could you please comment?
02-09-2014
Made a refactoring of the WCGraphicsPrismContext: http://cr.openjdk.java.net/~anashaty/RT-38290/webrev.refactor/ Not sure this make sense since there are much more potential regressions but not sure whether the code became significantly clearer...
29-08-2014
I was honestly trying to do that, but finally ended up with such duplication. The structure of states is originally strange: naturally this is a stack, but we have a stack (states) + top of the stack in a separate field (state) and that introduces some inconvenience in processing this structure. I'll think about refactoring the class...
29-08-2014
The fix looks good to me. I'd be completely happy if we don't have almost identical code in the restoreStateInternal and flushAllLayers methods. However I don't know how to reuse it gracefully...
29-08-2014
I also added validate/invalidate logic to avoid unnecessary layers rendering in case if the flush() is accidentally called several times. New webrev: http://cr.openjdk.java.net/~anashaty/RT-38290/webrev.01/
29-08-2014
Makes sense to me. I remember a bug about dispose() not being called on some objects, probably it was RT-19693. I'm not sure if it applies here.
28-08-2014
> Additional layers are created only when non-rectangular clipping is set... Ok, thank you. > dispose() is called explicitly for onscreen contexts Yes, I see now the call in NGWebView.renderContent. > dispose() - should dispose layers > we should call flush(); dispose(); // no functionality lost Ok, it looks like this virtually won't change the behavior for on-screen contexts. So, I don't object.
27-08-2014
> By the way, how rendering to a canvas worked before your fix? Did the buffered context flush only when JS GC collected it? As far as I understand the common pattern of using the clip() in JS is the following: ctx.save(); ..... ctx.clip(); ..... ctx.restore(); may be that's why the bug was not so visible. > But you are going to change the method for both WCBufferedContext and WCGraphicsPrismContext (which can be used as an end GC class) in this case... ? WCBufferedContext will inherit all the functionality in this case. The behavior would be the same for both: flush() - validates the underlying buffer dispose() - just disposes resources (as its name implies)
27-08-2014
> - at some point old contexts are disposed when JS GC collects them > - on dispose those context flushes outdated layers to the canvas By the way, how rendering to a canvas worked before your fix? Did the buffered context flush only when JS GC collected it? > 2. Change WCGraphicsPrismContext.dispose() which will not re-render layers,... But you are going to change the method for both WCBufferedContext and WCGraphicsPrismContext (which can be used as an end GC class) in this case... ?
27-08-2014
No, I'm not sure, but at least it should, because else we will get mem leaks
27-08-2014
Are you sure GC calls dispose() ? I thought it is only called explicitly.
27-08-2014
> Does it make sense to add a boolean param to renderLayer() which would tell it if it should dispose the layer after rendering? In order to avoid the splitting. That was my initial intention, but I thought the renderLayer() method name doesn't implicate that the rendered layer is disposed. I was actually a little bit surprised to find dispose() here. > However, when it is disposed (will it?) it looks like it will re-render all the layers (WCGraphicsPrismContext.dispose). Is it an expected behavior? Thought it will not have too much impact, but just realized that this could lead to rendering artifacts: - some code render something periodically to a canvas - each time it creates a new canvas 2d context, render to it with a clipping without save/restoreState which leads to layers remaining in this context - at some point old contexts are disposed when JS GC collects them - on dispose those context flushes outdated layers to the canvas In my mind there are two possible solutions: 1. Override dispose() in WCBufferedContext which will not re-render layers on dispose this is less risky since changes only offscreen buffer functionality 2. Change WCGraphicsPrismContext.dispose() which will not re-render layers, use only flush() and make sure it is called from every place where dispose() is called. this looks more risky, but is the better solution IMHO
27-08-2014
Does it make sense to add a boolean param to renderLayer() which would tell it if it should dispose the layer after rendering? In order to avoid the splitting. Also, could you please clarify the following. From your comments: 72 // WCBufferedContext might be referenced from a client JS code and thus 73 // couldn't be disposed right after rendering operations on it. However, when it is disposed (will it?) it looks like it will re-render all the layers (WCGraphicsPrismContext.dispose). Is it an expected behavior?
27-08-2014
> I actually asked about rendering to a canvas in general (not connecting to clipping) Additional layers are created only when non-rectangular clipping is set (sorry for not mentioned this) (I also saw another operation which uses this). For canvas these layers are not rendered unless context.restore() is called or dispose() > the dispose() method is triggered by GC (I thought it should be called somewhere explicitly, like Peter mentioned). I'm not sure whether the dispose() is actually triggered by GC for canvas. > Well, before doing that, we should at least understand when the dispose() method is called. dispose() is called explicitly for onscreen contexts and is not called explicitly for buffered contexts > Also, your new method renders the layers but doesn't dispose them, whereas previously they were disposed. So this functionality will be lost, right? IMHO: renderAllLayers - should render layers ( == flush()) dispose() - should dispose layers Thus for onscreen context we should call flush(); dispose(); // no functionality lost for buffered context we should call flush() and dispose() should normally be called by the GC
27-08-2014
I actually asked about rendering to a canvas in general (not connecting to clipping) with regard to your comment that the dispose() method is triggered by GC (I thought it should be called somewhere explicitly, like Peter mentioned). > WCBufferedContext will inherit all the functionality in this case. The behavior would be the same for both: Well, before doing that, we should at least understand when the dispose() method is called. I can't be sure this is a harmless change for now. Also, your new method renders the layers but doesn't dispose them, whereas previously they were disposed. So this functionality will be lost, right?
27-08-2014
The webrev could be viewed here: http://cr.openjdk.java.net/~anashaty/RT-38290/webrev.00/ The fix has been tested with LayoutTests: canvas, fast/canvas
20-08-2014
When the WebPage is rendered the WCGraphicsPrismContext is disposed after render cycle which causes restoring all the internal states and flushing all the internal layers (clip and transparent layers). In case of HTML canvas the graphics context (backed by WCBufferedContext) couldn't be disposed while it is referenced from the client JS code but at the same time the canvas backBuffer should be up-to-date after the rendering operations on it are completed. In other words we need to 'flush' the context after each RenderQueue processing.
15-08-2014
The very preliminary fix draft: restore all saved states on Rendering queue completion.
13-08-2014