Closed Bug 1257517 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics: Text, defect, critical)

39 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified
firefox-esr38 --- affected
firefox-esr45 --- fixed

People

(Reporter: Fanolian+BMO, Assigned: emk)

References

()

Details

(4 keywords)

Crash Data

Attachments

(2 files)

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.
Crash Signature: [@ mozilla::BinarySearchIf<T> ] [@ mozilla::BinarySearch<T> ]
Has Regression Range: --- → yes
Has STR: --- → yes
Component: General → Graphics: Text
The crash stack clearly shows font-related code, which makes the regression range somewhat suspicious, since I don't see anything related.
oops
Sorry ignore comment#2, wrong bug.
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
Blocks: 1067989
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(gfritzsche)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Flags: needinfo?(jwalden+bmo) → needinfo?(VYV03354)
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?)
Flags: needinfo?(VYV03354)
By the way, I can't reproduce the crash and Web Console spews many errors starting with "downloadable font: not usable by platform".
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.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8732115 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f060c3d871fb721cec59b195a8080fc26ff72bd8
Bug 1257517 - Restore accidentally-removed zero check for non-default UVS offset. r=jfkthame
Flags: needinfo?(gfritzsche)
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/f060c3d871fb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8732115 - Flags: approval-mozilla-beta?
Attachment #8732115 - Flags: approval-mozilla-aurora?
(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.
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.)
Keywords: crash
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.
Attachment #8732115 - Flags: approval-mozilla-beta?
Attachment #8732115 - Flags: approval-mozilla-beta+
Attachment #8732115 - Flags: approval-mozilla-aurora?
Attachment #8732115 - Flags: approval-mozilla-aurora+
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.
Attachment #8732115 - Flags: approval-mozilla-esr45?
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Comment on attachment 8732115 [details] [diff] [review]
Restore accidentally-removed zero check for non-default UVS offset

Polishing.
Should be in 45.1.0
Attachment #8732115 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Setting the flag back for verification on 45.1.0esr.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.