Closed Bug 1250669 Opened 4 years ago Closed 4 years ago

Fallback to GDI fonts if Device Creation fails on the content process

Categories

(Core :: Canvas: 2D, defect, critical)

47 Branch
x86_64
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Keywords: crash, regression, Whiteboard: gfx-noted)

Crash Data

Attachments

(2 files)

If the device creation fails on content process, we have skia canvas with a cairo content backend and dwrite fonts. Dwrite fonts shouldn't be supported without d2d, but we don't properly fall back to gdi fonts.
We'd still initialize dwrite fonts even if we didn't have a content device [1]. Thus, we'd still use Dwrite fonts even with a skia canvas backend and cairo content. If we don't have a content device, let's not init dwrite fonts and use GDI instead.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp?from=gfxWindowsPlatform.cpp#2535
Attachment #8722750 - Flags: review?(dvander)
Attachment #8722750 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/883f0723ad50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8722750 [details] [diff] [review]
Don't init dwrite fonts if content device creation fails

Approval Request Comment
[Feature/regressing bug #]: Bug 842894, add support for dwrite fonts in skia backend
[User impact if declined]: User crashes in e10s, see crash stats - https://crash-stats.mozilla.com/report/list?signature=get_style
[Describe test coverage new/current, TreeHerder]: Manual 
[Risks and why]: Low - This brings us back to behavior in Gecko 45. It also properly does not initialize d3d if we can't get a device on content.
[String/UUID change made/needed]: None
Attachment #8722750 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1249600
Regression from 46. Is this only e10s related or does something else happen without e10s enabled?
Flags: needinfo?(mchang)
Keywords: regression
These crash signatures are still showing up on nightly builds from March 1. If I look at the last 14 days of crashes there isn't a spike or really a drop happening when this was checked into m-c. Did it work? Can you verify it? Thanks!
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> These crash signatures are still showing up on nightly builds from March 1.
> If I look at the last 14 days of crashes there isn't a spike or really a
> drop happening when this was checked into m-c. Did it work? Can you verify
> it? Thanks!

Looks like it's still happening, thanks for checking. See bug 1249600.
Flags: needinfo?(mchang)
Comment on attachment 8722750 [details] [diff] [review]
Don't init dwrite fonts if content device creation fails

Fix for graphics crash, looks like this will have more work coming to support it.
Attachment #8722750 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
For the risks here, "It also properly does not initialize d3d if we can't get a device on content."  I'm not sure how we might see that reflected if it's an issue. d3d crashes?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> For the risks here, "It also properly does not initialize d3d if we can't
> get a device on content."  I'm not sure how we might see that reflected if
> it's an issue. d3d crashes?

I'm not sure, we might get some weird configuration state where we might reference d3d even though it wasn't initialized properly. This is what happened to dwrite fonts and how we got the current crash.
I'm hitting a merge conflict applying this to aurora.
Flags: needinfo?(mchang)
Since branch day just happened, this patch is for beta. Do we have to request uplift again?
Flags: needinfo?(mchang) → needinfo?(wkocher)
Also, I just realized, do we ever plan to enable e10s on 46? If not, we don't have to uplift.
Flags: needinfo?(lhenry)
Yes, the plan is that e10s will be enabled for part of the beta population as an experiment -- for a couple of weeks with apz off, then with it on.
Flags: needinfo?(lhenry)
Let's aim to get this into beta 2.
Hi Mason, this never did land, can you request uplift to beta?
Flags: needinfo?(mchang)
Comment on attachment 8727583 [details] [diff] [review]
Don't init dwrite fonts if content device creation fails for beta

See comment 4. This also needs to be uplifted to aurora as well.
Flags: needinfo?(mchang)
Attachment #8727583 - Flags: approval-mozilla-beta?
(In reply to Mason Chang [:mchang] from comment #18)
> Comment on attachment 8727583 [details] [diff] [review]
> Don't init dwrite fonts if content device creation fails for beta
> 
> See comment 4. This also needs to be uplifted to aurora as well.

Err scratch that, this is already in aurora.
Comment on attachment 8727583 [details] [diff] [review]
Don't init dwrite fonts if content device creation fails for beta

Fix for e10s related crash. This should make it into beta 4 next week.
Attachment #8727583 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Related to bug 1260770?
Flags: needinfo?(mchang)
Can we actually do this these days? We didn't used to be able to switch font backends at runtime because of being unable to just erase the font cache upon a device reset, -that's- why we always stayed on DWrite fonts.
Flags: needinfo?(jfkthame)
This sounds....scary. If we're going to switch font back-ends on the fly, you'll need to recreate the platform's gfxPlatformFontList (as there are separate DWrite and GDI implementations, and they create distinct gfxFontEntry types, etc); and if there are existing fonts in the various caches, holding on to existing (backend-specific) gfxDWriteFont objects, but the DWrite factory goes away because we've decided to switch to GDI fonts, I suspect all kinds of bad things will happen.

"Fallback to GDI fonts if Device Creation fails" sounds like a fine thing to do *during startup* when the gfxPlatform is being initialized; but if this happens mid-run due to a device reset, I have no confidence at all in the outcome. AFAIK, all the font code assumes use of a consistent backend.
Flags: needinfo?(jfkthame)
Yeah, rock and a hard place.  This is out of our hands - device reset happens, we can't get the same backend, we need to figure something out.  Is there a way to clean/reinitialize all the font stuff?
Flags: needinfo?(jfkthame)
(In reply to Milan Sreckovic [:milan] from comment #25)
> Yeah, rock and a hard place.  This is out of our hands - device reset
> happens, we can't get the same backend, we need to figure something out.  Is
> there a way to clean/reinitialize all the font stuff?

We can, our backends explicitly were meant to all support DWrite fonts for exactly this reason. I'm not sure I understand why we can't continue to do this. i.e. I don't know what problem this bug is attempting to solve.
Flags: needinfo?(milan)
(In reply to Jonathan Kew (:jfkthame) from comment #24)
> "Fallback to GDI fonts if Device Creation fails" sounds like a fine thing to
> do *during startup* when the gfxPlatform is being initialized; but if this
> happens mid-run due to a device reset, I have no confidence at all in the
> outcome. AFAIK, all the font code assumes use of a consistent backend.

This patch was only meant to address this case. If device creation fails on gfxPlatform startup, we fallback to GDI fonts. If we can't create a device, we don't init d2d [1]. This patch just fixed a corner case of that general notion.

If we get a TDR, and device creation fails, but we initialized dwrite fonts on gfxPlatform startup, I don't think we actively swap font hosts. I think we still use DWrite fonts don't we? Same with the inverse, if we failed to init a device, initialized with GDI fonts, get a TDR and now can use DWrite fonts we, don't we still use GDI Fonts?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#2302
(In reply to Milan Sreckovic [:milan] from comment #22)
> Related to bug 1260770?

Possibly? I see a lot of this in the crash reports:[1]

"D2D1.1? D2D1.1+ D2D? D2D1.1V? D2D1.1V+ D2D+ D3D11 Layers? D3D11 Layers+ D2D1.1- D2D- D3D11 Layers- "

Does D2D- mean we don't have D2D support? I saw some other reports showing device content creation failed.

[1] https://crash-stats.mozilla.com/report/index/8c1b6064-9548-4eac-90d8-2632a2160412
Flags: needinfo?(mchang)
(In reply to Mason Chang [:mchang] from comment #27)
> (In reply to Jonathan Kew (:jfkthame) from comment #24)
> > "Fallback to GDI fonts if Device Creation fails" sounds like a fine thing to
> > do *during startup* when the gfxPlatform is being initialized; but if this
> > happens mid-run due to a device reset, I have no confidence at all in the
> > outcome. AFAIK, all the font code assumes use of a consistent backend.
> 
> This patch was only meant to address this case. If device creation fails on
> gfxPlatform startup, we fallback to GDI fonts. If we can't create a device,
> we don't init d2d [1]. This patch just fixed a corner case of that general
> notion.
> 
> If we get a TDR, and device creation fails, but we initialized dwrite fonts
> on gfxPlatform startup, I don't think we actively swap font hosts. I think
> we still use DWrite fonts don't we? Same with the inverse, if we failed to
> init a device, initialized with GDI fonts, get a TDR and now can use DWrite
> fonts we, don't we still use GDI Fonts?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.
> cpp#2302

If we don't have D2D on startup we should never start using it after a TDR. If we do, that's a bug.

I think this is where I messed something up: https://hg.mozilla.org/releases/mozilla-beta/rev/3dd65d3b0732
(In reply to Bas Schouten (:bas.schouten) from comment #29)
> I think this is where I messed something up:
> https://hg.mozilla.org/releases/mozilla-beta/rev/3dd65d3b0732

Yeah, that would be problematic if it happens on the fly, after we've already got live DW fonts. And it looks like we can get there from HandleDeviceReset, so.... sadness.
Flags: needinfo?(jfkthame)
Flags: needinfo?(milan)
You need to log in before you can comment on or make changes to this bug.