Don't send empty canvases to WebRender

[dralley]

If any dimension of a canvas is 0, don't try to display it as it causes
problems inside webrender.

Minimal test case available here: #21411 (comment)

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #___ (GitHub issue number if applicable)

• There are tests for these changes OR
• These changes do not require tests because ___

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @emilio: components/layout/display_list/builder.rs

[highfive]

Warning

• These commits modify layout code, but no tests are modified. Please consider adding a test!

[dralley]

Cannot run test-tidy locally because Fedora 31 is Python 3 only

[cbrewster]

Since we have a minimal test case, it would be nice to add a test that makes sure we don't regress on this panic.

Since this will be a servo-specific test, I would put it here: tests/wpt/mozilla/tests/mozilla/canvas

[dralley]

There's no issue specifically relating to zero-sized-canvas panic -- should I create one specifically to reference in a comment, or just reference the motionmark issue?

[cbrewster]

Referencing the motionmark issue would be fine since that's where most the discussion is at regarding this fix.

[cbrewster]

Can you run ./mach update-manifest and push, that will update the test manifest file.

Once that is done, this looks good to me. Thanks again!

[cbrewster]

@bors-servo r+

[bors-servo]

📌 Commit 468814a has been approved by cbrewster

[gterzian]

I'm pretty sure you want to apply the fix also to layout_2020. cc @SimonSapin

[cbrewster]

@gterzian ah good catch!
@bors-servo r-

[dralley]

No idea if that is the correct fix for layout 2020, but it seems like a decent guess.

[cbrewster]

Maybe it would make more sense here?

servo/components/layout_2020/replaced.rs

Lines 203 to 230 in 75ca3d8

	ReplacedContentKind::Canvas(canvas_info) => {
	let image_key = match canvas_info.source {
	CanvasSource::WebGL(image_key) => image_key,
	CanvasSource::Image(ref ipc_renderer) => match *ipc_renderer {
	Some(ref ipc_renderer) => {
	let ipc_renderer = ipc_renderer.lock().unwrap();
	let (sender, receiver) = ipc::channel().unwrap();
	ipc_renderer
	.send(CanvasMsg::FromLayout(
	FromLayoutMsg::SendData(sender),
	canvas_info.canvas_id,
	))
	.unwrap();
	receiver.recv().unwrap().image_key
	},
	None => return vec![],
	},
	};
	vec![Fragment::Image(ImageFragment {
	debug_id: DebugId::new(),
	style: style.clone(),
	rect: Rect {
	start_corner: Vec2::zero(),
	size,
	},
	image_key,
	})]
	},

It might be good to check with someone familiar with layout 2020 canvas.

cc @SimonSapin @ferjm

[SimonSapin]

This doesn’t look right: it’ll generate a non-replaced box (as for <div></div>), but we want to generate a replaced box, which might have different layout behavior (even if zero-size). Somewhere under layout_2020/display_list/ would be the right place for this change.

[servo-wpt-sync]

Error syncing changes upstream. Logs saved in error-snapshot-1585841672750.

[dralley]

Alright, I've tested this patch for layout 2020 and it seems to work... If there is somewhere better, let me know. I couldn't find anywhere that looked more appropriate.

Comparing float equality makes me cringe a little bit but that is the only size available at this location.

[SimonSapin]

Sorry, but this still doesn’t look right. Returning None from the constructor of ReplacedContent still cause the box to be non-replaced, instead of a zero-size replaced box. Somewhere under layout_2020/display_list/ would be the right place for this change.

[dralley]

@SimonSapin Apologies, but I simply do not know where this is supposed to go. The only usages of the word "replace" under the layout2020/display_list/ directory is:

• one occurance of mem::replace
• two occurances of use crate::replaced::IntrinsicSizes;

And the one place under layout2020/display_list/ where I thought it looked possibly reasonable for this to go -- did not fix the problem. I'm stumped.

Would it be better to just merge the fix for layout 2013 and file an issue for layout 2020? Or is there any chance you could narrow it down a bit more?

[SimonSapin]

Would it be better to just merge the fix for layout 2013 and file an issue for layout 2020?

That’s fine with me too.

---

This code generates a fragment for the replaced box for a <canvas> element:

servo/components/layout_2020/replaced.rs

Lines 203 to 230 in e66ab11

	ReplacedContentKind::Canvas(canvas_info) => {
	let image_key = match canvas_info.source {
	CanvasSource::WebGL(image_key) => image_key,
	CanvasSource::Image(ref ipc_renderer) => match *ipc_renderer {
	Some(ref ipc_renderer) => {
	let ipc_renderer = ipc_renderer.lock().unwrap();
	let (sender, receiver) = ipc::channel().unwrap();
	ipc_renderer
	.send(CanvasMsg::FromLayout(
	FromLayoutMsg::SendData(sender),
	canvas_info.canvas_id,
	))
	.unwrap();
	receiver.recv().unwrap().image_key
	},
	None => return vec![],
	},
	};
	vec![Fragment::Image(ImageFragment {
	debug_id: DebugId::new(),
	style: style.clone(),
	rect: Rect {
	start_corner: Vec2::zero(),
	size,
	},
	image_key,
	})]
	},

It generates a Fragment::Image, because at that point the canvas has already been rendered to a pixel buffer within WebRender). This image is then added to the display list here:

servo/components/layout_2020/display_list/mod.rs

Lines 84 to 99 in e66ab11

	Fragment::Image(i) => {
	builder.is_contentful = true;
	let rect = i
	.rect
	.to_physical(i.style.writing_mode, containing_block)
	.translate(containing_block.origin.to_vector());
	let common = builder.common_properties(rect.clone().to_webrender());
	builder.wr.push_image(
	&common,
	rect.to_webrender(),
	image_rendering(i.style.get_inherited_box().image_rendering),
	wr::AlphaType::PremultipliedAlpha,
	i.image_key,
	wr::ColorF::WHITE,
	);
	},

In that second location, it should be possible to check for i.rect.size == Vec2::zero() (with use crate::geom::Vec2; at the top of the file) to skip generating a display item for zero-size images (including rendered <canvas>). But if the problem this is attempting to fix is with rendering canvas to pixels in the first place, then in the first location we could check for (self.intrinsic.width, self.intrinsic.height) == (Some(Length::zero()), Some(Length::zero())) (with use style::Zero; at the top of the file) and return an empty Vec of zero fragments.

[dralley]

@SimonSapin Thanks! I think the more accurate description of this issue is probably the latter one, that Webrender shouldn't be rendering 0-size canvases. The latter patch you suggested works (slightly modified), the others did not.

[SimonSapin]

Looks good, thanks!

@bors-servo r=cbrewster,SimonSapin

[bors-servo]

📌 Commit 61fb84d has been approved by cbrewster,SimonSapin

[bors-servo]

⌛ Testing commit 61fb84d with merge f7d3d4a...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: cbrewster,SimonSapin
Pushing f7d3d4a to master...