Closed Bug 970483 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: Text, defect)

28 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 + verified
firefox29 + verified
firefox30 + verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: lizzard, Assigned: emk)

References

Details

(Keywords: crash, regression, topcrash-win, Whiteboard: topcrash)

Crash Data

Attachments

(4 files, 2 obsolete files)

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".
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?
Flags: needinfo?(lhenry)
It is a startup crasher.

topcrash:
#12 on Nightly(Fx30) 
#37 on Aurora(Fx29)
#8 on Beta(Fx28)
Keywords: topcrash-win
OS: Windows NT → Windows 7
(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.
Flags: needinfo?(lhenry)
That is odd that the urls are different, but they point to the same thing!
(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.
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.
Marking this as a regression from bug 925599, as I strongly suspect that's what triggered the spike in crashiness.
Blocks: 925599
Keywords: regression
Assignee: nobody → VYV03354
Why prerelease versions are still running in the first place?
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.
Also changed aVersion to uint32_t to make the comparison faster on 32-bit platforms.
Attachment #8373682 - Flags: review?(benjamin)
Attachment #8373683 - Flags: review?(jfkthame)
Fixed a silly mistake.
Attachment #8373682 - Attachment is obsolete: true
Attachment #8373682 - Flags: review?(benjamin)
Attachment #8373685 - Flags: review?(benjamin)
s/ull/ul/g;
Attachment #8373685 - Attachment is obsolete: true
Attachment #8373685 - Flags: review?(benjamin)
Attachment #8373718 - Flags: review?(benjamin)
Attachment #8373683 - Flags: review?(jfkthame) → review+
Attachment #8373718 - Flags: review?(benjamin) → review?(netzen)
Attachment #8373718 - Flags: review?(netzen) → review+
>   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.
Whiteboard: topcrash → topcrash [leave open]
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
Attachment #8374780 - Flags: review?(netzen) → review+
I renamed the function because the new logic will return true for Vista or earlier.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b068af7b8d
Whiteboard: topcrash [leave open] → topcrash
https://hg.mozilla.org/mozilla-central/rev/41b068af7b8d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
We should verify that the DirectWrite font list is not disable on all supported platforms (including Win7 RTM without any SP).
(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?
> 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.
Flags: needinfo?(jfkthame)
The are no reports of this crash with Nightly builds after this fix was checked in.
(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.
Flags: needinfo?(jfkthame)
Please nominate for uplift to Aurora/Beta with a risk assessment.
Flags: needinfo?(VYV03354)
[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
Attachment #8376637 - Flags: approval-mozilla-beta?
Attachment #8376637 - Flags: approval-mozilla-aurora?
Flags: needinfo?(VYV03354)
> Also tested that the DirectWrite font backend did not regress on Win8.1, Win7 SP1, Win7 RTM.

And verified on Vista SP2.
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.
Attachment #8376637 - Flags: approval-mozilla-beta?
Attachment #8376637 - Flags: approval-mozilla-beta+
Attachment #8376637 - Flags: approval-mozilla-aurora?
Attachment #8376637 - Flags: approval-mozilla-aurora+
Flagging for verification in the next Beta/Aurora builds.
Keywords: verifyme
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
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.
(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.
> 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...
Ah, I misread the check logic, sorry. But is it guaranteed that the build number is monotonic?
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.
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 1406793
You need to log in before you can comment on or make changes to this bug.