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
