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

[Jim Graham]

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