Closed Bug 1208638 Opened 4 years ago Closed 3 years ago

Don't use D2D if CompositorD3D11::Initialize fails

Categories

(Core :: Graphics, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

If CompositorD3D11::Initialize fails, the content process will try to use D2D1, which is not great. We don't support or test this configuration.
Attached patch fixSplinter Review
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().
Attachment #8666520 - Flags: review?(jmuizelaar)
Attachment #8666520 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/17da3f535e29
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(dvander)
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(dvander)
Resolution: FIXED → ---
(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
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Flags: needinfo?(dvander)
Resolution: --- → FIXED
(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.
Flags: needinfo?(dvander)
(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)
Flags: needinfo?(dvander)
You need to log in before you can comment on or make changes to this bug.