1208638 - Don't use D2D if CompositorD3D11::Initialize fails

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

If CompositorD3D11::Initialize fails, the content process will try to use D2D1, which is not great. We don't support or test this configuration.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

This patch removes GetContentBackend() and replaces it with GetContentBackendFor(), which takes in a compositor type. This way gfxWindowsPlatform can fallback to Cairo if we failed to actually create a D3D11 compositor.

There was one caller that didn't know the compositor type, so I kept the old function as GetDefaultContentBackend().

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/919a831786f1

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ba52bbc272 for Windows build failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=15006240&repo=mozilla-inbound

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17da3f535e29

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/17da3f535e29

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm going to claim this patch wasn't enough, unless I'm misunderstanding the intent.  Returning false in CompositorD3D11::Initialize, synced to this patch on central, I end up with basic compositor and D2D1.1. Is there a piece missing?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #6)
> I'm going to claim this patch wasn't enough, unless I'm misunderstanding the
> intent.  Returning false in CompositorD3D11::Initialize, synced to this
> patch on central, I end up with basic compositor and D2D1.1. Is there a
> piece missing?

I just looked into this. There are two methods this patch failed to address, which bypass the layers backend checks. They are here [1] and here [2].

But, this patch was more about addressing ImageLayers/PaintedLayers. It didn't really attempt to fix up any temporary surface drawing code in layout (like CreateSamplingRestrictedDrawable). For example it made a conscious choice to not fix "nsSVGPathGeometryElement", since there wasn't any clear way to give it the layers backend.

I'd say we should file a new bug to make sure [1] and [2] are addressed and callers of GetDefaultContentBackend() are audited.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#630
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#183

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Anderson [:dvander] from comment #7)
> ...
> I'd say we should file a new bug to make sure [1] and [2] are addressed and
> callers of GetDefaultContentBackend() are audited.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#630
> [2]
> https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#183

Sounds like a plan.  Is this different than bug 1268628?  Either way, can you mark bug 1268628 for this, or create another one?  Thanks.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #8)
> (In reply to David Anderson [:dvander] from comment #7)
> > ...
> > I'd say we should file a new bug to make sure [1] and [2] are addressed and
> > callers of GetDefaultContentBackend() are audited.
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#630
> > [2]
> > https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.h#183
> 
> Sounds like a plan.  Is this different than bug 1268628?  Either way, can
> you mark bug 1268628 for this, or create another one?  Thanks.

I would say bug 1268628 is more a reporting problem. We'd have to expose more information to about:support or just kill D2D for the entire session instead of just the affected layer tree. (But if we do that, there's really no reason to fix the other calls in comment #7)