1257517 - Typekit script/font crashes [@ mozilla::BinarySearchIf<T> ], [@ mozilla::BinarySearch<T> ]

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

[Fanolian]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160316030233

Steps to reproduce:

Visit https://blogs.adobe.com/CCJKType/2016/03/the-missing-link.html


Actual results:

Nightly crashes.

Some crash reports:
https://crash-stats.mozilla.com/report/index/e0bf1753-aa49-4d93-91b3-7b2d62160317
https://crash-stats.mozilla.com/report/index/65710e5d-26c7-4b1b-b493-ddf3c2160317
https://crash-stats.mozilla.com/report/index/6ff1d7b0-fe1d-4624-898d-7e75f2160317


From Mozregression,
Last good Nightly: 2015-03-19
First bad Nightly: 2015-03-20
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf1060d8ce9f7cfaa1ab9be4a6c062f8de5abd95&tochange=4d2d97b3ba34d95b0d29ce099f8f5dc0a2ae722d

Further bisection:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf1060d8ce9f7cfaa1ab9be4a6c062f8de5abd95&tochange=0a11b73c77b78b08ea55b05e368a6522a34fa251


I can stop the crash by doing either of the following:
1. Disable javascript by Developer Tool before loading the affected page;
2. Forbid javascript from typekit.net by NoScript;
3. Block widget/tracker from "Typekit by Adobe" by Ghostery; or 
4. Save the affected page (complete page by Chrome) locally and load it in Nightly. (Since the webfont can't be downloaded)

I don't see any bug reports regarding fonts in the regression range, and thus I think this issue is javascript-related.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The crash stack clearly shows font-related code, which makes the regression range somewhat suspicious, since I don't see anything related.

Comment 3

[Alice0775 White]

oops
Sorry ignore comment#2, wrong bug.

Comment 4

[Alice0775 White]

I can also reproduce the crash on Firefox35 with new profile.
bp-46dc0f8f-f16b-4c41-89e9-d609f2160317

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=28f303a138f1&tochange=a63b9ceeb754

Regressed by:c984a4104674	Georg Fritzsche — Bug 1067989 - Unify some more binary search uses. r=waldo

Comment 5

[Alice0775 White]

Attachment: archived html (for the record)

Comment 6

[Jeff Walden [:Waldo]]

So if those three crash reports are representative, the problem clearly resides in gfxFontUtils::MapUVSToGlyphFormat14, and in these changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c984a4104674#l9.62

The BinarySearch/BinarySearchIf interface is horribly difficult to work with, IMO, concerning direction of comparisons and such.  :-(  So I'm not too surprised if somehow this was miswritten with a backwards comparison or something.

But I just read through it all now, and for the life of me I can't see how the old code there does anything different from what the current code does.  In principle I could probably debug this, but I'm not sure it's the best use of time for a non-domain-expert to look at this, as opposed to someone who actually knows how the code should behave at all, beyond its binary-searchiness.

Maybe we could get Masatoshi Kimura to look at this, seeing as it appears he introduced this code?

Comment 7

[Masatoshi Kimura [:emk]]

Binary search is binary search. This code is doing nothing more than binary search. If you find nothing obvious wrong, I suggest reverting the change. (Maybe the compiler dislikes the template/function object magic?)

Comment 8

[Masatoshi Kimura [:emk]]

By the way, I can't reproduce the crash and Web Console spews many errors starting with "downloadable font: not usable by platform".

Comment 9

[Masatoshi Kimura [:emk]]

Ah I got it. VarSelectorRecords can contain zero in the nonDefaultUVSOffset field. Even if the record is found, nonDefaultUVSOffset can be zero. So this rewrite is not equivalent to the old code:
>-    if (!nonDefUVSOffset) {
>+    size_t index;
>+    if (!BinarySearch(Format14CmapWrapper(*cmap14),
>+                      0, cmap14->numVarSelectorRecords, aVS, &index)) {
>         return 0;
>     }

Patch is coming.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Restore accidentally-removed zero check for non-default UVS offset

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f060c3d871fb721cec59b195a8080fc26ff72bd8
Bug 1257517 - Restore accidentally-removed zero check for non-default UVS offset. r=jfkthame

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> Ah I got it. VarSelectorRecords can contain zero in the nonDefaultUVSOffset
> field. Even if the record is found, nonDefaultUVSOffset can be zero.

Thanks!  I assumed I was misreading somehow, just couldn't see it.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> By the way, I can't reproduce the crash and Web Console spews many errors
> starting with "downloadable font: not usable by platform".

FTR, this is reproducible only if HWA (DirectWrite) is enabled.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f060c3d871fb

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 8732115 [details] [diff] [review]
Restore accidentally-removed zero check for non-default UVS offset

Approval Request Comment
[Feature/regressing bug #]: bug 1067989
[User impact if declined]: reproducible crash
[Describe test coverage new/current, TreeHerder]: Tested manually
[Risks and why]: Very low. Only adding back a error check for an edge case.
[String/UUID change made/needed]: none

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> [Describe test coverage new/current, TreeHerder]: Tested manually

No reason we can't land a test for this, right?  Even if it only kicks into play as a crash with DirectWrite, it's still extra assurance of this code being right, and continuing to be right in the future.

Comment 17

[Masatoshi Kimura [:emk]]

If I found a font tool that can generate a test font with zero non-default UVS offset, I'll create a test. At least FontForge always uses non-default UVS offset to avoid Win7 bug.
(I assume Typekit font itself is unusable due to license issues.)

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8732115 [details] [diff] [review]
Restore accidentally-removed zero check for non-default UVS offset

Crash fix for a regression since 35. Let's try this for the beta 5 build.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73efa9d61ae0

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9d60fba2210c

Comment 21

[Masatoshi Kimura [:emk]]

Comment on attachment 8732115 [details] [diff] [review]
Restore accidentally-removed zero check for non-default UVS offset

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes a reproducible crash bug, so this is a stability fix.
User impact if declined: Crash persists during one more ESR cycle.
Fix Landed on Version: 46 or later
Risk to taking this patch (and alternatives if risky): Very low, only restores a check for an edge case.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this crash with Firefox 45.0.1 on Windows 10 x86.

Confirming the fix across platform using the following builds:
*Firefox 46.0b6
*Latest 47.0a2 Aurora
*Latest 48.0a1 Nightly.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8732115 [details] [diff] [review]
Restore accidentally-removed zero check for non-default UVS offset

Polishing.
Should be in 45.1.0

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/9893fbddad1b

Comment 25

[Andrei Vaida [:avaida]]

Setting the flag back for verification on 45.1.0esr.