Add a sanity check for render task sizes #2956

nical · 2018-08-08T13:16:38Z

This PR moves the crash happening when failing to allocate large render task textures to the place where the size is assigned in the hope that the stack trace will be more actionable. This will make it easier to investigate #2955.

I'm not sure whether the explicit panic!() is needed or if the error!() macro already panics.

This change is


        Add a sanity check for render task sizes.

kvark · 2018-08-08T17:58:46Z

webrender/src/render_task.rs

+    if size.width > RENDER_TASK_SIZE_SANITY_CHECK ||
+        size.height > RENDER_TASK_SIZE_SANITY_CHECK {
+        error!("Attempting to create a render task of size {}x{}", size.width, size.height);
+        panic!();


let's just have this message in panic!

The reason I put it in error!() is that it gets logged in a special gfxCriticalError thing which I can search for in crash-stats. I don't know how to search through panic messages in crash-stats although it might be possible.

kvark · 2018-08-08T17:58:46Z

webrender/src/render_task.rs

@@ -458,6 +470,8 @@ impl RenderTask {
            }
        }

+        render_task_sanity_check(&outer_rect.size);


would it be easier (and more robust) if we just had a constructor like RenderTask::new_dynamic(size, kind, clear_mode, children) where the check is done?

I have a mild preference towards keeping the struct initialization syntax instead of using ::new methods when construction is simple because the latter isn't very easy to read due to the lack of named arguments.
My hope is that within a few weeks of this sanity check being in the wild we'll have data pointing us to the missing places where we need to be careful about render task sizes and we can then remove the check (or replace it with something a bit better where we check against the real maximum size or something that makes sense on a per-type-of-item basis).

I have a mild preference towards keeping the struct initialization syntax instead of using ::new methods when construction is simple

I agree! In my book the reasoning no longer stands if there is any logic that needs to be done though, which is the case here.

and we can then remove the check

I don't see why we'd want to remove the check ever. Better put it once and forever in a nice spot to protect us against eternal evils of infinite render tasks.

My own book has it that it's not worth losing the readability for a single sanity check function call that is only there for debugging purposes, but that's not really worth arguing over, I'll add the constructor.

I don't understand where "losing the readability" comes from. What am I missing?

oh, I was misreading it as "reliability", silly me


        Move the render task sanity checks in constructors.

kvark · 2018-08-09T16:38:18Z

@bors-servo r+

bors-servo · 2018-08-09T16:38:18Z

📌 Commit 2dd663e has been approved by kvark

bors-servo · 2018-08-09T16:38:21Z

⌛ Testing commit 2dd663e with merge e475061...


        Auto merge of #2956 - nical:large-rendertask, r=kvark

Add a sanity check for render task sizes This PR moves the crash happening when failing to allocate large render task textures to the place where the size is assigned in the hope that the stack trace will be more actionable. This will make it easier to investigate #2955. I'm not sure whether the explicit `panic!()` is needed or if the `error!()` macro already panics.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2956)

bors-servo · 2018-08-09T18:54:12Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing e475061 to master...

gw3583 · 2018-08-10T04:10:00Z

@nical @kvark This explicit panic occurs during the crash tests in CI. It may not be the same as the crash we're looking for, but I imagine it should be an easy fix:

thread 'WRRenderBackend#3' panicked at 'explicit panic', gfx/webrender/src/render_task.rs:43:9
TEST-UNEXPECTED-FAIL | file:///Users/cltbld/tasks/task_1533871261/build/tests/reftest/tests/layout/base/crashtests/265973-1.html | application terminated with exit code 1
REFTEST PROCESS-CRASH | file:///Users/cltbld/tasks/task_1533871261/build/tests/reftest/tests/layout/base/crashtests/265973-1.html | application crashed [@ mozalloc_abort]
Return code: 1
No suite end message was emitted by this harness.
# TBPL FAILURE #
The reftest suite: crashtest ran with return status: FAILURE
[taskcluster:error] exit status 2

gw3583 · 2018-08-10T04:24:34Z

Ah, I wonder if in this case it might be a false positive - it's possible that the size of the root render task gets set very large if the window size is very large, even though the root render task doesn't allocate any targets. Although .. that should get picked up before any render tasks get allocated, I think, so maybe that's not it.

nical · 2018-08-10T09:15:23Z

Link ot the failing try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b313719e75e0e1f9169cd8f6c38cc227d7d21cb0&selectedJob=193217773

The crashtest is:

<html>
<head>
</head>

<body>
<iframe style="border-top-width: 31378748; border-bottom-right-radius: 23895784; ">
</body>
</html>

Looking into it.

nical · 2018-08-10T10:24:11Z

So the crash is very similar to what was fixed in #2916 except that in this instance it is the border width and not the border radius that causes us to allocate a huge render task. I'll put a fix up today.


        Auto merge of #2966 - nical:huge-border-width, r=kvark

Prevent border widths to allocate huge render task surfaces. Following up from #2956 this PR makes us scale down the the intermediate render task we use to rasterize borders if the width is larger than 2k, just like we do with border radii.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2966)

Add a sanity check for render task sizes.

Loading status checks…

854019d

kvark reviewed Aug 8, 2018

View changes

Move the render task sanity checks in constructors.

Loading status checks…

2dd663e

nical force-pushed the nical:large-rendertask branch from abd86de to 2dd663e Aug 9, 2018

kvark approved these changes Aug 9, 2018

View changes

bors-servo merged commit 2dd663e into servo:master Aug 9, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

nical deleted the nical:large-rendertask branch Aug 10, 2018

nical mentioned this pull request Aug 10, 2018

Prevent border widths to allocate huge render task surfaces. #2966

Merged

servo / webrender

Add a sanity check for render task sizes #2956

Add a sanity check for render task sizes #2956

nical commented Aug 8, 2018 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Aug 9, 2018

bors-servo commented Aug 9, 2018

bors-servo commented Aug 9, 2018

bors-servo commented Aug 9, 2018

gw3583 commented Aug 10, 2018

gw3583 commented Aug 10, 2018

nical commented Aug 10, 2018

nical commented Aug 10, 2018

servo / webrender

Join GitHub today

Add a sanity check for render task sizes #2956

Add a sanity check for render task sizes #2956

Conversation

nical commented Aug 8, 2018 • edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Aug 9, 2018

bors-servo commented Aug 9, 2018

bors-servo commented Aug 9, 2018

bors-servo commented Aug 9, 2018

gw3583 commented Aug 10, 2018

gw3583 commented Aug 10, 2018

nical commented Aug 10, 2018

nical commented Aug 10, 2018

nical commented Aug 8, 2018 •

edited by larsbergstrom