1429508 - thread 'WRRenderBackend#1' panicked at 'Vector image error Unknown error', /builds/worker/workspace/build/src/gfx/webrender/src/resource_cache.rs:823:28

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev d5f42a23909e.

thread 'WRRenderBackend#1' panicked at 'Vector image error Unknown error', /builds/worker/workspace/build/src/gfx/webrender/src/resource_cache.rs:823:28
stack backtrace:
[Parent 10110, Main Thread] WARNING: attempt to modify an immutable nsStandardURL: file /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp, line 1698
[Parent 10110, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp, line 1009
   0:     0x7f3adaa470f3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::h8ed7485deb8ab958
   1:     0x7f3adaa41840 - std::sys_common::backtrace::_print::h3d4f9ea58578e60f
   2:     0x7f3adaa545f3 - std::panicking::default_hook::{{closure}}::h0088fe51b67c687c
   3:     0x7f3adaa54362 - std::panicking::default_hook::hf425c768c5ffbbad
   4:     0x7f3adaa54af6 - std::panicking::rust_panic_with_hook::h25b934bb4484e9e0
   5:     0x7f3adaa54984 - std::panicking::begin_panic::h59483e27e93d7bc6
   6:     0x7f3adaa54889 - std::panicking::begin_panic_fmt::h5f221297e8a3dbdb
   7:     0x7f3ada7f954f - webrender::resource_cache::ResourceCache::block_until_all_resources_added::h6251023497fa883e
   8:     0x7f3ada8b50d7 - webrender::frame_builder::FrameBuilder::build::h14092b62004c54e0
   9:     0x7f3ada849d3b - webrender::render_backend::Document::render::h7823269006843dee
  10:     0x7f3ada84d53a - webrender::render_backend::RenderBackend::run::h72b63eaaa65524e3
  11:     0x7f3ada7b7b51 - std::sys_common::backtrace::__rust_begin_short_backtrace::h5e11173f8d4d2ef9
  12:     0x7f3ada7b887d - std::panicking::try::do_call::h5b4eb2dbf08e40a1
  13:     0x7f3adaa593cb - __rust_maybe_catch_panic
Redirecting call to abort() to mozalloc_abort

OS|Linux|0.0.0 Linux 4.4.0-104-generic #127-Ubuntu SMP Mon Dec 11 12:16:42 UTC 2017 x86_64
CPU|amd64|family 6 model 78 stepping 3|1
GPU|||
Crash|SIGSEGV|0x0|49
49|0|firefox|mozalloc_abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:d5f42a23909e|33|0x5
49|1|firefox|abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:d5f42a23909e|80|0x5
49|2|libxul.so|panic_abort::__rust_start_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|59|0x5
49|3|libxul.so|std::panicking::rust_panic|git:github.com/rust-lang/rust:src/libstd/panicking.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|608|0x9
49|4|libxul.so|std::panicking::rust_panic_with_hook|git:github.com/rust-lang/rust:src/libstd/panicking.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|593|0xd
49|5|libxul.so|std::panicking::begin_panic<alloc::string::String>|git:github.com/rust-lang/rust:src/libstd/panicking.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|538|0x12
49|6|libxul.so|std::panicking::begin_panic_fmt|git:github.com/rust-lang/rust:src/libstd/panicking.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|522|0x1c
49|7|libxul.so|webrender::resource_cache::ResourceCache::block_until_all_resources_added|hg:hg.mozilla.org/mozilla-central:gfx/webrender/src/resource_cache.rs:d5f42a23909e|823|0x13
49|8|libxul.so|webrender::frame_builder::FrameBuilder::build|hg:hg.mozilla.org/mozilla-central:gfx/webrender/src/frame_builder.rs:d5f42a23909e|1724|0xf
49|9|libxul.so|webrender::render_backend::Document::render|hg:hg.mozilla.org/mozilla-central:gfx/webrender/src/frame.rs:d5f42a23909e|1087|0x23
49|10|libxul.so|webrender::render_backend::RenderBackend::run|hg:hg.mozilla.org/mozilla-central:gfx/webrender/src/render_backend.rs:d5f42a23909e|446|0x17
49|11|libxul.so|std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()>|hg:hg.mozilla.org/mozilla-central:gfx/webrender/src/renderer.rs:d5f42a23909e|2221|0x26
49|12|libxul.so|std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>|git:github.com/rust-lang/rust:src/libstd/thread/mod.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|400|0x18
49|13|libxul.so|panic_abort::__rust_maybe_catch_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|38|0x5
49|14|libxul.so|alloc::boxed::{{impl}}::call_box<(),closure>|git:github.com/rust-lang/rust:src/libstd/panicking.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|459|0x8
49|15|libxul.so|std::sys::imp::thread::{{impl}}::new::thread_start|git:github.com/rust-lang/rust:src/liballoc/boxed.rs:05e2e1c41414e8fc73d0f267ea8dab1a3eeeaa99|736|0x3
49|16|libpthread-2.23.so||||0x76ba
49|17|libc-2.23.so||||0x1073dd

Comment 1

[Daniel Holbert [:dholbert]]

(pretty sure this belongs in the webrender component, at first glance)

Comment 2

[Jamie Nicol [:jnicol]]

The call to CreateSimilarDrawTaraget in RecordedCreateSimilarDrawTarget::PlayEvent is failing, which leads to this panic.

Comment 3

[Jamie Nicol [:jnicol]]

CreateSimilarDrawTarget fails because the requested size is 181x182621, which is greater than the max surface size of 32767. This will always fail.

The page has a very tall nsTextControlFrame. In WebRenderCommandBuilder::GenerateFallbackData, there is a comment explaining why this is not clipped, saying we can rely on webrender to do so.

Indeed on the webrender side the blob image is a reasonable size (181x1024), and the base draw target when replaying is set to that size rather than the recorded size. However the error comes from creating the draw target for the CreateSimilarDrawTarget event, not the base draw target. This uses the recorded size rather than webrender's clipped size. This event originates from GenerateAndPushTextMask(), called from nsDisplayBackgroundColor::Paint().

Comment 4

[Jamie Nicol [:jnicol]]

Jeff, I've been thinking about this and I don't think there's any way we can clip the size of the CreateSimilarDrawTarget draw target when replaying. Certainly not in a general way. Do we therefore need to do some clipping in WebRenderCommandBuilder::GenerateFallbackData?

Comment 5

[Jamie Nicol [:jnicol]]

There are some WIP patches here, following advice from Jeff on irc to record at a higher level. i.e. that the draw target will be used for a mask, so we don't need to record the size.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2b2c0dcfaacacb6cba0854bce523f96c9552124

Recording without a size seems like a good approach. It was still panicking when replaying if you scrolled down the page - it would create the draw target from 0,0 rather than the same offset as the blob image's DT. I used a trick with DrawTargetTiled to get that to work. Then it was panicking because in DrawTargetSkia::PopLayer it needs to copy the mask DrawTargetTiled/SnapshotTiled to a non-tiled DataSourceSurface. There is an ugly and specific hack to make that work, but it needs generalised - possibly either by iterating the tiles or copying out a subregion.

The size of the mask draw target might not always be intended to be exactly the size of the base draw target, and we don't want to increase the size for the non-recording case. It might be possible to take the desired size and transform and be a bit cleverer.

I probably also need to figure out how to write a good test case to make sure I'm still rendering correctly.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

We should be able to create the actual mask surface based on the clipped size when we're playing back.

I think it might also be useful to also lift up the application of the mask if we need to make assurances about it's size and transform.

Comment 7

[Jamie Nicol [:jnicol]]

Attachment: Bug 1429508 - Mark DrawTarget::GetSize as const.

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

Comment 8

[Jamie Nicol [:jnicol]]

Attachment: Bug 1429508 - Make SnapshotTiled::GetDataSurface return a surface the size of backed tiles only.

Fix the DrawTargetTiled::mRect initialization, and expose through the
virtual function GetRect(). Make SnapshotTiled::GetDataSurface()
allocate a surface the size of this rect, rather the XMost and
YMost. Callers of SourceSurface::GetDataSurface() can query
SourceSurface::GetRect() and apply the necessary offset themselves.

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

Comment 9

[Jamie Nicol [:jnicol]]

Attachment: Bug 1429508 - Allow created mask surfaces to be clipped to the necessary size when replaying a recording.

Add a command CreateClippedDrawTarget to DrawTarget, which takes the
max required size and a transform between this draw target and the one
to be created. The created draw target may have its size clipped to
the size of this draw target, transformed to the new target's
space. This means that the new surface will be large enough so
that it is rendered to this draw target correctly, but not necessarily
any larger.

Usually this will just create a draw target of the requested size, for
simplicity. However, when replaying a recorded draw target we do clip
the size to the base draw target's size. This is done using a
DrawTargetTiled, so when applying the mask in PopLayer, we must take
the SourceSurface's offset in to account.

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

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8948648 [details]
Bug 1429508 - Mark DrawTarget::GetSize as const.

https://reviewboard.mozilla.org/r/218058/#review224320

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8948649 [details]
Bug 1429508 - Make SnapshotTiled::GetDataSurface return a surface the size of backed tiles only.

These seem reasonable. I'm going to punt the review to Bas though.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 8948649 [details]
Bug 1429508 - Make SnapshotTiled::GetDataSurface return a surface the size of backed tiles only.

https://reviewboard.mozilla.org/r/218060/#review224546

This makes me cringe a little, but at the same time, I have no better ideas.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 8948650 [details]
Bug 1429508 - Allow created mask surfaces to be clipped to the necessary size when replaying a recording.

https://reviewboard.mozilla.org/r/218062/#review225014

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad06ada05e45
Mark DrawTarget::GetSize as const. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/3ca1c8feda04
Make SnapshotTiled::GetDataSurface return a surface the size of backed tiles only. r=bas
https://hg.mozilla.org/integration/autoland/rev/9f354b89b323
Allow created mask surfaces to be clipped to the necessary size when replaying a recording. r=bas

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ad06ada05e45
https://hg.mozilla.org/mozilla-central/rev/3ca1c8feda04
https://hg.mozilla.org/mozilla-central/rev/9f354b89b323

Comment 16

[Intermittent Failures Robot]

3 failures in 702 pushes (0.004 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 2
* autoland: 1

Platform breakdown:
* linux64-qr: 2
* windows10-64-qr: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1429508&startday=2018-02-05&endday=2018-02-11&tree=all