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

I think you landed the patches for this bug in bug 1245813?

(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.

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.

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.

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.

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a26b792fc082 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fc553b1327

https://hg.mozilla.org/mozilla-central/rev/a26b792fc082 https://hg.mozilla.org/mozilla-central/rev/a2fc553b1327

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

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?

Milan, can you help me out here? Do we absolutely need this on 46?

Let's skip 46, just do 47.

OK, thanks. Tracking for 47+ then.

(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.

Comment on attachment 8743205 [details] [diff] [review] (beta) check before performing the remaining unsafe casts in SetCompositor Prevents crashes during device resets, Aurora47+

This is going to need rebasing for Aurora too. Can you please taking of fixing and pushing this, Nicolas?

There we go: https://hg.mozilla.org/releases/mozilla-aurora/rev/f328ac32af6a

Thanks :)