Closed Bug 1477471 Opened Last year Closed Last year

Crash in mozilla::gfx::DrawTarget::PushLayerWithBlend

Categories

(Core :: Graphics: WebRender, defect, critical)

x86_64
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: darkspirit, 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 

=============================================================
This looks like it might be because DrawTargetTiled doesn't implement PushLayerWithBlend
I ran into this yesterday by inspection and am surprised we didn't hit it sooner. Should be super easy to fix.
Assignee: nobody → bugmail
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 on attachment 8994575 [details]
Bug 1477471 - Add missing pop_back call.

https://reviewboard.mozilla.org/r/259114/#review266144
Attachment #8994575 - Flags: review?(jmuizelaar) → review+
Does DrawTargetDual support it? If we have a tiled component alpha layer, is the DrawTargetDual outside the DrawTargetTiled or inside of it?
(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.
Comment on attachment 8994591 [details]
Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled.

Argh MozReview. Carrying r+ from previous iteration.
Attachment #8994591 - Flags: review?(jmuizelaar) → review+
Attachment #8994592 - Flags: review?(jmuizelaar) → review+
(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 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 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)
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
(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.
You need to log in before you can comment on or make changes to this bug.