RFR: 8356137: GifImageDecode can produce opaque image when disposal method changes [v2]

[Jeremy Wood]

On Tue, 6 May 2025 10:43:02 GMT, Mikhail Yankelevich <myankelevich at openjdk.org> wrote:

>> Jeremy Wood has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - 8356137: removing sentence fragment
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#discussion_r2075200131
>>  - 8356137: fixing typo
>>  - 8356137: adding empty line at bottom of file
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#discussion_r2075192144
>>  - 8356137: removing layered pass/fail pattern
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#discussion_r2075190122
>>  - 8356137: removing (c) 2002
>>    
>>    This is in response to:
>>    https://github.com/openjdk/jdk/pull/25044#discussion_r2075186796
>
> test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 28:
> 
>> 26:  * @bug 8356137
>> 27:  * @summary This test verifies a non-zero transparent pixel in gifs works when
>> 28:  * the disposal method changes from 2 to 1, and when the
> 
> Nitpick: Do you need the `, and when the` ? 😄

Thanks, this is updated

> test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 105:
> 
>> 103:                     // we don't expect this to happen, but if something goes
>> 104:                     // wrong nobody else will print our stacktrace for us:
>> 105:                     e.printStackTrace();
> 
> Do you need this print here? Runtime exception should print it out anyway to the system.error afaik

Yes, because this eventually is caught here in GifImageDecoder.java:

                    try {
                        if (!readImage(totalframes == 0,
                                       disposal_method,
                                       delay)) {
                            return;
                        }
                    } catch (Exception e) {
                        if (verbose) {
                            e.printStackTrace();
                        }
                        return;
                    }


In this specific test file: I never expect an exception to be thrown, but one did come up when I was first drafting this test (because of my own error). It was hard to debug because it was unreported. I would prefer to leave these printStackTrace calls in, in case a developer someday makes a change and needs to see potential errors.

(Technically I could try to make verbose `true`, but that's declared as a final variable and I don't want to modify GifImageDecoder.java just for this.)

> test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 133:
> 
>> 131:                     }
>> 132:                 } catch(Exception e) {
>> 133:                     e.printStackTrace();
> 
> Do you need to print out the stack trace here when you are throwing it below?

Yes, this is caught in the same place the other one is ( https://github.com/openjdk/jdk/pull/25044#discussion_r2075930867 )

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075932570
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075930867
PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075932044