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

[Jim Graham]

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.

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.

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.

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...

			...jim

Mario Torre wrote:
> Il giorno mer, 10/06/2009 alle 03.02 -0700, Jim Graham ha scritto:
>> What is the need for this fix?  Is there a bug being fixed here other 
>> than you don't like the look of the code?
>>
>> The reason I ask is that your fix requires a method call with a test for 
>> every graphics operation.  I'd prefer if we didn't add overhead unless 
>> it was necessary for correct function.
>>
>> Also, the partially initialized state issue - while that technique can 
>> "in general" lead to issues, I don't think it is problematic here.  If 
>> that is the concern then we could target removing just that line.  Any 
>> references to the field from pipeline objects is safe since they won't 
>> be installed until a validate() is called which always sets the loops. 
>> The only suspect reference to loops would be the call from 
>> checkFontInfo() which might be invoked before the first validate() so it 
>> would need the protection against null, but almost no other piece of 
>> code you've modified can ever encounter a null loops field (or if it 
>> does then some validate() code forgot to set it)...
>>
>> 			...jim
> 
> Hi Jim!
> 
> Thanks for the kind reply.
> 
> I was tracking a bug in our SDL backend when I put my eyes on this code,
> but the bug itself is not related, just thought that this kind of code
> leads to unexpected errors (I shoot myself in the foot already with this
> stuff sometime ago...).
> 
> I noticed that the references to this variable are not always protected
> by a call to validate though. If I don't set the loop in the
> constructor, and don't check for null in the getter, I get NPE in
> various places, for example:
> 
> sun.java2d.pipe.GlyphListLoopPipe.drawGlyphList
> sun.java2d.pipe.AATextRenderer.drawGlyphList
> 
> I get those two with the Java2D demo, but there are other places that
> blindly just use the loop (in fact, everywhere loops is referenced, it
> is just used without checking for null), and they trust on the fact that
> loops is just never null.
> 
> There are two solution to this in my mind, either check for null
> everywhere loops is used (which is what I proposed with the getter) or
> selectively check for null in places we know it may be null (for example
> either in AATextRenderer.drawGlyphList, GlyphListLoopPipe.drawGlyphList,
> or in general in the various calls inside SunGraphics2D that end up in
> those methods).
> 
> The third solution, that works so far, is to protect checkFontInfo()
> with a null check like you proposed, because in fact checkFontInfo is
> called before validate, and initialise there a valid instance of the
> RenderLoops, if they are null, which is the best option for
> performances, too.
> 
> To be honest, those solutions looks a bit hacky to me, because we end up
> checking in places where "it may be used" other than in places where "it
> is actually used", but for the sake of saving few bytecodes and a method
> invocation, maybe we can go this path. Or, because it seems I opened a
> can of worm, I understand if you don't want to fix this issue at all ;)
> 
> I have prepared a new webrew with the proposed fix, where I check for
> null in checkFontInfo:
> 
> http://cr.openjdk.java.net/~neugens/100068/webrev.02/
> 
> I added a small comment to make clear that this guy may be null if not
> set via validate, checkFontInfo or setLoops.
> 
> Hope this helps!
> 
> Cheers,
> Mario