RFR: 4200096: OffScreenImageSource.removeConsumer NullPointerException

[Phil Race]

On Mon, 10 Apr 2023 16:16:32 GMT, Jeremy <duke at openjdk.org> wrote:

> This resolves a 25 year old P4 ticket: a NullPointerException is printed to System.err needlessly.
> 
> This resolution involves confirming that an ImageConsumer is still registered before every notification.
> 
> I'll understand if this is rejected as unimportant, but I stumbled across this in the real world the other day and thought this was a simple enough bug to practice on.

So the repeated checks for null make it look like you are dealing with some other thread setting it to null

If that were so, I don't think this approach is correct because the repeated null checks aren't a guarantee.
It can still go null between you checking it and then de-referencing it.

But I see that add/removeConsumer are synchronized methods
And the private method sendPixels() is only called from the private method produce()
which is only called from addConsumer()

So it seems impossible for some other thread to do this.
Therefore you must be doing it to yourself on the same thread.

I tracked down that the catch of null and printStackTrace were added under this bug
https://bugs.openjdk.org/browse/JDK-4905411

There it is implied that the callbacks such as
theConsumer.imageComplete(ImageConsumer.SINGLEFRAMEDONE);
are where the null is happening

The person who suggested the fix (not the person who implemented the fix)
was the person who architected the whole image producer/consumer model and near as I can tell
the idea was that you have a programming bug removing the consumer too soon.
I'd expect that you'd see this problem consistently since there's only one thread involved
(unless the callback kicks off another thread to do it)

It also points out that you're unlikely to need the repeated checks since it can only be nulled out
during the call backs (again unless the callback starts another thread to do it - unlikely I think).

If you think what you are doing is correct (I think the architect of the code supposed otherwise)
and you want to be sure, I'd capture "theConsumer" into a local var in each of the methods
sendPixel() and produce() and check and use the local var. Then no one can ever null that
out while you are using it. Since that's the intention of the design, I think that's a safe thing
to do.
However I suggest that if the null check finds it is null on entry that there be an

ImageConsumer localConsumer = theConsumer;
if (localConsumer ==  null) {
boolean debugging = <wrap in doprivleged get property("awt.consumer.debug")>
if (debugging) {
System.err.println("theConsumer is null - did you remove the consumer before production is complete ?"); e.printStackTrace())
 }
return
}
at the beginning of both methods

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13408#issuecomment-1502348061