[OpenJDK 2D-Dev] Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised

[Jim Graham]

Ah, I think I see the problem.  Phil can chime in here if I'm wrong.

Text now uses loops in most cases, but many of the validate methods 
assume that they don't need the loops.  I don't think that used to be 
the case, but it changed as a result of the LCD text loop work.  It used 
to be that AA used the alphafill field of SG2D and not the loops, and it 
still does for most rendering.  But now text rendering will almost 
always go through the loops and it is only working because of that call 
to getRenderLoops() in the constructor (and the fact that we never set 
it back to null.

I'd like to see invalidate() set the loops to null and see how far we 
get - I'll bet that we'd get NPEs all over the place, especially with AA 
rendering.

In the long run, this is another straw on the camel's back as far as 
rethinking the validation framework.  It's been tweaked and hacked quite 
a lot over the past 10 years and so there are a lot of issues that arise 
like this that aren't readily obvious from the code.  I don't think the 
camel's back is broken yet, but it is becoming more and more confusing 
for new engineers to start tinkering with it without seeing things break 
from seemingly innocuous changes.  :-(

For now, I'm wary of removing that call without a lot of testing.  I 
think putting a "loops=null" line in invalidatePipe() might accelerate 
some of that testing, though.

And Phil might want to chime in with a description of the new 
requirements on loops in light of the post-LCD text work...  Phil?

			...jim

Mario Torre wrote:
> Il giorno lun, 15/06/2009 alle 13.37 -0700, Jim Graham ha scritto: 
>> Hi Mario,
>>
>> How are the drawGlyphList methods called when the loops is null?  I ask 
>> because they are only ever installed on the SunGraphics2D object by 
>> virtue of a call to validatePipe() and all calls to validatePipe() 
>> should set the loops.  So, there is a bug somewhere that causes these 
>> loops to be installed without a full validate process.
> 
> Hi Jim,
> 
> So, I spent some time today (sorry for the delay, I had to find some
> free time slot for that and had to make a cake for my girl friend, which
> was the most difficult part :), and I debugged the Java2D demo with just
> the non initialised loops (so, no checks for null loops anywhere else in
> the code).
> 
> The Demo starts fine, but after some point I get the error attached in
> this mail.
> 
>> As I said in the email you quoted.  All calls from pipelines 
>> (GlyphListLoopPipe and AATextRenderer are both pipeline objects) should 
>> be safe because all calls to validatePipe() should set the loops.
> 
> I see that validatePipe is indeed called always, but sometimes the path
> that is chosen doesn't validate the rendering loop. I've seen that
> most of the time this is ok, because the loops are not used.
> 
> I guess this is the case for all the various text rendering (LCD or AA)
> in swing components, for example (is this correct?).
> 
>> Having said that I note that there are some pipelines that do not 
>> require loops and it would be OK for a call to validate that only uses 
>> such "non-loop-based" pipelines to leave the loops field uninitialized, 
>> but all validate calls which use loop-based pipes must update the loops 
>> field.
> 
> Exactly. I'm not deep into the code yet to distinguish when they are
> needed or not, but...
> 
>> Eliminating all of those uses of loops we then fall into the case where 
>> the usage in checkFontInfo is the only usage that does not occur from a 
>> pipeline...
> 
> ...this is exactly the case, putting a null check here solves the NPE.
> 
> For what I can see, at some point some component needs to paint to an
> offscreen surface (I don't think the offscreen surface is special,
> I think it's the application code that drives the bug, but anyway...).
> 
> This is the background image from the Java2D Intro pane, I think the
> other pane do something similar. Once the SunGraphics2d object is created,
> some redering hints are set. This is the application code:
> 
> g2.setRenderingHint(RenderingHints.KEY_ANTIALIASING, 
>                     RenderingHints.VALUE_ANTIALIAS_ON);
> 
> That is, turn on antialiasing, then:
> 
> g2.clearRect(0, 0, d.width, d.height);
> 
> Now what? This of course goes through validatePipe:
> 
> The first two if/else statements fall through but not this
> (SurfaceData, line 535 in OpenJDK):
> 
> } else if (sg2d.antialiasHint == SunHints.INTVAL_ANTIALIAS_ON) {
> 
> This guy sets the pipes but doesn't set the RederingLoops.
> 
> Then, again application code:
> 
> scene.render(d.width, d.height, g2);
> 
> Which after few loops finally renders the string on screen,
> which causes the crash.
> 
> So, in other words, everything goes fine until we draw text with
> the AA redering hints turned on.
> 
> Of course, before it was not failing because of the rendering loops
> were initialised in the constructor.
> 
> I'm not sure if we need to check for a null loops at the beginning
> of validatePipe or in checkFontoInfo. Maybe we can save something if we
> use checkFontInfo but I'm not so sure.
> 
> I hope this helps,
> Mario
>