pixels: Add limitation to max image total bytes length#37172
pixels: Add limitation to max image total bytes length#37172mrobinson merged 1 commit intoservo:mainfrom
Conversation
tharkum
force-pushed
the
max-image-total-bytes-lenght
branch
from
bf48891 to
b2b5c49
Compare
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Nice start. I think you should write a test for this change. You can add to to the mozilla suite which means that it is specific to Servo. For instance you can try to create an ImageData and OffscreenCanvas that is too large and observe the failure via catching and verifying the exception. It isn't enough that tests don't failure for new behavior like this; it must also be tested. The exception is when it isn't possible to test the new behavior for some reason or another.
Be honest I didn't run WPT tests even on local PC right now, so "Testing" in commit message is not trustful yet. There are some test candidates for this change, I expect that they should be PASSED:
|
If there is already existing WPT test coverage you shouldn't need to copy the tests. Servo already runs the WPT tests are part of it's test coverage. It's enough that the tests are newly passing. Once the build is working here, I'll trigger tests. Be sure to sure both "./mach test-tidy" and "./mach test-unit -r" locally an fix any issues that arise, then update your branch. |
tharkum
force-pushed
the
max-image-total-bytes-lenght
branch
from
b2b5c49 to
36734db
Compare
Fixed CRASHES caused by |
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Looks good. Thanks for the changes. Just a couple minor comments:
Limit maximum image allocation size up to 2GB to minimize
possibility of process killing due to OOM on some operations
with ImageBitmap/ImageData/Canvas/OffscreenCanvas
(construction, toBlob, toDataURL).
Chromium: 2^32-1 (~4GB)
Firefox: 2^31-1 (~2GB)
Testing:
- html/canvas/element/pixel-manipulation/2d.imageData.object.ctor.basics.html
assert_throws_dom("INDEX_SIZE_ERR", function() { new ImageData(1 << 31, 1 << 31); });
- html/canvas/element/manual/imagebitmap/createImageBitmap-invalid-args.html
makeOversizedCanvas + makeOversizedOffscreenCanvas
Signed-off-by: Andrei Volykhin <[email protected]>
tharkum
force-pushed
the
max-image-total-bytes-lenght
branch
from
36734db to
54032dc
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* main: (510 commits) DevTools: Fix empty `debugger > source` panel (servo#37197) dom: implement signal abort on controller and signal (servo#37192) build(deps): bump parking_lot from 0.12.3 to 0.12.4 (servo#37199) layout: Split overflow calculation after fragment tree construction (servo#37203) build(deps): bump parking_lot_core from 0.9.10 to 0.9.11 (servo#37202) build(deps): bump lock_api from 0.4.12 to 0.4.13 (servo#37201) build(deps): bump cc from 1.2.24 to 1.2.25 (servo#37198) Constellation can now optionally report memory usage when the page is loaded. (servo#37151) Implement Input `type=text` UA Shadow DOM (servo#37065) constellation: Wait for canvas thread to shut down before shutting down system font service (servo#37182) Add slot default display style test (servo#37189) Send synthetic keydown/keyup at ime_insert_text (servo#37175) script: Let canvas serialization to image fail gracefully (servo#37184) Implement basics of link preloading (servo#37036) compositor: Add an initial RefreshDriver (servo#37169) pixels: Add limitation to max image total bytes length (servo#37172) Chore: Remove unused variable in `transition-zero-duration-with-delay.html` (servo#37179) build(deps): bump ohos-ime from 0.2.0 to 0.3.0 (servo#37180) Add a user agent style for the `<slot>` element (servo#37174) build(deps): bump hitrace from 0.1.4 to 0.1.5 (servo#37170) ...
2 participants
Limit the maximum image allocation size to 2GB to minimize the possibility of out of memory errors on some
ImageBitmap,ImageData,Canvas, andOffscreenCanvasoperations such as construction,toBlob, andtoDataURL. Other browsers have similar limits:Testing: Improvements to the following tests:
html/canvas/element/pixel-manipulation/2d.imageData.object.ctor.basics.htmlassert_throws_dom("INDEX_SIZE_ERR", function() { new ImageData(1 << 31, 1 << 31); });
html/canvas/element/manual/imagebitmap/createImageBitmap-invalid-args.htmlmakeOversizedCanvas + makeOversizedOffscreenCanvas