1515932 - Crash in draw_tile_frame

Status: RESOLVED FIXED

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

Description

[Darkspirit]

(get_logan from bug 1507705 comment 5)
> Still crashes on https://www.rondomark.jp/
> 
> Firefox nightly 66.0a1 (2018-12-20) (64-bit), Build ID 20181220215605. Windows 10 1809 64-bit.

(get_logan from bug 1502717 comment 27)
> Same crash signature as resolved duplicate bug 1507705.
> bp-c681d5be-233d-45e7-941c-351040181221

This bug was filed from the Socorro interface and is
report bp-c681d5be-233d-45e7-941c-351040181221.
> assertion failed: dimensions.height >= self.max_dynamic_size.height
=============================================================

Top 10 frames of crashing thread:

0 xul.dll MOZ_CrashOOL mfbt/Assertions.h:314
1 xul.dll GeckoCrashOOL toolkit/xre/nsAppRunner.cpp:5118
2 xul.dll static void gkrust_shared::panic_hook toolkit/library/rust/shared/lib.rs:232
3 xul.dll static void core::ops::function::Fn::call<fn z:/libcore/ops/function.rs:78
4 xul.dll static void std::panicking::rust_panic_with_hook src/libstd/panicking.rs:480
5 xul.dll static <NoType> std::panicking::begin_panic<str*> z:/libstd/panicking.rs:410
6 xul.dll static void webrender::renderer::Renderer::draw_tile_frame gfx/wr/webrender/src/renderer.rs
7  @0x1e2666de7ff 
8 xul.dll static union core::result::Result<webrender::renderer::RendererStats, alloc::vec::Vec<webrender::renderer::RendererError>> webrender::renderer::Renderer::render_impl gfx/wr/webrender/src/renderer.rs:2567
9 xul.dll bool webrender_bindings::bindings::wr_renderer_render gfx/webrender_bindings/src/bindings.rs:617

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

Comment 1

[Darkspirit]

> (get_logan from bug 1507705 comment 5)
> > Still crashes on https://www.rondomark.jp/

I got this one on Linux: 
bp-17ad5ec6-16f3-4449-ac82-bd4a00181221 [@ webrender::render_task::RenderTask::new_picture ]
> explicit panic

Comment 2

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

Crash reports were reprocessed, so the signature for the report in comment 0 [1] is now draw_tile_frame. And there have been no crashes with that signature since the 20181221215622 buildid so I'm suspecting this is fixed.

Let's file new bugs for crashes with other signatures, and socorro bugs for updating skiplists etc for any other GeckoCrashOOL-type signatures.

[1] https://crash-stats.mozilla.com/report/index/c681d5be-233d-45e7-941c-351040181221

Comment 3

[Walter Labonte]

I got this issue today.

[Reproduce Steps]

1. Go to http://hitoki.info/
2. Then Firefox 65.0b10 will be crashed sometimes.

Comment 4

[Walter Labonte]

[Reproduce Steps]

1. Restart Firefox 65.0b10
2. Go to http://hitoki.info/
3. Then Firefox 65.0b10 will be crashed sometimes.

Comment 5

[Mayank Bansal]

confirmed crash on http://hitoki.info/
https://crash-stats.mozilla.org/report/index/e76acc64-717e-4e82-a1df-d45310190114#tab-bugzilla

@darkspirit: Would a new bug be better to track this, or should this one be reopened?

Comment 6

[Darkspirit]

(In reply to Mayank Bansal from comment #5)

confirmed crash on http://hitoki.info/
https://crash-stats.mozilla.org/report/index/e76acc64-717e-4e82-a1df-d45310190114#tab-bugzilla

@darkspirit: Would a new bug be better to track this, or should this one be reopened?

Debian Testing, KDE, X11, Macbook Pro

This crash is reproducible with build 2018-12-25 and latest m-c (b8f96f4d584a), so we should reopen this bug.
On 2018-10-01 it had the crash reason I saw in comment 1:
bug 1505934 will likely fix this issue as its try build doesn't crash (https://phabricator.services.mozilla.com/D16335), so let's mark as dupe.(?)

Comment 7

[Darkspirit]

Sorry, it looks like I was wrong in comment 6. :/
Two new occurrences with GTX 670 on Win10 even after bug 1505934 landed.

bp-bbd4963a-1da2-41f7-b289-c78aa0190118
assertion failed: dimensions.height >= self.max_dynamic_size.height
|[G0][GFX1-]: Attempting to allocate a texture of size 1280x9984 above the limit, trimming (t=66.8752)

I was able to reproduce once by loading http://hitoki.info/ multiple times: bp-4db7487c-7dd2-4771-b20b-3e0b60190119 (Linux)

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #7)

Sorry, it looks like I was wrong in comment 6. :/
Two new occurrences with GTX 670 on Win10 even after bug 1505934 landed.

bp-bbd4963a-1da2-41f7-b289-c78aa0190118
assertion failed: dimensions.height >= self.max_dynamic_size.height
|[G0][GFX1-]: Attempting to allocate a texture of size 1280x9984 above the limit, trimming (t=66.8752)

I was able to reproduce once by loading http://hitoki.info/ multiple times: bp-4db7487c-7dd2-4771-b20b-3e0b60190119 (Linux)

I also confirmed to reproduce the crash on nightly.

Comment 9

[Nicolas Silva [:nical]]

I'm looking at this on the side. Sotaro I don't know whether you have started investigating, if you want don't hesitate to reassign the bug.

Comment 10

[Sotaro Ikeda [:sotaro]]

No problem, I do not start working for it.

Comment 11

[Sotaro Ikeda [:sotaro]]

I saw two types of crashes.

Device::create_texture() : crashed on ubuntu

• https://searchfox.org/mozilla-central/rev/8b3be80f4217d8fc58078808ed981c44677db40a/gfx/webrender/src/device/gl.rs#1199

RenderTargetList<T>::check_ready() : crashed on Win10

• https://searchfox.org/mozilla-central/rev/d6e0b12f8d04791c4542824d8a9ee0b48279fbb6/gfx/webrender/src/tiling.rs#310

Comment 12

[Sotaro Ikeda [:sotaro]]

I also saw the crash at render_task_sanity_check() in RenderTask::new_picture().
https://searchfox.org/mozilla-central/rev/3f1b0144a3a558959e028fd23c03fe5b37f7f812/gfx/webrender/src/render_task.rs#356

The crash happened easily when display size was large. In my case, size of P50(Win10) was 3840*2160.

Large task was added during handling PictureCompositeMode::Filter in PicturePrimitive::prepare_for_render(). It uses clipped.size as task's dynamic_size.
https://searchfox.org/mozilla-central/rev/3f9c2dda5c694cc3ca0dc517d54f8087abe9bc57/gfx/webrender/src/picture.rs#906

Comment 13

[Sotaro Ikeda [:sotaro]]

max_texture_size was shrunk to 8192 by Bug 1454187. It made easier to hit the crash.

Comment 14

[Matt Woodrow (:mattwoodrow)]

So it looks like we get to allocate_target_texture and compute a value for 'dimensions' that larger than the max texture size.

We call device.create_texture, which clamps the size of the resulting texture to the max texture size.

Then we call list.check_ready, where the dimensions of the texture are now smaller than the max_dynamic_size, and it asserts.

Should we be trying to work around the assertion, and making sure that things just work (quality will be lower, since it'll downscale), or should we try to find a way to clamp the size earlier in the pipeline?

Comment 15

[Sotaro Ikeda [:sotaro]]

:gw, can you advice to comment 14?

Comment 16

[Glenn Watson [:gw]]

I think adding support for drawing the target at a smaller scale is probably the right fix for this issue (at least in terms of shipping the MVP). Emilio is looking into this now to see what's involved.

Comment 17

[Jessie [:jbonisteel] pls NI]

Just assigning this to Emilio for now to make it more clear who is taking a look at this for now

Comment 18

[Dzmitry Malyshau [:kvark]]

Starting looking at this now. The plan is to move the decision about local/screen space rasterization from pre_update to post_update, where the bounds are known.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

So, issues I'm finding (of course when reducing the max surface size to something small like 124px or so so that this code kicks in very often) are:

• mix-blend-mode. I haven't been able to get mix-blend-mode to work properly with a downscaled picture, basically because all that code assumes stuff about the picture task size, and we'd need to propagate that information around.

• All our UV computation works in terms of mapping to raster root coordinates, which means that we cannot downscale individually pictures, we can only downscale all the pictures on a raster root at once. This is a bit of a pity, but maybe it's ok? That means that we don't honor all the RasterSpace::Local requests though. Unless we force-create a raster-root for them, but that means we'd probably get clipping wrong in a bunch of cases (because the clip collector stuff is (maybe was? not sure) not sound unless the stacking context is not a fixed pos containing block in CSS).

Dzmitry, does this sound right? Looking at the crashers at bug 1505934 involve perspective, so I think it should be fine, but just want to make sure that downscaling only the contents of raster roots is ok / the way to go.

Comment 20

[Dzmitry Malyshau [:kvark]]

I would expect us to be able to downscale picture per surface, which is finer grain than "per raster root".

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP patch for local scaling

Note that this is not cleaned up in any way, and I'm pretty sure a bunch of stuff is wrong (the size scaling in get_raster_rects for starters). I've had a bunch of versions more or less closely to be green, this is just the state of my patches in my tree right now. Uploading because Glenn is going to take a stab at it, in case of something from there seems cherry-pickable.

Comment 22

[Dzmitry Malyshau [:kvark]]

Attachment: WR rasterization root fall-back

This is a follow-up to https://phabricator.services.mozilla.com/D16560

Previously, we had a conservative estimation of the local size based on the footprint
of the screen onto the potential raster root. This was too conservative in general,
and in some cases it wasn't conservative enough, since with filters we can have areas
needed in local space that don't necessarily project on the screen.

This change is doing an exact check for the surface size after we compute it, and
falls back to the parent raster root accordingly.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b3c012e005e
WR rasterization root fall-back r=gw

Comment 24

[Dzmitry Malyshau [:kvark]]

Attachment: WR fix establish_raster_root boolean on a picture

This is a follow-up to https://phabricator.services.mozilla.com/D18258
which updates establish_raster_root boolean. It affects the perspective interpolation
of brushes.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c01ab8f12024
WR fix establish_raster_root boolean on a picture r=gw

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3b3c012e005e

Comment 27

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/c01ab8f12024

Comment 28

[Walter Labonte]

I am a comment 4. It looks fine after comment 27. I have confirmed it's O.K. on Nightly build id 20190202094451. Thanks for fixing!