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

[Jim Graham]

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