691581 - [WinXP] "ASSERTION: mFUnitsConvFactor not valid" with tiny font size

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

On WinXP only (not Windows 7), tiny font sizes trigger:

###!!! ASSERTION: mFUnitsConvFactor not valid: 'mFUnitsConvFactor > 0.0f', file e:\builds\moz2_slave\try-w32-dbg\build\obj-firefox\dist\include\gfxFont.h, line 998

Comment 1

[Jonathan Kew [:jfkthame]]

Hmm, guessing this'll be a result of bug 670072, which made us round fractional font sizes to whole pixels for GDI, which makes very small sizes go to zero.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, ensure GDI font resource is created with non-zero size

Possible patch, ensure that the GDI font resource is created with a non-zero size (even if we're rounding our font size to zero) so that we don't hit degenerate cases when trying to get metrics.

Does this resolve the problem (without regressing bug 670072)? It seems OK in my testing on Win7 (using GDI fonts), but I don't have an XP machine currently set up for easy testing.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Still asserts on XP. Jonathan, is the patch in this bug still applicable?

Comment 4

[Jonathan Kew [:jfkthame]]

Probably; but AFAICS from comment 2, I never tested it under XP, and was hoping someone else would.

The other thing is, I'm not sure it's really the right place to be fixing this. Could you post a stack for where we hit the assertion? I'm wondering whether we should be short-circuiting some code path if mAdjustedSize is zero, and thereby avoid this altogether.

Comment 5

[Jonathan Kew [:jfkthame]]

Never mind, I found your try run and looked at where it's asserting. I think there's a more correct fix we can try; I'll post a patch shortly.

Comment 6

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea2efc4976b

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Don't let a zero-sized font result in assertions from FUnitsToDevUnitsFactor()

Comment 8

[John Daggett (:jtd)]

Comment on attachment 8681691 [details] [diff] [review]
Don't let a zero-sized font result in assertions from FUnitsToDevUnitsFactor()

Review of attachment 8681691 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, seems a bit dangerous to have a conversion factor that can go to zero but I guess we've commented where it's defined clearly.

Comment 9

[John Daggett (:jtd)]

Also, please include a crashtest for this.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7658cff5f572347f1d4c3eea412e71280e8d47
Bug 691581 - Don't let a zero-sized font result in assertions from FUnitsToDevUnitsFactor(). r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf33dcba0d02886e7f2c45e1bd7392a5f05d0d4
Bug 691581 - Add crashtest.

Comment 11

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/214de8010262a5a0f4e7631ebc572bcfe0afdf7c
Backed out changeset 5cf33dcba0d0 (bug 691581) for upcoming bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/aee53bd3a239fbd917ae4726d341d9e0da5c520d
Backed out changeset 6e7658cff5f5 (bug 691581) for upcoming bustage

Comment 12

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a07aed4624

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a07aed4624

Note that WinXP jobs don't run by default on Try anymore [1]. I've forced them to run on this push with mozci. Note that the crashtest was failing in comment 6 as well when they were forced to run.

[1] try: -b d -p win32 -u crashtest[Windows XP] -t none

Comment 14

[Jonathan Kew [:jfkthame]]

Ohhhhhh! Thanks. Wish I'd known that sooner.... I assumed they were just taking "forever" get scheduled for some infra reason.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11486a275847896bf97beff11d6c07ffea3611e9
Bug 691581 - Don't let a zero-sized font result in assertions from FUnitsToDevUnitsFactor(). r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a8ab5e08568fc92f8f01db5c5cefaa663ed30f
Bug 691581 - Add crashtest.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11486a275847
https://hg.mozilla.org/mozilla-central/rev/b0a8ab5e0856

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/11486a275847
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b0a8ab5e0856