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

[Phil Race]

I don't think it is related to the LCD text work. I think it was the JDK 7 b17 fix:
6263951: Java does not use fast AA text loops when regular AA hint is set.
So there's a requirement that renderLoops is non-null in some case when
they would not previously have been used.

-phil.

Jim Graham wrote:
> 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
>>