8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

johanvos · 2020-06-09T19:33:01Z

This addresses https://bugs.openjdk.java.net/browse/JDK-8246348

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

Reviewers

Phil Race (prr - Reviewer)
Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/249/head:pull/249
$ git checkout pull/249

A surrogate pair only counts for 1 character (and not for 2). This fixes JDK-8246348

…parate UTF 8 strings for each TextRun

bridgekeeper · 2020-06-09T19:33:29Z

👋 Welcome back jvos! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

Add test that will fail on Ubuntu 20.04 if this PR is not applied. The test is currently commented, as PGFont.getResource returns null for fonts in testing.

mlbridge · 2020-06-10T12:07:23Z

Webrevs

kevinrushforth · 2020-06-10T23:34:18Z

/reviewers 2

openjdk · 2020-06-10T23:34:30Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java

prrace · 2020-06-11T18:08:18Z

Instead of a space you could use a ZWNJ U+FEFF. Because that is also the endian-ness
mark, Unicode actually prefers you now to use U+2060 but you need to make sure these
are processed correctly by pango. Unlike a space they should have no rendering effect - not even an advance.

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java

check on 0 char not needed anymore

kevinrushforth · 2020-06-12T13:23:29Z

Once you address the question of storing a null value in the Map, the only remaining question I have is whether to use a LinkedHashMap instead of an ordinary HashMap. In general, I like the predictability of a LinkedHashMap for maps that are iterated, but in this case, I don't feel strongly about it one way or the other.

Only store the pointer to the utf8 string if its creation was successful

kevinrushforth · 2020-06-12T18:18:16Z

The fix looks good now.

As for the test, even if StubFont were updated to provide a real font, the StubToolkit doesn't load Prism (so none of the text rendering code is exercised). Do you think you could instead add a simple test or two in the system tests project instead (maybe one testing UTF16 chars and one with a 0 char)?

…airs

johanvos · 2020-06-15T08:55:34Z

As for the test, even if StubFont were updated to provide a real font, the StubToolkit doesn't load Prism (so none of the text rendering code is exercised). Do you think you could instead add a simple test or two in the system tests project instead (maybe one testing UTF16 chars and one with a 0 char)?

I added a systemtest

kevinrushforth

The new test look good. I confirm that it fails without your patch and passes with your patch. I added a few suggestions to bring it in line with current best practices.

tests/system/src/test/java/test/com/sun/javafx/font/freetype/PangoTest.java

modules/javafx.graphics/src/test/java/test/com/sun/javafx/text/TextLayoutTest.java

tests/system/src/test/java/test/com/sun/javafx/font/freetype/PangoTest.java

revert ignored test in :graphics

kevinrushforth

Looks good.

openjdk · 2020-06-15T16:53:29Z

@johanvos This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars

Reviewed-by: prr, kcr

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 18 commits pushed to the master branch:

b200891: 8244824: TableView : Incorrect German translation
b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
ba501ef: 8246357: Allow static build of webkit library on linux
a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
a78b3fb: 8242523: Update the animation and clip envelope classes
... and 8 more: https://git.openjdk.java.net/jfx/compare/2d98fe6cae71673cf1de89b39846a13d9505c4c4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate b2008913d7dbb3f38d006019a6e634f151496313.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

johanvos · 2020-06-15T17:12:33Z

/integrate

openjdk · 2020-06-15T17:13:03Z

@johanvos The following commits have been pushed to master since your change was applied:

b200891: 8244824: TableView : Incorrect German translation
b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
ba501ef: 8246357: Allow static build of webkit library on linux
a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version
853ac78: 8245282: Button/Combo Behavior: memory leak on dispose
a78b3fb: 8242523: Update the animation and clip envelope classes
1ab653c: 8244657: ChoiceBox/ToolBarSkin: misbehavior on switching skin
804ccce: 8244195: [TEST_BUG] Convert the system tests TabPanePermuteGetTabsTest to unit test
9edba9c: 8243110: SVGTest.testSVGRenderingWithPattern fails intermittently
168b7f7: 8246099: Intermittent test failures in SandboxAppTest
c41777e: 8245634: [TestBug] Enable and fix tests ignored with message "impl_cssSet API removed"
3ceee69: 8245499: Text input controls should show handles on iOS
8914bd2: 8234540: javafx.web LeakTest.testGarbageCollectability fails intermittently
16f446a: 8234876: Unit test classes should not extend Application

Your commit was automatically rebased without conflicts.

Pushed as commit bf2e972.

johanvos added 2 commits June 9, 2020 14:00

Use number of real characters when obtaining pointers in a string.

0eed97d

A surrogate pair only counts for 1 character (and not for 2). This fixes JDK-8246348

allow different TextRuns to share the same UTF 16 text, but create se…

e9184be

…parate UTF 8 strings for each TextRun

johanvos changed the title ~~[WIP] 8246348: Crash in libpango on ubuntu 20.04~~ [WIP] 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars Jun 10, 2020

Add check for 0 in text

d345c74

Add test that will fail on Ubuntu 20.04 if this PR is not applied. The test is currently commented, as PGFont.getResource returns null for fonts in testing.

johanvos changed the title ~~[WIP] 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars~~ 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars Jun 10, 2020

openjdk bot added the rfr Ready for review label Jun 10, 2020

kevinrushforth self-requested a review June 10, 2020 23:34

kevinrushforth reviewed Jun 11, 2020

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java Outdated Show resolved Hide resolved

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java Outdated Show resolved Hide resolved

kevinrushforth reviewed Jun 11, 2020

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java Show resolved Hide resolved

prrace reviewed Jun 11, 2020

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/javafx/font/freetype/PangoGlyphLayout.java Show resolved Hide resolved

process reviewer comments: start == str

998f55c

check on 0 char not needed anymore

use LinkedHashMap instead of HashMap

0d6e909

Only store the pointer to the utf8 string if its creation was successful

prrace approved these changes Jun 12, 2020

View reviewed changes

Add test for testing Pango behavior with Character(0) and surrogate p…

8c6e73a

…airs

kevinrushforth reviewed Jun 15, 2020

View reviewed changes

tests/system/src/test/java/test/com/sun/javafx/font/freetype/PangoTest.java Outdated Show resolved Hide resolved

use latest patterns in system test

cc178e7

revert ignored test in :graphics

openjdk bot removed the rfr Ready for review label Jun 15, 2020

remove trailing whitespace

889c013

openjdk bot added the rfr Ready for review label Jun 15, 2020

kevinrushforth approved these changes Jun 15, 2020

View reviewed changes

prrace approved these changes Jun 15, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jun 15, 2020

openjdk bot closed this Jun 15, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Jun 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

johanvos commented Jun 9, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 9, 2020

mlbridge bot commented Jun 10, 2020 •

edited

Loading

kevinrushforth commented Jun 10, 2020

openjdk bot commented Jun 10, 2020

prrace commented Jun 11, 2020

kevinrushforth commented Jun 12, 2020

kevinrushforth commented Jun 12, 2020

johanvos commented Jun 15, 2020

kevinrushforth left a comment

kevinrushforth left a comment

openjdk bot commented Jun 15, 2020 •

edited

Loading

johanvos commented Jun 15, 2020

openjdk bot commented Jun 15, 2020

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars #249

Conversation

johanvos commented Jun 9, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jun 9, 2020

mlbridge bot commented Jun 10, 2020 • edited Loading

Webrevs

kevinrushforth commented Jun 10, 2020

openjdk bot commented Jun 10, 2020

prrace commented Jun 11, 2020

kevinrushforth commented Jun 12, 2020

kevinrushforth commented Jun 12, 2020

johanvos commented Jun 15, 2020

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 15, 2020 • edited Loading

johanvos commented Jun 15, 2020

openjdk bot commented Jun 15, 2020

johanvos commented Jun 9, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 10, 2020 •

edited

Loading

openjdk bot commented Jun 15, 2020 •

edited

Loading