canvas: Apply large size limitations by tharkum · Pull Request #36445 · servo/servo

tharkum · 2025-04-10T12:52:11Z

To prevent any potential crash/OOM issues with "canvas" element
from "rogue" applications let's apply large size limitations for context
canvas2d's draw target to Servo (similar approach in Firefox/Chromium -
they limits width and height to 32767/65535 pixels).

Fixes: #36155, #34117, #30164, #24710

--

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes
tests/wpt/tests/html/canvas/element/canvas-host/2d.canvas.host.size.large.html
tests/wpt/tests/html/canvas/offscreen/canvas-host/2d.canvas.host.size.large.html
tests/wpt/tests/html/canvas/offscreen/canvas-host/2d.canvas.host.size.large.worker.js

tharkum · 2025-04-10T12:53:07Z

cc @xiaochengh, @sagudev < Please review.

components/shared/canvas/canvas.rs

components/script/canvas_state.rs

components/script/dom/htmlcanvaselement.rs

tharkum · 2025-04-11T11:54:12Z

@xiaochengh < I decided to simplify PR and remove the "lazy allocation of canvas draw target" functionality
and prepare another PR for it.
So now this PR has only "is_paintable" validation for HTMLCanvasElement.

xiaochengh

Thanks for splitting it up and making it easier to review!

components/script/canvas_state.rs

components/script/dom/htmlcanvaselement.rs

xiaochengh · 2025-04-11T12:34:45Z

components/script/dom/offscreencanvas.rs

+            _ => false,
+        };
+
+        (paintable && !size.is_empty()) || is_supported_canvas_size(size)


Could you explain why OffScreenCanvas needs to check more than paintable?

You compared with outdated HTMLCanvasElement.rs (in the latest version they have similar check)
https://github.com/servo/servo/pull/36445/files#diff-7483a0f0ae501ab9da1432c28321c22f39647521072498f773903f360b026ce3R388

In an early message you mentioned that WebGL needs this check as a fixup, in which case I believe it should be moved to WebGL related types.

If it's general, then could you elaborate the difference between the paintability of a DrawTarget and that of a canvas (be it HTMLCanvas or offscreen)?

HTMLCanvasElement "paintable" (top level)

ContextCanvas2D DrawTarget "paintable" (low level)

WebGL/WebGPU DrawingBuffer "paintable" (low level)

There are some places in code there we could/must check possibility to snapshot image data OR draw from other canvas and current proposal was to up checking of "paintability" to top level instead of waiting until it will be done at context level (but it will be more CPU resourceble because we can do validation twice, but could help on some edge cases).

NOTE:
"Paintable" on low level (per context) will be validated against draw target size limitations + context lost status (for GPU accelerated rendering - not yet supported on 100%)

sagudev · 2025-04-11T12:59:47Z

Canvas context is paintable if:

"2d": draw target is paintable (depends from canvas size)
"webgl": always true (no context lost tracking)
"webgpu": always true

Hm, do we really need paintable concept? Looking at this PR give me idea to simply move canvas size handling to internal get_image_data of contexts: sagudev@1f34641 that builds on top of #36119 (while still not fixing max size, but that should be only mater of additional checks in get_image_data of 2d canvas context). WDYT?

tharkum · 2025-04-11T13:48:47Z

Canvas context is paintable if:
"2d": draw target is paintable (depends from canvas size)
"webgl": always true (no context lost tracking)
"webgpu": always true

Hm, do we really need paintable concept? Looking at this PR give me idea to simply move canvas size handling to internal get_image_data of contexts: sagudev@1f34641 that builds on top of #36119 (while still not fixing max size, but that should be only mater of additional checks in get_image_data of 2d canvas context). WDYT?

Your change with snapshot concept is a bit large, so need time to check it well, but at first view "paintable" concept is more top-level compare with "snapshot".

Instead of doing "paintable" validation inside each context method implementation we will do it inside HTMLCanvasElement.

"Paintable" concept will allow to do:

Preliminary check before call "get_image_data" from "createImageBitmap/toBlob/toDataURL" methods
and to return transparent black image ("snapshot::cleared") if canvas is not paintable.
Don't execute CanvasContext2D operations on non paintable draw target

Anyway I will check your PR and return back with more feedback.

sagudev · 2025-04-11T13:53:00Z

transparent black image ("snapshot::cleared") if canvas is not paintable

The problem here is that snapshot::cleared will do allocation, so in case of too big canvas we would still get OOM.

tharkum · 2025-04-11T14:06:35Z

transparent black image ("snapshot::cleared") if canvas is not paintable

The problem here is that snapshot::cleared will do allocation, so in case of too big canvas we would still get OOM.

https://github.com/servo/servo/pull/36119/files#diff-7483a0f0ae501ab9da1432c28321c22f39647521072498f773903f360b026ce3R382

How about to replace it to this "is_paintable" check (included validation of max canvas size) to avoid OOM?
if !self.paintable() ~~~~ ! (context_paintable || is_supported_canvas_size(size))

sagudev · 2025-04-11T14:15:33Z

transparent black image ("snapshot::cleared") if canvas is not paintable

The problem here is that snapshot::cleared will do allocation, so in case of too big canvas we would still get OOM.

https://github.com/servo/servo/pull/36119/files#diff-7483a0f0ae501ab9da1432c28321c22f39647521072498f773903f360b026ce3R382

How about to replace it to this "is_paintable" check (included validation of max canvas size) to avoid OOM? if !self.paintable() ~~~~ ! (context_paintable || is_supported_canvas_size(size))

That, would work, but that's included in sagudev@1f34641 (which I pushed just now, but already linked before). Actually rather than putting spotlight on my PR I just wanted to put it on sagudev@1f34641, which does similar as paintable but for each context in get_image_data. I think this is important for two reasons:

context have context specific requirements
makes get_image_data on specific context without HTMLCanvasElement actually usable in edge cases.

So I am questioning reasoning for this:

Instead of doing "paintable" validation inside each context method implementation we will do it inside HTMLCanvasElement.

Is there any reason why we cannot do this in each context and I am missing it?

tharkum · 2025-04-14T06:29:49Z

transparent black image ("snapshot::cleared") if canvas is not paintable

The problem here is that snapshot::cleared will do allocation, so in case of too big canvas we would still get OOM.

https://github.com/servo/servo/pull/36119/files#diff-7483a0f0ae501ab9da1432c28321c22f39647521072498f773903f360b026ce3R382
How about to replace it to this "is_paintable" check (included validation of max canvas size) to avoid OOM? if !self.paintable() ~~~~ ! (context_paintable || is_supported_canvas_size(size))

That, would work, but that's included in sagudev@1f34641 (which I pushed just now, but already linked before). Actually rather than putting spotlight on my PR I just wanted to put it on sagudev@1f34641, which does similar as paintable but for each context in get_image_data. I think this is important for two reasons:

context have context specific requirements

makes get_image_data on specific context without HTMLCanvasElement actually usable in edge cases.

So I am questioning reasoning for this:

Instead of doing "paintable" validation inside each context method implementation we will do it inside HTMLCanvasElement.

Is there any reason why we cannot do this in each context and I am missing it?

@sagudev < Sorry for long reply...

I viewed your "snapshot" PR and "yes" after your unification of "get_image_data" my proposed "paintable" concept
for HTMLCanvasElement could be omitted, because we have only one useful consumer (get image data)
of this change on top-level and it is more convenient to check restrictions on context level instead
of HTML canvas element.

There are one possible (but not mandatory) consumer of it is "CanvasState::draw_image_internal" for HTMLCanvasElement/OffscreenCanvas, but it could also be done per context level right now (with software image drawing).

So I can propose to cut-off "HTMLCanvasElement::is_paintable" change from PR and preserve only "CanvasState::is_paintable" for DrawTarget (to validate canvas size).

sagudev · 2025-04-14T06:42:50Z

So I can propose to cut-off "HTMLCanvasElement::is_paintable" change from PR and preserve only "CanvasState::is_paintable" for DrawTarget (to validate canvas size).

Sounds good to me.

BTW I created test site that allow to set custom canvas size on different context, to see how other browsers respond to edge cases (and to see what they return on toDataUrl), but I haven't got the time to test any browser https://sagudev.github.io/briefcase/canvas-sizes.html (it does not work on servo tho).

tharkum · 2025-04-14T06:53:51Z

So I can propose to cut-off "HTMLCanvasElement::is_paintable" change from PR and preserve only "CanvasState::is_paintable" for DrawTarget (to validate canvas size).

Sounds good to me.

BTW I created test site that allow to set custom canvas size on different context, to see how other browsers respond to edge cases (and to see what they return on toDataUrl), but I haven't got the time to test any browser https://sagudev.github.io/briefcase/canvas-sizes.html (it does not work on servo tho).

There are some existed application for testing canvas size:
https://jhildenbiddle.github.io/canvas-size/#/

sagudev · 2025-04-14T07:23:33Z

So I can propose to cut-off "HTMLCanvasElement::is_paintable" change from PR and preserve only "CanvasState::is_paintable" for DrawTarget (to validate canvas size).

Sounds good to me.
BTW I created test site that allow to set custom canvas size on different context, to see how other browsers respond to edge cases (and to see what they return on toDataUrl), but I haven't got the time to test any browser https://sagudev.github.io/briefcase/canvas-sizes.html (it does not work on servo tho).

There are some existed application for testing canvas size: https://jhildenbiddle.github.io/canvas-size/#/

But they do not test if they actually work or what does canvas methods return, the ones we are most interested for this PR are createImageBitmap, toDataUrl and toBlobUrl.

xiaochengh

LGTM.

Thanks for further splitting the PR and making it nice and clean!

mrobinson

Looks good, but I have a few very small suggestions:

mrobinson · 2025-04-15T19:04:03Z

components/script/canvas_state.rs

+    if size.is_empty() {
+        return false;
+    }
+    if size.width > MAX_CANVAS_SIZE || size.height > MAX_CANVAS_SIZE {
+        return false;
+    }
+    if size.area() > MAX_CANVAS_AREA {
+        return false;
+    }
+    true


I would just combine this one:

Suggested change

if size.is_empty() {

return false;

}

if size.width > MAX_CANVAS_SIZE || size.height > MAX_CANVAS_SIZE {

return false;

}

if size.area() > MAX_CANVAS_AREA {

return false;

}

true

!size.is_empty() &&

size <= Size2D::new(MAX_CANVAS_SIZE, MAX_CANVAS_SIZE) &&

size.area() < MAX_CANVAS_AREA

And size <= Size2D::new(MAX_CANVAS_SIZE, MAX_CANVAS_SIZE) is not supported
(PartialOrd is not implemeted for Size2D), so I replace it to
size.greater_than(Size2D::new(MAX_CANVAS_SIZE, MAX_CANVAS_SIZE)).none()

mrobinson · 2025-04-15T19:05:05Z

components/script/canvas_state.rs

+        }
+    }
+
+    fn get_paintable_size(&self) -> Size2D<u64> {


Getters in Rust usually don't use get_: https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter:

Suggested change

fn get_paintable_size(&self) -> Size2D<u64> {

fn paintable_size(&self) -> Size2D<u64> {

I know about Rust naming policy, but I was confused by existed getter methods names (like get_ipc_renderer, get_canvas_id), so I decided that in Servo are using non Rust naming policy is OK.

Servo is very old and predates Rust naming policy, so I think general rule of thumb is that new stuff try to follow it, but existing stuff can keep the old names.

mrobinson · 2025-04-15T19:06:33Z

components/script/canvas_state.rs

+
+#[derive(Clone, Debug, JSTraceable, MallocSizeOf)]
+/// Helps observe states of canvas draw target.
+struct DrawTarget {


The use of DrawTarget here is a little bit confusing, because this isn't the target of the draw itself, but metadata about it. I would call it DrawTargetSize.

Suggested change

struct DrawTarget {

struct DrawTargetSize {

Yes, it has mostly metadata in itself, but the goal is to have mirrored version (with size + state) of canvas draw target but on script thread. Later I have plan to add delayed creation of draw target for Canvas2D context (was cutoff from current version of PR) and it will add new filed "created/allocated" to this struct.

I was inspired by existed WebGPU implementation in Servo - it also have the same thing - metadata version of WebGPU drawing buffer on script thread.
https://github.com/servo/servo/blob/main/components/script/dom/webgpu/gpucanvascontext.rs#L54

Canvas2D output bitmap -> CanvasState::DrawTarget
WebGPU drawing buffer -> GPUCanvasContext:DrawingBuffer
WebGL drawing buffer -> None

Anyway if after you and/or @sagudev decide to rename it to DrawTargetSize - no problem.

Okay. That plan sounds interesting. For now let's give it a name that matches what it does. It's no problem to rename it later.

I was inspired by existed WebGPU implementation in Servo - it also have the same thing - metadata version of WebGPU drawing buffer on script thread.

The reason why we named it this way in WebGPU is because it matches the spec and it's also used referenced in spec steps (this way also out code matches spec better).

But here there are no exact steps written out in spec so either name is fine by me.

Okay. Let's call this DrawTargetSize then. Thanks!

@mrobinson < To do member renaming also?

draw_target -> draw_target_size?

Yes. Thanks!

mrobinson · 2025-04-15T19:07:01Z

components/script/canvas_state.rs

+const MAX_CANVAS_SIZE: u64 = 65535;
+
+#[derive(Clone, Debug, JSTraceable, MallocSizeOf)]
+/// Helps observe states of canvas draw target.


Please put documentation above the derive line and perhaps expand to:

Suggested change

/// Helps observe states of canvas draw target.

/// Information about the canvas draw target size and whether the size is valid.

mrobinson · 2025-04-15T19:10:31Z

components/script/canvas_state.rs

    }
 }

+pub(crate) fn is_supported_canvas_size(size: Size2D<u64>) -> bool {


Does this need to be pub(crate)? I would actually make this a static method on DrawTargetSize above.

impl DrawTargetSize ... fn is_supported_canvas_size() ...

Then you can call it using Self::is_supported_canvas_size(...).

Moved to DrawTarget struct. However may be better to move it to https://github.com/servo/servo/blob/main/components/shared/canvas/lib.rs ??

I would keep this in the impl block of DrawTargetSize.

sagudev · 2025-04-16T16:47:25Z

components/script/canvas_state.rs

+    fn paintable_size(&self) -> Size2D<u64> {
+        if self.paintable {
+            self.size
+        } else {
+            Size2D::zero()
+        }
+    }


So we will use paintable size for toDataUrl,toBlobUrl and similar (hence in edge cases we will error), but when rendering we will simply return empty htmlcanvassource (transparent black image as handled by WR/Layout), right? Can we document this somewhere in code.

tharkum · 2025-04-18T07:17:05Z

@mrobinson @sagudev < After your suggestions I reviewed PR code and decide to simplify a little bit all this mess "DrawTarget/DrawTargetSize" thing a lot. So i simply replaced it with just "size" field and use it as "paintable" marker for canvas state.

@sagudev < Regarding "toDataUrl,toBlobUrl" - good catch!
I didn't checked it well before and now I added state validation to prevent any pixel reading IPC call for non-paintable canvas (it be handled properly on top level in different places as transparent black pixel data with natural canvas size!).
After your "snapshot" PR will be landed it will be much more better handled in one place as snapshot::cleared().

sagudev · 2025-04-18T07:26:57Z

components/script/dom/canvasrenderingcontext2d.rs

+        if self.canvas_state.is_paintable() {
+            Some(
+                self.canvas_state
+                    .get_rect(self.size(), Rect::from_size(self.size())),
+            )
+        } else {
+            None
+        }


Shouldn't we do this as part of get_rect?

Fixed as CanvasState::get_rect() -> Option<Vec<u8>>

mrobinson

Looks good. Sorry for the long delay in reviewing!

tharkum · 2025-04-29T12:50:48Z

@mrobinson @sagudev < Please review.

I rebased it on top of "main" branch with "snapshot" feature.

To prevent any potential crash/OOM issues with <canvas> element from "rogue" applications let's apply upper size limitations for rendering context's output bitmap (draw target) to Servo (similar approach in Firefox/Chromium - they limits width and height to 32767/65535 pixels). Bug: servo#36155 Signed-off-by: Andrei Volykhin <[email protected]>

TimvdLippe · 2025-04-29T17:32:31Z

FYI the mentioned issues haven't been closed as they weren't part of separate lines. GitHub only closes the first issue in such a list.

sagudev · 2025-04-29T18:08:07Z

FYI the mentioned issues haven't been closed as they weren't part of separate lines. GitHub only closes the first issue in such a list.

Done.

tharkum requested a review from gterzian as a code owner April 10, 2025 12:52

tharkum mentioned this pull request Apr 10, 2025

Intermittent OK in /html/canvas/element/canvas-host/2d.canvas.host.size.large.html #36155

Closed

tharkum force-pushed the canvas2d-size-large branch from 69069ab to c3d8cdb Compare April 10, 2025 13:09

xiaochengh reviewed Apr 10, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch from c3d8cdb to 50dd0c8 Compare April 11, 2025 11:50

xiaochengh reviewed Apr 11, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch from 50dd0c8 to af0e53a Compare April 11, 2025 13:27

tharkum force-pushed the canvas2d-size-large branch from af0e53a to c5d98ad Compare April 14, 2025 07:46

tharkum requested a review from xiaochengh April 14, 2025 13:24

xiaochengh approved these changes Apr 15, 2025

View reviewed changes

sagudev self-requested a review April 15, 2025 17:41

mrobinson requested changes Apr 15, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch 2 times, most recently from 5e429aa to 9a251f3 Compare April 16, 2025 08:29

tharkum requested a review from mrobinson April 16, 2025 09:53

sagudev reviewed Apr 16, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch from 9a251f3 to f4a8b32 Compare April 18, 2025 07:07

sagudev reviewed Apr 18, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch from f4a8b32 to de29e50 Compare April 18, 2025 07:48

sagudev mentioned this pull request Apr 22, 2025

Introduce snapshot concept of canvas #36119

Merged

mrobinson approved these changes Apr 25, 2025

View reviewed changes

tharkum force-pushed the canvas2d-size-large branch from de29e50 to d26c041 Compare April 29, 2025 12:49

tharkum force-pushed the canvas2d-size-large branch from d26c041 to 4f7f22f Compare April 29, 2025 12:59

mrobinson added this pull request to the merge queue Apr 29, 2025

Merged via the queue into servo:main with commit 6b2a755 Apr 29, 2025
21 checks passed

tharkum deleted the canvas2d-size-large branch April 30, 2025 06:05

	fn get_paintable_size(&self) -> Size2D<u64> {
	fn paintable_size(&self) -> Size2D<u64> {

	/// Helps observe states of canvas draw target.
	/// Information about the canvas draw target size and whether the size is valid.

Uh oh!

Conversation

tharkum commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tharkum commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tharkum commented Apr 11, 2025

Uh oh!

xiaochengh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaochengh Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagudev commented Apr 11, 2025

Uh oh!

tharkum commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagudev commented Apr 11, 2025

Uh oh!

tharkum commented Apr 11, 2025

Uh oh!

sagudev commented Apr 11, 2025

Uh oh!

tharkum commented Apr 14, 2025

Uh oh!

sagudev commented Apr 14, 2025

Uh oh!

tharkum commented Apr 14, 2025

Uh oh!

sagudev commented Apr 14, 2025

Uh oh!

xiaochengh left a comment

Choose a reason for hiding this comment

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tharkum commented Apr 10, 2025 •

edited

Loading

xiaochengh Apr 11, 2025 •

edited

Loading

tharkum commented Apr 11, 2025 •

edited

Loading