Closed
Bug 1258768
Opened 8 years ago
Closed 8 years ago
Fix unsafe casts in the different TextureHost::SetCompositor implementations
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: nical, Assigned: nical)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(3 files, 1 obsolete file)
22.69 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
5.61 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
22.01 KB,
patch
|
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
I think we end up performing a bad cast form BasicCOmpositor* -> CompositorOGL* on Mac somewhere in the bc7 tests.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nical.bugzilla
Comment 1•8 years ago
|
||
I think you landed the patches for this bug in bug 1245813?
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 2•8 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)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8738521 -
Flags: review?(dvander)
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•8 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•8 years ago
|
||
Attachment #8740037 -
Flags: review?(dvander)
Assignee | ||
Comment 7•8 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26b792fc082 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2fc553b1327
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a26b792fc082 https://hg.mozilla.org/mozilla-central/rev/a2fc553b1327
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 11•8 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?
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
OK, thanks. Tracking for 47+ then.
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
Updated•8 years ago
|
Attachment #8743205 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Assignee | ||
Comment 16•8 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+
Comment 18•8 years ago
|
||
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•8 years ago
|
||
There we go: https://hg.mozilla.org/releases/mozilla-aurora/rev/f328ac32af6a
Flags: needinfo?(nical.bugzilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•