Entanglement game panics while cropping image in canvas thread

[jdm]

http://entanglement.gopherwoodstudios.com/

index 29536 out of range for slice of length 28968 (thread CanvasThread, at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libcore/slice.rs:578)
stack backtrace:
   0:        0x10dff9351 - backtrace::backtrace::trace::h6545df4cf0bad269
   1:        0x10dff1fc4 - backtrace::capture::Backtrace::new::ha2ca403b809d6889
   2:        0x10cf9d3eb - servo::main::_$u7b$$u7b$closure$u7d$$u7d$::h85aa86e426463240
   3:        0x11356ad54 - std::panicking::rust_panic_with_hook::h33761bada49f3713
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libstd/panicking.rs:556
   4:        0x11356ac34 - std::panicking::begin_panic<collections::string::String>
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libstd/panicking.rs:517
   5:        0x11356ab52 - std::panicking::begin_panic_fmt::h4c0c43306d52c68f
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libstd/panicking.rs:501
   6:        0x11356aab7 - rust_begin_unwind
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libstd/panicking.rs:477
   7:        0x1135943a0 - core::panicking::panic_fmt::hbd633f652cd3a047
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libcore/panicking.rs:69
   8:        0x113594499 - core::slice::slice_index_len_fail::h03f3b8ee39d37b51
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libcore/slice.rs:578
   9:        0x10df5c937 - _$LT$core..ops..Range$LT$usize$GT$$u20$as$u20$core..slice..SliceIndex$LT$T$GT$$GT$::index::h04293639053b9ddf
  10:        0x10df4e0ae - core::slice::_$LT$impl$u20$core..ops..Index$LT$I$GT$$u20$for$u20$$u5b$T$u5d$$GT$::index::h3471b9a54a6b2422
  11:        0x10df04154 - _$LT$collections..vec..Vec$LT$T$GT$$u20$as$u20$core..ops..Index$LT$core..ops..Range$LT$usize$GT$$GT$$GT$::index::h1ad1eef08b37f28c
  12:        0x10df64cb6 - canvas::canvas_paint_thread::crop_image::h268bfdbd90d72e2b
  13:        0x10df6060d - canvas::canvas_paint_thread::CanvasPaintThread::draw_image::h4d0034b5b537d014
  14:        0x10df00ce2 - canvas::canvas_paint_thread::CanvasPaintThread::start::_$u7b$$u7b$closure$u7d$$u7d$::he49fa60cee0ae681
  15:        0x10deffe2a - _$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::hdd81703e3d73574a
  16:        0x10df6e63e - std::panicking::try::do_call::hc8aa764ace046ad8
  17:        0x11356bc6a - __rust_maybe_catch_panic
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libpanic_unwind/lib.rs:98
  18:        0x10df6dd6a - std::panicking::try::h06d63f4ed6ae7124
  19:        0x10deef916 - std::panic::catch_unwind::h692c1a21a99f5971
  20:        0x10def117e - std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::h1ed031b0871efa30
  21:        0x10def8773 - _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::hfc0bf2c0bf5e3109
  22:        0x113569cd4 - std::sys::imp::thread::{{impl}}::new::thread_start
                        at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/liballoc/boxed.rs:615
  23:     0x7fff91cdf898 - _pthread_body
  24:     0x7fff91cdf729 - _pthread_start
ERROR:servo: index 29536 out of range for slice of length 28968

[jdm]

I'm pretty sure the problem is in the calculation of crop_area_bytes_length, which multiplies crop_rect.size.height by itself, and ignores crop_rect.size.width.

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[mozfreddyb]

I am taking a stab.

[mozfreddyb]

Here's what I did

index 4377855..2eb11d5 100644
--- a/components/canvas/canvas_paint_thread.rs
+++ b/components/canvas/canvas_paint_thread.rs
@@ -745,7 +745,7 @@ fn crop_image(image_data: Vec<u8>,
     // (consecutive elements in a pixel row of the image are contiguous in memory)
     let stride = image_size.width * 4;
     let image_bytes_length = image_size.height * image_size.width * 4;
-    let crop_area_bytes_length = crop_rect.size.height * crop_rect.size.height * 4;
+    let crop_area_bytes_length = crop_rect.size.height * crop_rect.size.width * 4;
     // If the image size is less or equal than the crop area we do nothing
     if image_bytes_length <= crop_area_bytes_length {
         return image_data;

Now, ./mach run http://entanglement.gopherwoodstudios.com/ gives some new errors on the console.
But I don't think they are so bad that the render window should remain blank 😕

ERROR:script::dom::bindings::error: Error at http://entanglement.gopherwoodstudios.com/j/en-US-v3-9-7.js:1:255637 bQ.fillText is not a function
ERROR:script::dom::bindings::error: Error at http://entanglement.gopherwoodstudios.com/j/en-US-v3-9-7.js:1:18085 Audio is not defined

[mozfreddyb]

To be clear, I get the same output and the same blank page without my patch applied. Please advise :)

[Ms2ger]

If the panic is gone, that's a valuable improvement already. The remaining errors refer to #11681 and #6711.

[mozfreddyb]

Alright, thanks! Wasn't sure whether the other problems would come before or after the panic. Pull Request coming in.

[jdm]

The changes in #15423 look good, but we need to write a new automated test that verifies that the fix works as expected.

[n0max]

I would like to work on this. Is drawing-images-to-the-canvas the correct position for the test?

@highfive: assign me

[highfive]

Hey @n0max! Thanks for your interest in working on this issue. It's now assigned to you!

[jdm]

No, we don't support the OffscreenCanvas API yet. A test belongs in tests/wpt/web-platform-tests/2dcontext/drawing-images-to-the-canvas/ instead.

[n0max]

I have some problems to write a test that fails without the fix. The error is only visible on a image with height > width and crop_rect.origin.x > 0.
I tried to simulate this with the 2x2.png and reduced width, but the image retains its original resolution. When I use a canvas as source, the image_data is already croped and crop_image returns in each case the unchanged image_data.
The only solution I see is to add new images for this test. For example 2x4.png and 4x2.png.

[jdm]

A data URL would work as well, but there's nothing wrong with adding new image files.