RFC: Fix for JDK-8188030

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Fix for JDK-8188030

Mario Torre-6
Hi all,

I would like to propose a fix for
https://bugs.openjdk.java.net/browse/JDK-8188030.

The issue is basically that CFF fonts are considered better match than
Type 1, but are discarded, leaving the font array with no elements.

The fix is here:

http://cr.openjdk.java.net/~neugens/8188030/webrev.01/

I attached a reproducer to the bug report, but you need a somewhat
minimal system setup for that to work.

The fix is for OpenJDK 10 at this point, but I plan to backport it all
the way down to 7.

Any comments?

Cheers,
Mario
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Phil Race
This is fine for JDK 9 + 10 but the reason CFF fonts are discarded
is that we weren't supporting them properly in Oracle JDK until 9.
We should have removed this check in 9 but it was forgotten.

If you backport this to 7 and 8 it will be a problem there - for
Oracle JDK, not OpenJDK.

Although it won't matter for openjdk7 .. since Oracle isn't using that
forest any more

But it will be an issue for 8. Not sure how to handle that but
it should not be backported without a resolution there.

-phil.

On 09/27/2017 06:52 AM, Mario Torre wrote:

> Hi all,
>
> I would like to propose a fix for
> https://bugs.openjdk.java.net/browse/JDK-8188030.
>
> The issue is basically that CFF fonts are considered better match than
> Type 1, but are discarded, leaving the font array with no elements.
>
> The fix is here:
>
> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>
> I attached a reproducer to the bug report, but you need a somewhat
> minimal system setup for that to work.
>
> The fix is for OpenJDK 10 at this point, but I plan to backport it all
> the way down to 7.
>
> Any comments?
>
> Cheers,
> Mario

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-5
Hi Phil,

I think I may be fine if we need to live with a local patch in our RPM in 8, this use case should be pretty rare, although I'll check first if there’s some ifdefs that I can use for the backport to 8.

I tried to figure out if I could just change the logic but all I could get was NPEs.

Perhaps a more fine tuned search to exclude CFF in the first place, but I’m not sure how to craft the query for FontConfig in this case.

I’ll push to 10 for now and backport to 9 in the meantime. Do I need another reviewer ok?

Cheers,
Mario

On Wed 27. Sep 2017 at 23:15, Phil Race <[hidden email]> wrote:
This is fine for JDK 9 + 10 but the reason CFF fonts are discarded
is that we weren't supporting them properly in Oracle JDK until 9.
We should have removed this check in 9 but it was forgotten.

If you backport this to 7 and 8 it will be a problem there - for
Oracle JDK, not OpenJDK.

Although it won't matter for openjdk7 .. since Oracle isn't using that
forest any more

But it will be an issue for 8. Not sure how to handle that but
it should not be backported without a resolution there.

-phil.

On 09/27/2017 06:52 AM, Mario Torre wrote:
> Hi all,
>
> I would like to propose a fix for
> https://bugs.openjdk.java.net/browse/JDK-8188030.
>
> The issue is basically that CFF fonts are considered better match than
> Type 1, but are discarded, leaving the font array with no elements.
>
> The fix is here:
>
> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>
> I attached a reproducer to the bug report, but you need a somewhat
> minimal system setup for that to work.
>
> The fix is for OpenJDK 10 at this point, but I plan to backport it all
> the way down to 7.
>
> Any comments?
>
> Cheers,
> Mario

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Phil Race
I think for  JDK 8 you can pass down a flag to decide whether to include or exclude CFF.
This flag would be the value of FontUtilities.isOpenJDK() and that should tell us what we need.
That would preserve the status quo for Oracle JDK and it would be our problem if the same
bug were then reported against that release.

-phil.

On 9/27/17, 5:35 PM, Mario Torre wrote:
Hi Phil,

I think I may be fine if we need to live with a local patch in our RPM in 8, this use case should be pretty rare, although I'll check first if there’s some ifdefs that I can use for the backport to 8.

I tried to figure out if I could just change the logic but all I could get was NPEs.

Perhaps a more fine tuned search to exclude CFF in the first place, but I’m not sure how to craft the query for FontConfig in this case.

I’ll push to 10 for now and backport to 9 in the meantime. Do I need another reviewer ok?

Cheers,
Mario

On Wed 27. Sep 2017 at 23:15, Phil Race <[hidden email]> wrote:
This is fine for JDK 9 + 10 but the reason CFF fonts are discarded
is that we weren't supporting them properly in Oracle JDK until 9.
We should have removed this check in 9 but it was forgotten.

If you backport this to 7 and 8 it will be a problem there - for
Oracle JDK, not OpenJDK.

Although it won't matter for openjdk7 .. since Oracle isn't using that
forest any more

But it will be an issue for 8. Not sure how to handle that but
it should not be backported without a resolution there.

-phil.

On 09/27/2017 06:52 AM, Mario Torre wrote:
> Hi all,
>
> I would like to propose a fix for
> https://bugs.openjdk.java.net/browse/JDK-8188030.
>
> The issue is basically that CFF fonts are considered better match than
> Type 1, but are discarded, leaving the font array with no elements.
>
> The fix is here:
>
> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>
> I attached a reproducer to the bug report, but you need a somewhat
> minimal system setup for that to work.
>
> The fix is for OpenJDK 10 at this point, but I plan to backport it all
> the way down to 7.
>
> Any comments?
>
> Cheers,
> Mario

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-6
On Thu, Sep 28, 2017 at 6:23 PM, Philip Race <[hidden email]> wrote:
> I think for  JDK 8 you can pass down a flag to decide whether to include or
> exclude CFF.
> This flag would be the value of FontUtilities.isOpenJDK() and that should
> tell us what we need.
> That would preserve the status quo for Oracle JDK and it would be our
> problem if the same
> bug were then reported against that release.

Is something like that what you have in mind?

http://cr.openjdk.java.net/~neugens/8188030/webrev-jdk8.01/

Cheers,
Mario
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Phil Race
Yes, that would work .. ie preserve the status quo .. not needed in 10
though, this would be 8-only.

Are you going to push the 10 version of the fix any time soon ?

-phil.

On 10/19/2017 01:59 AM, Mario Torre wrote:

> On Thu, Sep 28, 2017 at 6:23 PM, Philip Race <[hidden email]> wrote:
>> I think for  JDK 8 you can pass down a flag to decide whether to include or
>> exclude CFF.
>> This flag would be the value of FontUtilities.isOpenJDK() and that should
>> tell us what we need.
>> That would preserve the status quo for Oracle JDK and it would be our
>> problem if the same
>> bug were then reported against that release.
> Is something like that what you have in mind?
>
> http://cr.openjdk.java.net/~neugens/8188030/webrev-jdk8.01/
>
> Cheers,
> Mario

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-6
On Mon, Oct 23, 2017 at 9:28 PM, Phil Race <[hidden email]> wrote:
> Yes, that would work .. ie preserve the status quo .. not needed in 10
> though, this would be 8-only.
>
> Are you going to push the 10 version of the fix any time soon ?

I would like to push it today, however I'm not sure if I still need a
second reviewer's OK.

I'm a bit confused on the repositories though, afaik the unified JDK
hasn't been created yet, so for 10 I guess the following is still the
valid target:

http://hg.openjdk.java.net/jdk10/client/

But for 9 the thing is more confusing, should I skip that version and
backport to it later or just wait that the unified repo is created?

Cheers,
Mario
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-5
In reply to this post by Mario Torre-6
2017-09-27 15:52 GMT+02:00 Mario Torre <[hidden email]>:

> Hi all,
>
> I would like to propose a fix for
> https://bugs.openjdk.java.net/browse/JDK-8188030.
>
> The issue is basically that CFF fonts are considered better match than
> Type 1, but are discarded, leaving the font array with no elements.
>
> The fix is here:
>
> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>
> I attached a reproducer to the bug report, but you need a somewhat
> minimal system setup for that to work.
>
> The fix is for OpenJDK 10 at this point, but I plan to backport it all
> the way down to 7.
>
> Any comments?

Hi all,

Is there a second reviewer for that (and for the 8 version of the patch)?

Cheers,
Mario
--
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens
Proud GNU Classpath developer: http://www.classpath.org/
OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards:
http://endsoftpatents.org/
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Sergey Bylokhov
Looks fine.

On 26/10/2017 07:28, Mario Torre wrote:

> 2017-09-27 15:52 GMT+02:00 Mario Torre <[hidden email]>:
>> Hi all,
>>
>> I would like to propose a fix for
>> https://bugs.openjdk.java.net/browse/JDK-8188030.
>>
>> The issue is basically that CFF fonts are considered better match than
>> Type 1, but are discarded, leaving the font array with no elements.
>>
>> The fix is here:
>>
>> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>>
>> I attached a reproducer to the bug report, but you need a somewhat
>> minimal system setup for that to work.
>>
>> The fix is for OpenJDK 10 at this point, but I plan to backport it all
>> the way down to 7.
>>
>> Any comments?
>
> Hi all,
>
> Is there a second reviewer for that (and for the 8 version of the patch)?
>
> Cheers,
> Mario
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-6
Thanks,

I just pushed:

http://hg.openjdk.java.net/jdk10/client/

I'll start the process for the backports now.

Cheers,
Mario

On Thu, Oct 26, 2017 at 9:18 PM, Sergey Bylokhov
<[hidden email]> wrote:

> Looks fine.
>
> On 26/10/2017 07:28, Mario Torre wrote:
>>
>> 2017-09-27 15:52 GMT+02:00 Mario Torre <[hidden email]>:
>>>
>>> Hi all,
>>>
>>> I would like to propose a fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8188030.
>>>
>>> The issue is basically that CFF fonts are considered better match than
>>> Type 1, but are discarded, leaving the font array with no elements.
>>>
>>> The fix is here:
>>>
>>> http://cr.openjdk.java.net/~neugens/8188030/webrev.01/
>>>
>>> I attached a reproducer to the bug report, but you need a somewhat
>>> minimal system setup for that to work.
>>>
>>> The fix is for OpenJDK 10 at this point, but I plan to backport it all
>>> the way down to 7.
>>>
>>> Any comments?
>>
>>
>> Hi all,
>>
>> Is there a second reviewer for that (and for the 8 version of the patch)?
>>
>> Cheers,
>> Mario
>>
>
>
> --
> Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Phil Race
In reply to this post by Phil Race
Mario,

Can you please request 8 backport approval for this version (which I
have approved as below)

I know some internal folks who like to see it in 8 as well and I had to
tell them you already have a fix for 8 in hand ..

The jdk-updates project is only for 9+ so the usual old 8 rules should
apply for this.

-phil.

On 10/23/2017 12:28 PM, Phil Race wrote:

> Yes, that would work .. ie preserve the status quo .. not needed in 10
> though, this would be 8-only.
>
> Are you going to push the 10 version of the fix any time soon ?
>
> -phil.
>
> On 10/19/2017 01:59 AM, Mario Torre wrote:
>> On Thu, Sep 28, 2017 at 6:23 PM, Philip Race <[hidden email]>
>> wrote:
>>> I think for  JDK 8 you can pass down a flag to decide whether to
>>> include or
>>> exclude CFF.
>>> This flag would be the value of FontUtilities.isOpenJDK() and that
>>> should
>>> tell us what we need.
>>> That would preserve the status quo for Oracle JDK and it would be our
>>> problem if the same
>>> bug were then reported against that release.
>> Is something like that what you have in mind?
>>
>> http://cr.openjdk.java.net/~neugens/8188030/webrev-jdk8.01/
>>
>> Cheers,
>> Mario
>

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-6
On Wed, Nov 15, 2017 at 5:46 PM, Phil Race <[hidden email]> wrote:

> Mario,
>
> Can you please request 8 backport approval for this version (which I have
> approved as below)
>
> I know some internal folks who like to see it in 8 as well and I had to tell
> them you already have a fix for 8 in hand ..
>
> The jdk-updates project is only for 9+ so the usual old 8 rules should apply
> for this.

Right, I was sidetracked by that discussion but this is on my list,
I'm preparing the backport request as we speak.

Cheers,
Mario
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Phil Race
Sorry .. but there may be more work needed on 8 that I forgot about.
But its for Oracle JDK to account for the fact that CFF fonts are still
being excluded there.

In a nutshell .. if we end up with zero fonts as a result .. just add
the Oracle JDK's Lucida Sans
as slot 0 font so that there is SOMETHING.

However I know you can't test this .. since you don't have that font.

So if you'd prefer to stick to what you can test I'd quite understand
and we'd handle that part separately.

-phil.

On 11/15/2017 09:07 AM, Mario Torre wrote:

> On Wed, Nov 15, 2017 at 5:46 PM, Phil Race <[hidden email]> wrote:
>> Mario,
>>
>> Can you please request 8 backport approval for this version (which I have
>> approved as below)
>>
>> I know some internal folks who like to see it in 8 as well and I had to tell
>> them you already have a fix for 8 in hand ..
>>
>> The jdk-updates project is only for 9+ so the usual old 8 rules should apply
>> for this.
> Right, I was sidetracked by that discussion but this is on my list,
> I'm preparing the backport request as we speak.
>
> Cheers,
> Mario

Reply | Threaded
Open this post in threaded view
|

Re: RFC: Fix for JDK-8188030

Mario Torre-6
On Wed, Nov 15, 2017 at 6:20 PM, Phil Race <[hidden email]> wrote:
> Sorry .. but there may be more work needed on 8 that I forgot about.
> But its for Oracle JDK to account for the fact that CFF fonts are still
> being excluded there.
>
> In a nutshell .. if we end up with zero fonts as a result .. just add the
> Oracle JDK's Lucida Sans
> as slot 0 font so that there is SOMETHING.

Right, makes sense.

> However I know you can't test this .. since you don't have that font.
>
> So if you'd prefer to stick to what you can test I'd quite understand and
> we'd handle that part separately.

I can't test that properly but I can simulate it by artificially
setting isOpenJDK false somewhere before the CFF check, this will
bring back the zero font bug again, then just add some random font I
have locally.

What's the exact name for the font I need to use exactly in the 0
slot? I can prepare the patch then let you test it in full on your
side. We're shipping the fix in our local RPM so I'm not in a rush to
push the fix, I rather prefer this is done properly.

Cheers,
Mario