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

[Mario Torre]

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
-- 
Mario Torre, Software Developer, http://www.jroller.com/neugens/
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com   * Tel: +49-721-663 968-44
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

USt-Id: DE216375633, Handelsregister HRB 109481, AG Mannheim
Geschäftsführer: Dr. James J. Hunt

Please, support open standards:
http://endsoftpatents.org/