Closed
Bug 1477471
Opened 6 years ago
Closed 6 years ago
Crash in mozilla::gfx::DrawTarget::PushLayerWithBlend
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | --- | fixed |
People
(Reporter: jan, Assigned: kats)
References
(Blocks 1 open bug)
Details
(Keywords: crash, nightly-community)
Crash Data
Attachments
(2 files, 2 obsolete files)
AMD, Hawaii PRO [Radeon R9 290/390] This bug was filed from the Socorro interface and is report bp-cbf26edd-f250-43aa-b021-bd2aa0180721. > MOZ_CRASH(GFX: PushLayerWithBlend) ============================================================= Top 10 frames of crashing thread: 0 xul.dll mozilla::gfx::DrawTarget::PushLayerWithBlend gfx/2d/2D.h:1279 1 xul.dll bool mozilla::gfx::RecordedEvent::DoWithEvent<MemReader, `lambda at z:/build/build/src/gfx/2d/InlineTranslator.cpp:78:32'> gfx/2d/RecordedEventImpl.h:3596 2 xul.dll mozilla::gfx::InlineTranslator::TranslateRecording gfx/2d/InlineTranslator.cpp:77 3 xul.dll wr_moz2d_render_cb gfx/webrender_bindings/Moz2DImageRenderer.cpp 4 xul.dll static void rayon_core::job::{{impl}}::execute<closure> third_party/rust/rayon-core/src/job.rs:156 5 xul.dll static void rayon_core::registry::WorkerThread::wait_until_cold<rayon_core::latch::CountLatch> third_party/rust/rayon-core/src/registry.rs:567 6 xul.dll static void std::sys_common::backtrace::__rust_begin_short_backtrace<closure, src/libstd/sys_common/backtrace.rs:137 7 xul.dll static void alloc::boxed::{{impl}}::call_box< src/liballoc/boxed.rs:640 8 xul.dll static void std::sys::windows::thread::{{impl}}::new::thread_start src/libstd/sys/windows/thread.rs:55 9 kernel32.dll BaseThreadInitThunk =============================================================
Assignee | ||
Comment 1•6 years ago
|
||
This looks like it might be because DrawTargetTiled doesn't implement PushLayerWithBlend
Comment 2•6 years ago
|
||
I ran into this yesterday by inspection and am surprised we didn't hit it sooner. Should be super easy to fix.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bugmail
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8994574 [details] Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled. https://reviewboard.mozilla.org/r/259112/#review266140 ::: gfx/2d/DrawTargetTiled.cpp:335 (Diff revision 1) > void > DrawTargetTiled::PushLayer(bool aOpaque, Float aOpacity, SourceSurface* aMask, > const Matrix& aMaskTransform, const IntRect& aBounds, > bool aCopyBackground) > { > + PushLayerWithBlend(aOpaque, aOpacity, aMask, aMaskTransform, aBounds, aCopyBackground, CompositionOp::OP_OVER); This will break using DrawTargetTiled with other backends that don't yet support PushLayerWithBlend. I don't think that's a problem (I think we only use DrawTargetTiled with DrawTargetSkia) and if it is I guess will just get crashes for them. If you're feeling more risk averse you could just duplicate the code.
Attachment #8994574 -
Flags: review?(jmuizelaar) → review+
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8994575 [details] Bug 1477471 - Add missing pop_back call. https://reviewboard.mozilla.org/r/259114/#review266144
Attachment #8994575 -
Flags: review?(jmuizelaar) → review+
Comment 7•6 years ago
|
||
Does DrawTargetDual support it? If we have a tiled component alpha layer, is the DrawTargetDual outside the DrawTargetTiled or inside of it?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8994574 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8994575 -
Attachment is obsolete: true
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5) > This will break using DrawTargetTiled with other backends that don't yet > support PushLayerWithBlend. I don't think that's a problem (I think we only > use DrawTargetTiled with DrawTargetSkia) and if it is I guess will just get > crashes for them. > > If you're feeling more risk averse you could just duplicate the code. Good point. I'm feeling risk averse, so I updated the patch. (In reply to Markus Stange [:mstange] from comment #7) > Does DrawTargetDual support it? It doesn't appear to. > If we have a tiled component alpha layer, is > the DrawTargetDual outside the DrawTargetTiled or inside of it? Do we create DrawTargetDual instances when rendering blobs? I don't see a codepath that would result in that happening.
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8994591 [details] Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled. Argh MozReview. Carrying r+ from previous iteration.
Attachment #8994591 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•6 years ago
|
Attachment #8994592 -
Flags: review?(jmuizelaar) → review+
Comment 12•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > > If we have a tiled component alpha layer, is > > the DrawTargetDual outside the DrawTargetTiled or inside of it? > > Do we create DrawTargetDual instances when rendering blobs? I don't see a > codepath that would result in that happening. Not for blobs. But we use DrawTargetTiled for tiling as well.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8994591 [details] Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled. https://reviewboard.mozilla.org/r/259132/#review266150 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/2d/DrawTargetTiled.cpp:360 (Diff revision 1) > + bool aCopyBackground, > + CompositionOp aOp) > +{ > + // XXX - not sure this is what we want or whether we want to continue drawing to a larger > + // intermediate surface, that would require tweaking the code in here a little though. > + for (size_t i = 0; i < mTiles.size(); i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] for (size_t i = 0; i < mTiles.size(); i++) { ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (auto & mTile : mTiles)
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8994592 [details] Bug 1477471 - Add missing pop_back call. https://reviewboard.mozilla.org/r/259134/#review266152 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: gfx/2d/DrawTargetTiled.cpp:360 (Diff revision 1) > bool aCopyBackground, > CompositionOp aOp) > { > // XXX - not sure this is what we want or whether we want to continue drawing to a larger > // intermediate surface, that would require tweaking the code in here a little though. > for (size_t i = 0; i < mTiles.size(); i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] for (size_t i = 0; i < mTiles.size(); i++) { ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (auto & mTile : mTiles)
Comment 15•6 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a01697589ad0 Implement PushLayerWithBlend for DrawTargetTiled. r=jrmuizel https://hg.mozilla.org/integration/mozilla-inbound/rev/dc036d48bba7 Add missing pop_back call. r=jrmuizel
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Code Review Bot [:reviewbot] from comment #14) > Warning: Use range-based for loop instead [clang-tidy: > modernize-loop-convert] > > for (size_t i = 0; i < mTiles.size(); i++) { > ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > (auto & mTile : mTiles) I ignored this for the sake of having properly duplicated code in the two functions, so that it's more obvious it's a straight copy.
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a01697589ad0 https://hg.mozilla.org/mozilla-central/rev/dc036d48bba7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
status-firefox61:
--- → unaffected
status-firefox62:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•