1647918 - Hit MOZ_CRASH(called `Option::unwrap()` on a `None` value) at gfx/wr/webrender/src/batch.rs:2845

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Tyson Smith [:tsmith]]

Hit MOZ_CRASH(called Option::unwrap() on a None value) at gfx/wr/webrender/src/batch.rs:2845

#0 0x7fd3d5acd424 in AnnotateMozCrashReason /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:42:19
#1 0x7fd3d5acd424 in MOZ_Crash /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:331:3
#2 0x7fd3d5acd424 in RustMozCrash /builds/worker/checkouts/gecko/mozglue/static/rust/wrappers.cpp:17:3
#3 0x7fd3d5acd3d4 in mozglue_static::panic_hook::h718309d1c883b225 /builds/worker/checkouts/gecko/mozglue/static/rust/lib.rs:89:8
#4 0x7fd3d5accccb in core::ops::function::Fn::call::hff608039b849de82 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libcore/ops/function.rs:72:4
#5 0x7fd3d6e6abd4 in std::panicking::rust_panic_with_hook::hb976084785e50594 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:474:16
#6 0x7fd3d6e6a6ea in rust_begin_unwind /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:378:4
#7 0x7fd3d6e927d0 in core::panicking::panic_fmt::h45f7d6868edb5678 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libcore/panicking.rs:85:13
#8 0x7fd3d6e9271c in core::panicking::panic::h0fd4184f909d9498 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libcore/panicking.rs:52:4
#9 0x7fd3d553128f in core::option::Option$LT$T$GT$::unwrap::h012db3e3dfac196e /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libcore/macros/mod.rs:10:8
#10 0x7fd3d553128f in webrender::batch::BatchBuilder::add_segmented_prim_to_batch::h56d8193a42feb80e /builds/worker/checkouts/gecko/gfx/wr/webrender/src/batch.rs:2845:40
#11 0x7fd3d552c64d in webrender::batch::BatchBuilder::add_prim_to_batch::h572be306f97ca01b /builds/worker/checkouts/gecko/gfx/wr/webrender/src/batch.rs:2262:20
#12 0x7fd3d552a75b in webrender::batch::BatchBuilder::add_pic_to_batch::h1e12a1f5ce727afa /builds/worker/checkouts/gecko/gfx/wr/webrender/src/batch.rs:766:16
#13 0x7fd3d552a75b in webrender::batch::BatchBuilder::add_prim_to_batch::h572be306f97ca01b /builds/worker/checkouts/gecko/gfx/wr/webrender/src/batch.rs:1900:24
#14 0x7fd3d561983b in webrender::batch::BatchBuilder::add_pic_to_batch::h1e12a1f5ce727afa /builds/worker/checkouts/gecko/gfx/wr/webrender/src/batch.rs:766:16
#15 0x7fd3d561983b in _$LT$webrender..render_target..ColorRenderTarget$u20$as$u20$webrender..render_target..RenderTarget$GT$::build::h7b360eee6e1d2be4 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_target.rs:398:20
#16 0x7fd3d55748fa in webrender::render_target::RenderTargetList$LT$T$GT$::build::hf6faf1f9766e8b7d /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_target.rs:212:12
#17 0x7fd3d55748fa in webrender::frame_builder::build_render_pass::h10e4f0a7787ee44b /builds/worker/checkouts/gecko/gfx/wr/webrender/src/frame_builder.rs:994:12
#18 0x7fd3d55748fa in webrender::frame_builder::FrameBuilder::build::h5e7aeaff558d8ec0 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/frame_builder.rs:634:16
#19 0x7fd3d55f6feb in webrender::render_backend::Document::build_frame::he2a5f0ee30bb8b64 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_backend.rs:615:24
#20 0x7fd3d560b176 in webrender::render_backend::RenderBackend::update_document::h37ecbb23c20bdb8b /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_backend.rs:1520:40
#21 0x7fd3d560789b in webrender::render_backend::RenderBackend::prepare_transactions::h2533c62e75064bc1 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_backend.rs:1357:31
#22 0x7fd3d560789b in webrender::render_backend::RenderBackend::process_api_msg::h55190795dc708db6 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_backend.rs:1300:16
#23 0x7fd3d55fb3b3 in webrender::render_backend::RenderBackend::run::h85617324568a2120 /builds/worker/checkouts/gecko/gfx/wr/webrender/src/render_backend.rs:930:20
#24 0x7fd3d5421055 in webrender::renderer::Renderer::new::_$u7b$$u7b$closure$u7d$$u7d$::hb637c31b3788e1bd /builds/worker/checkouts/gecko/gfx/wr/webrender/src/renderer.rs:2629:12
#25 0x7fd3d5421055 in std::sys_common::backtrace::__rust_begin_short_backtrace::h3abec178bdac604f /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/sys_common/backtrace.rs:130:4
#26 0x7fd3d543f9c2 in std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h04655ad4debe6b2a /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/thread/mod.rs:475:16
#27 0x7fd3d543f9c2 in _$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::hc3927586943dcb9c /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panic.rs:318:8
#28 0x7fd3d543f9c2 in std::panicking::try::do_call::he90dd169dbd31238 /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/panicking.rs:303:39
#29 0x7fd3d6e7bbf8 in __rust_maybe_catch_panic /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libpanic_abort/lib.rs:30:4

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/GVw-lzkv__wSYogByNP2Mg/index.html

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Looks like the clip task was completely clipped out, so it didn't have an address to give.... Is that something we should expect?

Comment 3

[Dzmitry Malyshau [:kvark]]

Attachment: Refactor the clip task assignment logic in WR

I'm not totally sure that this patch is right. On one hand, doing a check matches the
other arms, which call to add_segment_to_batch, which also do check.
One the other hand, we should be detecting the visibility earlier and assigning
PrimitiveVisibilityIndex::INVALID to it.

Looking at request_resources_for_prim,
it seems to be assigning INVALID only for tiled images that are not visible, without
affecting the others.

Looking at update_primitive_visibility, it seems to selectively INVALID in some branches
with continue but not the others. This smells as bad code to me. Perhaps, that's where
the issue really is.

Comment 4

[Dzmitry Malyshau [:kvark]]

Yes, I believe we should expect the task to exist at that point.
Continuing discussion in https://phabricator.services.mozilla.com/D82121 with Glenn

Comment 5

[Dzmitry Malyshau [:kvark]]

Found a weird spot in https://github.com/servo/webrender/pull/3289/files#diff-33603c013bcfbe19d5d30c93a810759aR2845
Refactoring this to be more explicit, and it looks like the "early out" should behave like the other early outs and return false.
Another weird place is here: we are in needs_mask branch yet we end up without assigning any clip tasks.

Comment 6

[Dzmitry Malyshau [:kvark]]

With some help of Pernosco, I think I have an idea of what's going on. One frame the clip_task_index is assigned properly (on an image with needs_mask = true). The other frame the clip moves in such a way that only a single segment is visible, as produced by the segment builder. With a single segment, we get SegmentInstanceIndex::UNUSED assigned by build_segments_if_needed, which causes update_clip_task to exit.

The old code didn't realize that it needs to stop working with the primitive, and so it tried to use the stale task index. The new code does realize that, and shouldn't be using the task index any more. I can probably spend a week reconstructing this use case in more detail, if we happen to hit this again.

Dump of findings

Primitive Instance:

kind=Image{data_handle={index=11, epoch=(18), uid.uid=37}, image_instance_index=(20), is_compositor_surface=false}, local_clip_rect={origin={x=12.0, y=45.0}, size={width=16.0, height=16.0}}, id=(31174), prepared_frame_id=(46), visibility_info=(15171), clip_chain_id=(24201)}], flags.bits=6, cache_scroll_root=None

Primitive Info:

$8 = webrender::prim_store::PrimitiveVisibility {
  clip_chain: webrender::clip::ClipChainInstance {
    clips_range: webrender::clip::ClipNodeRange {
      first: 6049,
      count: 1
    },
    local_clip_rect: euclid::rect::Rect<f32, webrender_api::units::LayoutPixel> {
      origin: euclid::point::Point2D<f32, webrender_api::units::LayoutPixel> {
        x: 12,
        y: 45,
        _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
      },
      size: euclid::size::Size2D<f32, webrender_api::units::LayoutPixel> {
        width: 16,
        height: 16,
        _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
      }
    },
    has_non_local_clips: false,
    needs_mask: true,
    pic_clip_rect: euclid::rect::Rect<f32, webrender_api::units::PicturePixel> {
      origin: euclid::point::Point2D<f32, webrender_api::units::PicturePixel> {
        x: 12,
        y: 45,
        _unit: core::marker::PhantomData<webrender_api::units::PicturePixel>
      },
      size: euclid::size::Size2D<f32, webrender_api::units::PicturePixel> {
        width: 16,
        height: 16,
        _unit: core::marker::PhantomData<webrender_api::units::PicturePixel>
      }
    },
    pic_spatial_node_index: webrender::spatial_tree::SpatialNodeIndex (
      1
    )
  },
  clipped_world_rect: euclid::rect::Rect<f32, webrender_api::units::WorldPixel> {
    origin: euclid::point::Point2D<f32, webrender_api::units::WorldPixel> {
      x: 12,
      y: 45,
      _unit: core::marker::PhantomData<webrender_api::units::WorldPixel>
    },
    size: euclid::size::Size2D<f32, webrender_api::units::WorldPixel> {
      width: 16,
      height: 16,
      _unit: core::marker::PhantomData<webrender_api::units::WorldPixel>
    }
  },
  clip_task_index: webrender::prim_store::ClipTaskIndex (
    6398
  ),
  flags: webrender::prim_store::PrimitiveVisibilityFlags {
    bits: 0
  },
  visibility_mask: webrender::prim_store::PrimitiveVisibilityMask {
    bits: 1
  },
  combined_local_clip_rect: euclid::rect::Rect<f32, webrender_api::units::LayoutPixel> {
    origin: euclid::point::Point2D<f32, webrender_api::units::LayoutPixel> {
      x: 12,
      y: 45,
      _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
    },
    size: euclid::size::Size2D<f32, webrender_api::units::LayoutPixel> {
      width: 16,
      height: 16,
      _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
    }
  }
}

Image Instance:

$6 = webrender::prim_store::image::ImageInstance {
  segment_instance_index: webrender::storage::Index<webrender::prim_store::SegmentedInstance> (
    4294967294,
    core::marker::PhantomData<webrender::prim_store::SegmentedInstance>
  ),
  tight_local_clip_rect: euclid::rect::Rect<f32, webrender_api::units::LayoutPixel> {
    origin: euclid::point::Point2D<f32, webrender_api::units::LayoutPixel> {
      x: 0,
      y: 0,
      _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
    },
    size: euclid::size::Size2D<f32, webrender_api::units::LayoutPixel> {
      width: 0,
      height: 0,
      _unit: core::marker::PhantomData<webrender_api::units::LayoutPixel>
    }
  },
  visible_tiles: alloc::vec::Vec<webrender::prim_store::image::VisibleImageTile> {
    buf: alloc::raw_vec::RawVec<webrender::prim_store::image::VisibleImageTile, alloc::alloc::Global> {
      ptr: core::ptr::unique::Unique<webrender::prim_store::image::VisibleImageTile> {
        pointer: 0x4,
        _marker: core::marker::PhantomData<webrender::prim_store::image::VisibleImageTile>
      },
      cap: 0,
      a: alloc::alloc::Global
    },
    len: 0
  }
}

Comment 7

[Dzmitry Malyshau [:kvark]]

Tyson, would you happen to have an HTML that triggers that crash?

Comment 8

[Tyson Smith [:tsmith]]

Attachment: testcase.html

I'm not 100% sure if this is the right testcase for this crash. These assertions look similar and often get grouped in the same bucket.

Comment 9

[Darkspirit]

bp-cec72675-c45b-45a9-8543-93d000200714

called Option::unwrap() on a None value

Comment 10

[Dzmitry Malyshau [:kvark]]

Tyson, I'm not seeing Nightly crashing on it, and no fixes have gone in yet. Maybe you have other candidates, and I can check all of them?

Comment 11

[Dzmitry Malyshau [:kvark]]

Nvm, it doesn't crash on Linux but does crash on macOS. Investigating!

Comment 12

[Dzmitry Malyshau [:kvark]]

Attachment: Don't unwrap space mapping in add_prim_to_batch for text in WR

this is a crash I'm seeing on "testcase.html" attachment,
which is different from the originally reported one.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68618a10778e
Don't unwrap space mapping in add_prim_to_batch for text in WR r=gw,jnicol

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/68618a10778e

Comment 15

[Dzmitry Malyshau [:kvark]]

Another fix is needed here. Different Option:unwrap() are easily mixed up when looking at the crashes.

Comment 16

[Dzmitry Malyshau [:kvark]]

@gw I've been looking at this bug more, and I'm still not totally sure what happened there. Pernosco fails to see the most important pieces of local data and the stack (which get inlined/optimized)...
Given that it's a crash we produced, if it happens to show up again, we need to make sure the repro case is provided right away. That would make it easy for me to build a YAML test from it.
For now, let's get this in as it improves the code quality and robustness, even if it doesn't end up affecting the panic.

Also, attaching some more data, just in case it will be useful to anyone.

Comment 17

[Dzmitry Malyshau [:kvark]]

Attachment: primitive-info.md

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c19d6d676868
Refactor the clip task assignment logic in WR r=gw

Comment 19

[Alexandru Michis [:malexandru]]

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