Review Reqeust for Bug 100068 - SunGraphics2D exposes a reference to itself while non fully initialised

[Mario Torre-4]

Hello all!

While hacking on Cacio I've found that SunGraphics2D exposes a reference
to "this" inside the constructor to another class, while initialising a
field that contains the RederingLoops.

I filed a bug report and proposed a patch for review:

https://bugs.openjdk.java.net/show_bug.cgi?id=100068

The webrew is here:

http://cr.openjdk.java.net/~neugens/100068/webrev.01/

There is not much to say about the rationale for the bug/fix, just that
the code looks a bit borked to me with those public references (there
are others around, I think I should fix them all at some point), but the
real problem is indeed exposing "this" in the constructor.

I hope I did the webrew correctly, as I had other patches around and no
patch queue on this tree (yeah, yeah, I know...) so I had to do some
manual tricks to make webrew happy, but the patch should be complete.

I tested with all the swing apps that come with OpenJDK, as well as
those two nice things:

http://java.sun.com/products/java-media/2D/samples/index.html

I'll bug you every day if you make me wait too much about that review :)

Have fun,
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: [phone]
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/

[Jim Graham-3]

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

Mario Torre wrote:

> Hello all!
>
> While hacking on Cacio I've found that SunGraphics2D exposes a reference
> to "this" inside the constructor to another class, while initialising a
> field that contains the RederingLoops.
>
> I filed a bug report and proposed a patch for review:
>
> https://bugs.openjdk.java.net/show_bug.cgi?id=100068
>
> The webrew is here:
>
> http://cr.openjdk.java.net/~neugens/100068/webrev.01/
>
> There is not much to say about the rationale for the bug/fix, just that
> the code looks a bit borked to me with those public references (there
> are others around, I think I should fix them all at some point), but the
> real problem is indeed exposing "this" in the constructor.
>
> I hope I did the webrew correctly, as I had other patches around and no
> patch queue on this tree (yeah, yeah, I know...) so I had to do some
> manual tricks to make webrew happy, but the patch should be complete.
>
> I tested with all the swing apps that come with OpenJDK, as well as
> those two nice things:
>
> http://java.sun.com/products/java-media/2D/samples/index.html
>
> I'll bug you every day if you make me wait too much about that review :)
>
> Have fun,
> Mario

[Mario Torre-4]

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: [phone]
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/

[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

[Mario Torre-4]

Il giorno ven, 12/06/2009 alle 10.13 -0700, Phil Race ha scritto:
> > 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.

Hi Phil!

No, because this is with the "plain" OpenJDK and not with my
SurfaceData. This is because all the users of this field expect a valid
non null value, but if we don't set it in the constructor, only the
calls that happens after validate() will see a non null value. Having
this null check in checkFontInfo also makes the NPE disappear in the
Java2D demo, thus obtaining the same effect as initialising in the
constructor, but without exposing "this". It has to be checked if there
are other possible location where this variable is used non initialised,
but I doubt. It's not really robust though if we depend on this
initialisation in the constructor the way it is in my opinion. Not that
is a serious bug, and in fact we could even say it's not a bug because
the code works, but it's not correct in my point of view and looks easy
to fix, so... Just want to be sure that the proposed solution doesn't
add new bugs in some corner cases or doesn't add overhead, so who knows
the code better has the last word on that, of course.

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: [phone]
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/

[phil.race]

Mario,

Did you, or can you, share a test case (and any platform specific info
needed) to repro this?

-phil.

Mario Torre wrote:

> Il giorno ven, 12/06/2009 alle 10.13 -0700, Phil Race ha scritto:
>  
>>> 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.
>>    
>
> Hi Phil!
>
> No, because this is with the "plain" OpenJDK and not with my
> SurfaceData. This is because all the users of this field expect a valid
> non null value, but if we don't set it in the constructor, only the
> calls that happens after validate() will see a non null value. Having
> this null check in checkFontInfo also makes the NPE disappear in the
> Java2D demo, thus obtaining the same effect as initialising in the
> constructor, but without exposing "this". It has to be checked if there
> are other possible location where this variable is used non initialised,
> but I doubt. It's not really robust though if we depend on this
> initialisation in the constructor the way it is in my opinion. Not that
> is a serious bug, and in fact we could even say it's not a bug because
> the code works, but it's not correct in my point of view and looks easy
> to fix, so... Just want to be sure that the proposed solution doesn't
> add new bugs in some corner cases or doesn't add overhead, so who knows
> the code better has the last word on that, of course.
>
> Cheers,
> Mario
>  

[Mario Torre-4]

Il giorno ven, 12/06/2009 alle 18.41 -0700, Phil Race ha scritto:
> Mario,
>
> Did you, or can you, share a test case (and any platform specific info
> needed) to repro this?
>
> -phil.

Hi Phil!

I think I didn't explained myself very well :)

The purpose of my change is to remove the line of code that initialises
the loops from the constructor:

@@ -254,11 +254,10 @@
         font = f;
         if (font == null) {
             font = defaultFont;
         }
 
-        loops = sd.getRenderLoops(this);
         setDevClip(sd.getBounds());
         invalidatePipe();
     }
 
     protected Object clone() {

The reason I want to do this is to avoid a reference to "this"
to be passed to an external class because SG2D may not be fully initialised,
and I would say that this is at least non nice :)

As for the NPE, I was referring to the fact that just removing the call to
sd.getRenderLoops(this) (which is, again, what I want to do), leaves the loops
uninitialised. This happens in the Java2D demo, for example, because validate
is not called in all the cases as the first thing.

A solution could be to put a check for null in checkFontInfo, like Jim suggested,
because this is called before validate.

I'll prepare a test case for this issue based on the J2D demo,
but of course it's only useful to show the NPE if you remove the
sd.getRenderLoops(this) line from the constructor.

I hope this clarifies a bit my idea.

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: [phone]
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/

[Jim Graham-3]

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

[Jim Graham-3]

Hi Mario,

Ah, I can see that I was behind a bit in my emails.

The question I still have is why the loops can be null while valid
pipelines are installed?

The SG2D starts life with invalid pipes installed, which means that
*ALL* graphics operations get vectored through a validate process before
any work is done.  Since no pipeline operation objects are installed at
that point (i.e. all pipeline fields in SG2D have a reference to an
InvalidPipe instance installed in them), then no references to loops can
occur.  When the InvalidPipe methods are invoked it validates the SG2D,
which both installs real pipelines and fills in the loops member, and
then reinvokes on the new pipelines - which should be OK since the loops
field should have been installed by then.  So, some other bug must
exist, one of:

- Some validatePipe implementation in some SurfaceData class is
installing a loop-based pipeline without initializing the loops

- Some code is calling a method on a loop-based pipeline that it did not
get from the SG2D itself (i.e. it got it from a different SG2D, but
invoked it on this SG2D - or it had a static reference to a pipeline
object and just invoked it directly rather than invoking the one that
was installed on the SG2D it is using).  All calls to pipeline objects
should be using the paradigm of "sg2d.fooPipe.Render(sg2d, ...)".

- A race condition where thread A is validating the pipeline and
installs the pipeline objects but hasn't yet reached the code to install
the loops while thread B starts rendering using that SG2D thus invoking
an operation on a partially initialized pipeline - in this case the NPE
is appropriate and allowed since we don't support multi-threaded use of
the Graphics2D objects.

                        ...jim

Mario Torre wrote:

> Il giorno ven, 12/06/2009 alle 18.41 -0700, Phil Race ha scritto:
>> Mario,
>>
>> Did you, or can you, share a test case (and any platform specific info
>> needed) to repro this?
>>
>> -phil.
>
> Hi Phil!
>
> I think I didn't explained myself very well :)
>
> The purpose of my change is to remove the line of code that initialises
> the loops from the constructor:
>
> @@ -254,11 +254,10 @@
>          font = f;
>          if (font == null) {
>              font = defaultFont;
>          }
>  
> -        loops = sd.getRenderLoops(this);
>          setDevClip(sd.getBounds());
>          invalidatePipe();
>      }
>  
>      protected Object clone() {
>
> The reason I want to do this is to avoid a reference to "this"
> to be passed to an external class because SG2D may not be fully initialised,
> and I would say that this is at least non nice :)
>
> As for the NPE, I was referring to the fact that just removing the call to
> sd.getRenderLoops(this) (which is, again, what I want to do), leaves the loops
> uninitialised. This happens in the Java2D demo, for example, because validate
> is not called in all the cases as the first thing.
>
> A solution could be to put a check for null in checkFontInfo, like Jim suggested,
> because this is called before validate.
>
> I'll prepare a test case for this issue based on the J2D demo,
> but of course it's only useful to show the NPE if you remove the
> sd.getRenderLoops(this) line from the constructor.
>
> I hope this clarifies a bit my idea.
>
> Cheers,
> Mario

[Roman Kennke-5]

Hi there,

first of all, let me say that - especially in the light of this thread,
I support Mario's change (removal of this one line from the constructor)
for the following reasons:

- It should not be necessary as you say, this field should always be
initialized before use by validatePipe().
- If it's not, it's most likely a bug.
- Therefore this line is only good for hiding bugs.

One thing is not quite clear to me:

> - A race condition where thread A is validating the pipeline and
> installs the pipeline objects but hasn't yet reached the code to install
> the loops while thread B starts rendering using that SG2D thus invoking
> an operation on a partially initialized pipeline - in this case the NPE
> is appropriate and allowed since we don't support multi-threaded use of
> the Graphics2D objects.

I was always under the assumption, that Java2D should be thread safe.
And in many places we make sure it is, e.g. in the SurfaceData objects.
But now you say, it isn't. So what is the real thing about Java2D and
thread safety. Is it only supposed to be thread safe when each thread
gets its own Graphics2D object? I think I remember back in the GNU
Classpath days we had a test case that shared a Graphics (no Graphics2D
back then..) object between threads and was supposed to be ok, but I
might be wrong here... Can you enlighten me what is the situation w/
Java2D and thread safety?

/ Roman

[Jim Graham-3]

My design goal was "doesn't corrupt anything if run from multiple
threads" (in other words don't require synchronization in any common
places just to keep it functional and don't require it to be used from a
particular thread), but only correct behavior if run from 1 thread at a
time.

In other words, it can be used by multiple threads in sequence, but not
simultaneously.

                        ...jim

Roman Kennke wrote:

> Hi there,
>
> first of all, let me say that - especially in the light of this thread,
> I support Mario's change (removal of this one line from the constructor)
> for the following reasons:
>
> - It should not be necessary as you say, this field should always be
> initialized before use by validatePipe().
> - If it's not, it's most likely a bug.
> - Therefore this line is only good for hiding bugs.
>
> One thing is not quite clear to me:
>
>> - A race condition where thread A is validating the pipeline and
>> installs the pipeline objects but hasn't yet reached the code to
>> install the loops while thread B starts rendering using that SG2D thus
>> invoking an operation on a partially initialized pipeline - in this
>> case the NPE is appropriate and allowed since we don't support
>> multi-threaded use of the Graphics2D objects.
>
> I was always under the assumption, that Java2D should be thread safe.
> And in many places we make sure it is, e.g. in the SurfaceData objects.
> But now you say, it isn't. So what is the real thing about Java2D and
> thread safety. Is it only supposed to be thread safe when each thread
> gets its own Graphics2D object? I think I remember back in the GNU
> Classpath days we had a test case that shared a Graphics (no Graphics2D
> back then..) object between threads and was supposed to be ok, but I
> might be wrong here... Can you enlighten me what is the situation w/
> Java2D and thread safety?
>
> / Roman
>

[Jim Graham-3]

I should clarify that what I meant by "doesn't corrupt anything" is that
the world won't come to an end or the system crash.  It's protected
against harm from MT, but it doesn't support MT.

And another way of stating it is that we won't consider race condition
bugs when it is used in an MT fashion, but we would consider fixing bugs
that cause it to crash or that cause a G2D to latch onto a specific thread.

It's probably also a good place to let slip that a certain demo that was
mentioned previously in the thread has always been considered "poorly
written" and I wouldn't put it past it to be violating the threading
design goals we have.  But I would never come right out and say
something like that since it serves as a good "make sure even kooky
things continue to work" telltale...  [goes back to looking nonchalant]

                        ...jim

Jim Graham wrote:

> My design goal was "doesn't corrupt anything if run from multiple
> threads" (in other words don't require synchronization in any common
> places just to keep it functional and don't require it to be used from a
> particular thread), but only correct behavior if run from 1 thread at a
> time.
>
> In other words, it can be used by multiple threads in sequence, but not
> simultaneously.
>
>             ...jim

[Mario Torre-4]

Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto:

> It's probably also a good place to let slip that a certain demo that was
> mentioned previously in the thread has always been considered "poorly
> written" and I wouldn't put it past it to be violating the threading
> design goals we have.  But I would never come right out and say
> something like that since it serves as a good "make sure even kooky
> things continue to work" telltale...  [goes back to looking nonchalant]
>

Ugh... don't tell me I'm debugging with a wrong test case... :S

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: [phone]
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/

[Jim Graham-3]

Not necessarily.  The demo serves to test a wide variety of 2D
functionality all in one place.  It has worked very well to point out
real bugs in the past that we have fixed, though it does have some
questionable practices inside it that can turn up false bugs and I
believe its abuse of threading is one such practice.

Send me (off-line) the full stack traces of the failures you were seeing
and I'll check and see if they occur in areas where it might be trying
to do graphics from multiple threads at once...

                        ...jim

Mario Torre wrote:

> Il giorno mar, 16/06/2009 alle 12.08 -0700, Jim Graham ha scritto:
>
>> It's probably also a good place to let slip that a certain demo that was
>> mentioned previously in the thread has always been considered "poorly
>> written" and I wouldn't put it past it to be violating the threading
>> design goals we have.  But I would never come right out and say
>> something like that since it serves as a good "make sure even kooky
>> things continue to work" telltale...  [goes back to looking nonchalant]
>>
>
> Ugh... don't tell me I'm debugging with a wrong test case... :S
>
> Cheers,
> Mario

[Mario Torre-4]

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
--
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: [phone]
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/
--
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: [phone]
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/

In various place after some time the demo is started or when selecting the
ArcsCurves panel:

In Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
        at sun.java2d.pipe.AATextRenderer.drawGlyphList(AATextRenderer.java:40)
        at sun.java2d.pipe.GlyphListPipe.drawString(GlyphListPipe.java:72)
        at sun.java2d.SunGraphics2D.drawString(SunGraphics2D.java:2808)
        at demos.Arcs_Curves.Arcs.drawDemo(Arcs.java:128)
        at DemoSurface.paint(DemoSurface.java:303)
        at javax.swing.JComponent.paintToOffscreen(JComponent.java:5180)
        at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:302)
        at javax.swing.RepaintManager.paint(RepaintManager.java:1232)
        at javax.swing.JComponent._paintImmediately(JComponent.java:5128)
        at javax.swing.JComponent.paintImmediately(JComponent.java:4938)
        at DemoSurface.paintImmediately(DemoSurface.java:275)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:818)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:770)
        at javax.swing.RepaintManager.prePaintDirtyRegions(RepaintManager.java:712)
        at javax.swing.RepaintManager.access$700(RepaintManager.java:60)
        at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1647)
        at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:235)
        at java.awt.EventQueue.dispatchEvent(EventQueue.java:603)
        at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:286)
        at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:201)
        at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:191)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:186)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:178)
        at java.awt.EventDispatchThread.run(EventDispatchThread.java:139)

After selecting the Tranform panel:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
        at sun.java2d.pipe.GlyphListLoopPipe.drawGlyphList(GlyphListLoopPipe.java:49)
        at sun.java2d.pipe.GlyphListPipe.drawGlyphVector(GlyphListPipe.java:137)
        at sun.java2d.pipe.ValidatePipe.drawGlyphVector(ValidatePipe.java:171)
        at sun.java2d.SunGraphics2D.drawGlyphVector(SunGraphics2D.java:2883)
        at sun.font.ExtendedTextSourceLabel.handleDraw(ExtendedTextSourceLabel.java:193)
        at sun.font.Decoration.drawTextAndDecorations(Decoration.java:122)
        at sun.font.ExtendedTextSourceLabel.draw(ExtendedTextSourceLabel.java:197)
        at java.awt.font.TextLine.draw(TextLine.java:774)
        at java.awt.font.TextLayout.draw(TextLayout.java:2638)
        at demos.Transforms.SelectTx.drawDemo(SelectTx.java:210)
        at DemoSurface.paint(DemoSurface.java:303)
        at javax.swing.JComponent.paintToOffscreen(JComponent.java:5180)
        at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:302)
        at javax.swing.RepaintManager.paint(RepaintManager.java:1232)
        at javax.swing.JComponent._paintImmediately(JComponent.java:5128)
        at javax.swing.JComponent.paintImmediately(JComponent.java:4938)
        at DemoSurface.paintImmediately(DemoSurface.java:275)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:818)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:770)
        at javax.swing.RepaintManager.prePaintDirtyRegions(RepaintManager.java:712)
        at javax.swing.RepaintManager.access$700(RepaintManager.java:60)
        at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1647)
        at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:235)
        at java.awt.EventQueue.dispatchEvent(EventQueue.java:603)
        at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:286)
        at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:201)
        at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:191)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:186)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:178)
        at java.awt.EventDispatchThread.run(EventDispatchThread.java:139)

[Jim Graham-3]

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
>

[Mario Torre-4]

Il giorno mar, 16/06/2009 alle 16.18 -0700, Jim Graham ha scritto:
> Ah, I think I see the problem.  Phil can chime in here if I'm wrong.

Yay, glad it helped :)

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

That makes completely sense to me.

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

I'll try that tomorrow. I can also help with the testing, I would like
to see those changes in the main code base at some point, and they will
be surely useful for Cacio too.

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

Eh... I've seen this in the FontManager code too, and in Classpath we
had similar problems for some of the most obscure things too, that's an
obvious consequence of code that has to preserve backward
forever-compatibility, but I have to say that the Java2D code does some
really cool things in many places :)

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

That would be interesting :)

Cheers (and good night!),
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: [phone]
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/
--
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: [phone]
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/

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

[Jim Graham-3]

I guess the thing that bothers me here is that the cases which
"incidentally" rather than "intentionally" rely on the text loops may be
using whatever text loops happened to have been installed by a prior
validation pipeline, because they clearly are not installing any.  If
those pipelines can use a text loop then they need to "intentionally"
install the right text loop rather than rely on whatever is already
installed, otherwise the wrong rendering attributes (composite mode or
paint type, etc.) may be used, no?

So, doesn't this represent a (potential) bug which won't be fixed even
if we do lazy initialization of the loops?

I go back to my previous comment that perhaps what we need is for
invalidatePipe to set the loops to null and then fix all the places
where we were relying on "old loops" by setting them intentionally in
the corresponding validate sequences...

                        ...jim

Phil Race wrote:

> 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-4]

Il giorno mer, 17/06/2009 alle 14.07 -0700, Jim Graham ha scritto:

> I go back to my previous comment that perhaps what we need is for
> invalidatePipe to set the loops to null and then fix all the places
> where we were relying on "old loops" by setting them intentionally in
> the corresponding validate sequences...
>

I think the exceptions attached in my last mail are the two most obvious
places to start, because the other pipes install a new loops
via validatePipe().

But I'll debug it a little further and send you some updates asap.

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: [phone]
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/