Fix unsafe casts in the different TextureHost::SetCompositor implementations

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46+ wontfix, firefox47+ fixed, firefox48+ fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

3 years ago
I think we end up performing a bad cast form BasicCOmpositor* -> CompositorOGL* on Mac somewhere in the bc7 tests.
Assignee

Updated

3 years ago
Whiteboard: [gfx-noted]
Assignee

Updated

3 years ago
Assignee: nobody → nical.bugzilla
I think you landed the patches for this bug in bug 1245813?
Flags: needinfo?(nical.bugzilla)
Assignee

Comment 2

3 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #1)
> I think you landed the patches for this bug in bug 1245813?

Yes, I took care of most of the unsafe casts in bug 1245813, but I forgot a few of them so I'll handle those here.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8738521 [details] [diff] [review]
check before performing the remaining unsafe casts in SetCompositor

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

Sorry for the late review - Could we add something to Compositor.h like:

  virtual CompositorOGL* AsCompositorOGL() { return nullptr; }
  ...

etc, for each compositor type? Then these asserts become a lot easier to write, and we can safely null crash and get rid of static casts.
Attachment #8738521 - Flags: review?(dvander)
Assignee

Comment 5

3 years ago
Same patch with the AsCompositorFoo() down-casting pattern that we use in other places.

I noticed that there are some inconsistencies in whether the texture should keep the previous compositor when the check fails, and I didn't attempt to fix this in this patch, although I think it'd be good to have all texture implementations on the same page regarding that, to avoid surprises.

There are also a few static_casts left (outside of the SetCompositor code), I'll submit a followup patch in this bug.
Attachment #8738521 - Attachment is obsolete: true
Attachment #8740015 - Flags: review?(dvander)
Assignee

Comment 6

3 years ago
Attachment #8740037 - Flags: review?(dvander)
Assignee

Comment 7

3 years ago
Comment on attachment 8740015 [details] [diff] [review]
Use virtual downcasting methods

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

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +637,5 @@
>    gfxWindowsPlatform::GetPlatform()->GetD3D11Device(&device);
>    return device;
>  }
>  
> +static CompositorD3D11* AssertD3D9Compositor(Compositor* aCompositor)

There's a typo here (D3D9 instead of D3D11), fixed locally.
Attachment #8740015 - Flags: review?(dvander) → review+
Comment on attachment 8740037 [details] [diff] [review]
The remaining unsafecompositor casts.

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

Thanks! Very glad to see all these go.
Attachment #8740037 - Flags: review?(dvander) → review+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a26b792fc082
https://hg.mozilla.org/mozilla-central/rev/a2fc553b1327
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1265282
Assignee

Comment 11

3 years ago
first patch rebased on top of beta. 

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Incorrect unsafe casts & crashes during device resets.
[Describe test coverage new/current, TreeHerder]: has been on nightly for a little while.
[Risks and why]: rather low, most of the risk is me making a typo while rebasing the patch, a try run will be enough to check that. The risk of not taking the patch is more worrying.
[String/UUID change made/needed]: None.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0e749325d7c
Attachment #8743205 - Flags: approval-mozilla-beta?
Attachment #8743205 - Flags: approval-mozilla-aurora?
Nical, this will have to land on m-r as soon as possible to make it into 46. How bad is this crashing problem? When did it start/regress?
Flags: needinfo?(nical.bugzilla)
Milan, can you help me out here? Do we absolutely need this on 46?
Flags: needinfo?(milan)
Let's skip 46, just do 47.
Flags: needinfo?(milan)
OK, thanks. Tracking for 47+ then.
Attachment #8743205 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Assignee

Comment 16

3 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Nical, this will have to land on m-r as soon as possible to make it into 46.
> How bad is this crashing problem? When did it start/regress?

m-r as in release? I am a bit confused about the timing here, I thought there was some time left. I definitely would not land to release directly.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8743205 [details] [diff] [review]
(beta) check before performing the remaining unsafe casts in SetCompositor

Prevents crashes during device resets, Aurora47+
Attachment #8743205 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is going to need rebasing for Aurora too. Can you please taking of fixing and pushing this, Nicolas?
Flags: needinfo?(nical.bugzilla)
Assignee

Comment 19

3 years ago
There we go: https://hg.mozilla.org/releases/mozilla-aurora/rev/f328ac32af6a
Flags: needinfo?(nical.bugzilla)
Depends on: 1267972
You need to log in before you can comment on or make changes to this bug.