On Tue, 31 Mar 2020 05:33:35 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:

> This is a regression of 
> [JDK-8212034](https://bugs.openjdk.java.net/browse/JDK-8212034).
> When image is loaded in WebView usinga url, WebView attempts to load a image 
> frames with partial image data. This was
> implemented under, JDK-8153148 -> WCImageDecoderImpl.addImageData() -> calls 
> loadFrames() with partial image data.
> 
> Call to jpeg_read_header() may fail when the partial image data has 
> incomplete header information.
> 
> In the given case the jpeg_read_header() call fails and code execution flow 
> enters the 'if
> (setjmp(jerr->setjmp_buffer)) {}' block and results in call to 
> disposeIIO(env, data);, which in turn calls
> imageio_dispose. This will free cinfo->err and set it to NULL, and the 
> subsequent call to (*cinfo->err->format_message)
> crashes.  Verified All test run, Sanity tests with Ensemble app and Tested 
> different web pages. Added a test, The test
> passes with fix and causes a native crash without the fix.

The fix looks good. I can also confirm that the test fails (crashes) without 
your fix and passes with your fix.

The new LoadCorruptJPEGTest.java test doesn't use Robot, so it should be moved 
to a package not underneath
`test.robot`. For consistency with similar tests in `javafx.graphics`, I 
recommend `test.com.sun.javafx.iio`.

I left a few other comments on the test.

tests/system/src/test/java/test/robot/javafx/iio/LoadCorruptJPEGTest.java line 
64:

> 63:         Util.runAndWait(() -> {
> 64:             URL resource = this.getClass().getResource("corrupt.jpg");
> 65:             FileInputStream input = null;

Minor: It's generally preferred to use `LoadCorruptJPEGTest.class` rather than 
the `getClass()` method call.

tests/system/src/test/java/test/robot/javafx/iio/LoadCorruptJPEGTest.java line 
69:

> 68:             } catch (FileNotFoundException e) {
> 69:                 e.printStackTrace();
> 70:             }

The `FileNotFoundException` should cause the test to fail. Rather than doing a 
try / catch, I recommend adding `throws
Exception` to the test method.

tests/system/src/test/java/test/robot/javafx/iio/LoadCorruptJPEGTest.java line 
88:

> 87:             });
> 88:             stage.setAlwaysOnTop(true);
> 89:             stage.show();

This call is not needed.

tests/system/src/test/java/test/robot/javafx/iio/LoadCorruptJPEGTest.java line 
116:

> 115:
> 116:     private static void waitForLatch(CountDownLatch latch, int seconds, 
> String msg) {
> 117:         try {

This method is unused and can be removed.

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

PR: https://git.openjdk.java.net/jfx/pull/154

Reply via email to