970483 - crash in gfxDWriteFont::ComputeMetrics(gfxFont::AntialiasOption)

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-e12b6815-e215-481b-91e4-188d72140208.
=============================================================


This seems to be a startup crash that became a top crasher for 28.0b1 with about 1110 crashes for 275 instances, around build 2014020516. The signature summary is marked as having "high exploitability".

Comment 1

[Benjamin Smedberg]

https://crash-analysis.mozilla.com/bsmedberg/bug970483-nightly.csv
https://crash-analysis.mozilla.com/bsmedberg/bug970483-nightly.svg

Regression range almost certainly from the 20131122030202 to 20131123030208 nightlies. lizzard can you create the link for that range?

Comment 2

[Tracy Walker [:tracy]]

It is a startup crasher.

topcrash:
#12 on Nightly(Fx30) 
#37 on Aurora(Fx29)
#8 on Beta(Fx28)

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> https://crash-analysis.mozilla.com/bsmedberg/bug970483-nightly.csv
> https://crash-analysis.mozilla.com/bsmedberg/bug970483-nightly.svg
> 
> Regression range almost certainly from the 20131122030202 to 20131123030208
> nightlies. 

Cool data! So that range would be

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dbf94e314cde&tochange=98aa428a392c

if I'm reading things correctly.

One possible culprit in there might be bug 941638; I'll try to investigate that and see if there's a connection.

Comment 4

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

I think that might be https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=dbf94e314cde&enddate=98aa428a392c.

Comment 5

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

That is odd that the urls are different, but they point to the same thing!

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> One possible culprit in there might be bug 941638; I'll try to investigate
> that and see if there's a connection.

Trying to hit the relevant codepaths there, I'm not immediately able to reproduce a crash... though that's far from conclusive, of course.

Might also be worth looking at bug 925599.

Comment 7

[Jonathan Kew [:jfkthame]]

Hmm.... It looks to me like bug 925599 has removed the Win7 build ID check that was introduced in bug 630201, which blocked D2D/DWrite on builds prior to WINDOWS7_RTM_BUILD, which is 7600.

The crash reports here all seem to be from earlier builds - mostly 6.1.7100, with a smattering of others. Which makes me suspect that we really should be continuing to block DirectWrite (and D2D) on such systems.

Comment 8

[Jonathan Kew [:jfkthame]]

Marking this as a regression from bug 925599, as I strongly suspect that's what triggered the spike in crashiness.

Comment 9

[Masatoshi Kimura [:emk]]

Why prerelease versions are still running in the first place?

Comment 10

[(Away)]

Looks like 7100 was the Release Candidate build of Win7. People may have hacked the OS in order to keep using it for free or something.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Introduce IsWindowsBuildOrLater() and IsWin7RTMOrLater()

Also changed aVersion to uint32_t to make the comparison faster on 32-bit platforms.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Blackliist pre-RTM Windows 7 again

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Introduce IsWindowsBuildOrLater() and IsWin7RTMOrLater()

Fixed a silly mistake.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Introduce IsWindowsBuildOrLater() and IsWin7RTMOrLater()

s/ull/ul/g;

Comment 15

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c184d7e0732
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d43ac987f76

Comment 16

[Masatoshi Kimura [:emk]]

>   1.130 +    return IsWin7SP1OrLater() ||
>   1.131 +           (IsWin7OrLater() && IsWindowsBuildOrLater(7600));

Ugh, this logic was wrong. It will return false for Vista while we support DirectWrite on Vista SP2+PUP.
The logic should have been
  IsWin7SP1OrLater() || !IsWin7OrLater() || IsWindowsBuildOrLater(7600)
. I'll post a followup patch later.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Fix the logic to determine the pre-RTM Windows 7

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 8374780 [details] [diff] [review]
Fix the logic to determine the pre-RTM Windows 7

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

::: xpcom/base/WindowsVersion.h
@@ +113,5 @@
>    IsWin8OrLater()
>    { return IsWindowsVersionOrLater(0x06020000ul); }
>  
>    MOZ_ALWAYS_INLINE bool
> +  IsNotWin7PreRTM()

nit: I actually prefer the old name

Comment 19

[Masatoshi Kimura [:emk]]

I renamed the function because the new logic will return true for Vista or earlier.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c184d7e0732
https://hg.mozilla.org/mozilla-central/rev/2d43ac987f76

Comment 21

[Brian R. Bondy [:bbondy]]

Comment on attachment 8374780 [details] [diff] [review]
Fix the logic to determine the pre-RTM Windows 7

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

Yep makes sense.

Comment 22

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41b068af7b8d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41b068af7b8d

Comment 24

[u279076]

Looks like this has been fixed. The latest build reported against this crash is Firefox 30.0a1 20140211030201. We should give it a few more days before calling it verified though.

Comment 25

[Masatoshi Kimura [:emk]]

We should verify that the DirectWrite font list is not disable on all supported platforms (including Win7 RTM without any SP).

Comment 26

[u279076]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> We should verify that the DirectWrite font list is not disable on all
> supported platforms (including Win7 RTM without any SP).

How do we check this, and what are the supported platforms?

Comment 27

[Masatoshi Kimura [:emk]]

> How do we check this,
Dunno. Jonathan?

> and what are the supported platforms?

According to the System Requirements, we support WinXP SP2+, Win2003 SP1+, Windows Vista, 7, and 8.
http://www.mozilla.org/en-US/firefox/27.0/system-requirements/
Due to the complexity of the version check logic, we should at least verify it on Win7 RTM SP0.

Comment 28

[Tracy Walker [:tracy]]

The are no reports of this crash with Nightly builds after this fix was checked in.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #26)

> How do we check this

One way to tell whether you're using the GDI or DirectWrite font backend is to load this testcase:

data:text/html,<div style="font-family:Arial">
  <p style="font-weight:700">700<p style="font-weight:900">900

Under GDI fonts, both <p> elements will render with the same font (Arial Bold), so the styles of "700" and "900" will match; under DirectWrite, font-weight:900 should result in using the Arial Black face, so the "900" will be much bolder/fatter.

Expected behavior is that when hardware acceleration is enabled (and not blocked due to graphics card or driver issues), you'll see DirectWrite fonts; when it's disabled/blocked, you'll see GDI fonts - unless the gfx.font_rendering.directwrite.enabled pref is set to true, in which case you should get DirectWrite fonts even without h/w graphics acceleration.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift to Aurora/Beta with a risk assessment.

Comment 31

[Masatoshi Kimura [:emk]]

Attachment: Combined patch for landing on branches

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 925599
User impact if declined: High-volume crash
Testing completed (on m-c, etc.): on m-c. Also tested that the DirectWrite font backend did not regress on Win8.1, Win7 SP1, Win7 RTM.
Risk to taking this patch (and alternatives if risky): Low 
String or IDL/UUID changes made by this patch: None

Comment 32

[Masatoshi Kimura [:emk]]

> Also tested that the DirectWrite font backend did not regress on Win8.1, Win7 SP1, Win7 RTM.

And verified on Vista SP2.

Comment 33

[Masatoshi Kimura [:emk]]

And Windows 8.
Vista requires SP2 for DirectWrite and XP doesn't support DirectWrite at all.
So I believe I verified it on all possible supported versions.

Comment 34

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b6395bfa152
https://hg.mozilla.org/releases/mozilla-beta/rev/4a14fddfebe9

Comment 35

[u279076]

Flagging for verification in the next Beta/Aurora builds.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4a14fddfebe9

Comment 37

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

This is still crashing for Firefox 28 for builds from 20140218122424. 
 
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=gfxDWriteFont%3A%3AComputeMetrics%28gfxFont%3A%3AAntialiasOption%29#tab-reports

Comment 38

[Jonathan Kew [:jfkthame]]

The reports I see there from the 20140218122424 build show the OS version as "6.1.7100 Service Pack 3". Which is the pre-Win7-RTM build ID that we've seen problems with and aimed to blacklist; but it also reports "service pack 3", which takes precedence and makes the IsNotWin7PreRTM() function return true.[1]

So if I'm interpreting the numbers correctly, this is a pre-RTM Win7 with SP3 added. I'd guess that's not a supported configuration.

Maybe the Windows build ID should take precedence over the reported Windows version number for the purpose of this check.

Or maybe we should decide that people who are still running a pre-RTM version of Windows, and have somehow hacked in a service pack as well, deserve whatever crashes they get.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/WindowsVersion.h#119

Comment 39

[(Away)]

Windows 7 doesn't even have a Service Pack 3. So far there is only SP1. Their version APIs might be hacked or lying (there are tools that will change it for compat testing purposes, but I doubt that's the case here).

In theory you could just do:
return !IsWin7OrLater() || IsWindowsBuildOrLater(7600);
because the real Win7SP1 is build 7601. I didn't say anything earlier since it wasn't worth worrying about. And maybe it still isn't.

Comment 40

[Masatoshi Kimura [:emk]]

(In reply to David Major [:dmajor] from comment #39)
> (there are tools that will
> change it for compat testing purposes, but I doubt that's the case here).

Official VersionLie shims don't affect VerifyVersionInfo (it is a little known benefit of this function). So it must be an unofficial hack.

> In theory you could just do:
> return !IsWin7OrLater() || IsWindowsBuildOrLater(7600);
> because the real Win7SP1 is build 7601. I didn't say anything earlier since
> it wasn't worth worrying about. And maybe it still isn't.

It will exclude Vista SP2, Win8 and all later versions.

Comment 41

[(Away)]

> It will exclude Vista SP2, Win8 and all later versions.
I don't understand why. Build numbers increase based on the trunk. Win8 was 9200. Vista SP2 was 6002.

But maybe we don't care about unofficial hacks...

Comment 42

[Masatoshi Kimura [:emk]]

Ah, I misread the check logic, sorry. But is it guaranteed that the build number is monotonic?

Comment 43

[Emanuel Hoogeveen [:ehoogeveen]]

Even if it's not guaranteed to stay monotonic in the future, that part of the check only applies to versions of Windows prior to Windows 7, right? If http://www.gaijin.at/en/lstwinver.php is to be believed, they have been monotonic so far (the one I find unclear in that list is "Windows Server 2008 R2, RTM (Release to Manufacturing)" - will that get detected as being Windows 7 or later? The list does have a disclaimer that the version numbers may be wrong though).

There's also http://www.howtogeek.com/140411/learn-the-secrets-of-the-windows-build-number/ which says that each new version will start with a version number divisible by 16 and that the last 3 have increased their internal version number by 1600 each time.

Comment 44

[u279076]

Firefox 30.0a1: 
Zero crashes in the last week

Firefox 29.0a2: 
One crash in the last week with 20140209004002. We can ignore this crash since the fix didn't land until Feb 16.

Firefox 28.0b4:
11 crashes in the last week, all from the same user. This user is running Windows NT 6.1.7100 Service Pack 3 which apparently is a pre-release version of Windows 7. I do not believe we should be supporting users who are running pre-release operating systems many months/years after release.

I am marking this bug verified fixed based on this data.