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

[Phil Race]

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

Isn't that just a bug in (I guess) your SurfaceData subclass ?

-phil.

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