1477471 - Crash in mozilla::gfx::DrawTarget::PushLayerWithBlend

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Darkspirit]

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 

=============================================================

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This looks like it might be because DrawTargetTiled doesn't implement PushLayerWithBlend

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I ran into this yesterday by inspection and am surprised we didn't hit it sooner. Should be super easy to fix.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled.

Review commit: https://reviewboard.mozilla.org/r/259112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259112/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1477471 - Add missing pop_back call.

Review commit: https://reviewboard.mozilla.org/r/259114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259114/

Comment 5

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8994575 [details]
Bug 1477471 - Add missing pop_back call.

https://reviewboard.mozilla.org/r/259114/#review266144

Comment 7

[Markus Stange [:mstange]]

Does DrawTargetDual support it? If we have a tiled component alpha layer, is the DrawTargetDual outside the DrawTargetTiled or inside of it?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled.

Review commit: https://reviewboard.mozilla.org/r/259132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259132/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1477471 - Add missing pop_back call.

Review commit: https://reviewboard.mozilla.org/r/259134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259134/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8994591 [details]
Bug 1477471 - Implement PushLayerWithBlend for DrawTargetTiled.

Argh MozReview. Carrying r+ from previous iteration.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a01697589ad0
https://hg.mozilla.org/mozilla-central/rev/dc036d48bba7