Crash when running the MotionMark benchmark #21411
Labels
A-gfx/uncategorized
A-webrender
C-assigned
There is someone working on resolving the issue
I-crash
No impact; the issue is one of maintainability or tidiness.
Comments
|
Instead of the previous assertion, I now see a stack overflow: |
jdm
added
I-crash
|
@highfive: assign me |
|
Hey @dralley! Thanks for your interest in working on this issue. It's now assigned to you! |
|
The issue is right here: https://github.com/servo/webrender/blob/master/webrender/src/renderer.rs#L3647 The texture
|
|
Unfortunately nobody on the servo team contributes to the webrender project, so we aren't in a good position to answer those questions. The folks in #gfx on Matrix will be better positioned for that. |
|
I should have also mentioned, both of the traces above are out of date. The current failure backtrace is: |
|
Recording some discussion on gfx matrix chat for posterity:
|
|
When I drop a debug print in here: I get edit: probably not an overflow problem, probably this: https://github.com/servo/webrender/blob/master/webrender/src/texture_cache.rs#L164
|
|
@dralley I found a smaller test-case for this. The problem happens when we put a 0x0 canvas image in a display list (I add a width/height to the style so this isn't culled from the display list). This will cause Servo to add the 0x0 image to WebRender which creates that empty texture cache entry (https://searchfox.org/mozilla-central/rev/8526066f548af9ec3ebb462ff73c47ccc183f533/gfx/wr/webrender/src/texture_cache.rs#1472). Later when WebRender tries to update the texture cache it encounters the invalid I'm not exactly sure what Servo should be doing here, but I hope this gives you a good place to start looking! <canvas id="myCanvas" width="0" height="0" style="width:10px; height:10px;"></canvas>
<script>
var c = document.getElementById("myCanvas");
var ctx = c.getContext("2d");
ctx.clearRect(0, 0, c.width, c.height);
</script>The canvas image is uploaded to WebRender here: servo/components/canvas/canvas_data.rs Lines 966 to 1005 in 4f5110f |
|
@cbrewster That information is super helpful, thanks! It looks like it used to be the case that WebRender would simply When I revert that commit, I get a panic at the assert location. So if this is restriction was deliberately relaxed, then I think the problem is not necessarily the fact that Servo is feeding WebRender with these empty textures, it's just that support for that is not complete? Unless I'm missing something. ================= A cache entry is created for so-called "empty" textures (where at least one dimension <=0), but it is never actually allocated. See this code: https://github.com/servo/webrender/blob/master/webrender/src/texture_cache.rs#L1472-L1486 It looks like But it's still present in |
|
This patch fixes the minimal test case - it now loads without error. Does it look correct? master...dralley:fix-motionmark Of course, that is not enough to fix MotionMark. I'm not sure if this is a side effect of the change, an entirely different issue, an symptom of an incomplete fix, etc. |
|
That looks like an issue that should be filed in https://github.com/jrmuizel/raqote/ once we identify the 2d canvas operation that is triggering it. |
|
@dralley that patch looks good to me! |
|
It looks like it fails while trying to draw a line to a point with a negative y-coordinate. |
|
@dralley it would be great if you could open a pull request with your change from above. My guess is the raqote panic is due to raqote assuming its draw target will not be zero-sized (similar issue to here, but different enough to maybe have its own issue filed). Also, just as a followup, the zero-sized cache item support in WebRender will likely be removed soon, it was added as a invalid glyph-caching mechanism, but was never used in the way it was intended. (more info https://bugzilla.mozilla.org/show_bug.cgi?id=1595768) |
5 tasks
bors-servo
added a commit
that referenced
this issue
Apr 3, 2020
Don't send empty canvases to WebRender 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) --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
@cbrewster The reason:
But, I have no idea which part of this needs to be fixed. |
|
@dralley I'm not very familiar with raqote, it would probably be worth opening an issue on the raqote repository and linking this issue. A possible solution on Servo's side would be to not issue canvas commands to the raqote draw target if the target is |
|
With the new raqote patch, it runs without crashing (although some benchmarks don't display properly or at all) until hitting this: which comes from |
|
I think we could work around that if we added "-moz-transition" to . |
|
@jdm It looks like the current version of that file is auto-generated: https://github.com/servo/servo/blob/master/components/script/dom/webidls/CSSStyleDeclaration.webidl But I can't seem to find the "GlobalGen.py" file that supposedly created this file, that the comment version references. I did find it in the git history, but it no longer seems to exist under that name. https://github.com/servo/servo/blob/ad02092b12891dee2bd8878019403185fb6f8678/components/script/dom/bindings/codegen/GlobalGen.py Do you know where it is? |
|
The generation now happens in servo/components/script/dom/bindings/codegen/run.py Lines 72 to 83 in 402db83 |
|
And that script is run during the build script. |
|
I think the easiest solution would be to modify the contents of |
|
@jdm Do you think there's possibly a deeper root cause here? I can't say really I understand what is going on with all of the codegen, but it kinda sorta looks like it should already have this prefix when |
|
Those extra prefixes aren't used in servo: servo/components/style/properties/data.py Lines 592 to 596 in a8b8f46 |
|
Gotcha |
|
Something like this? def add_css_properties_attributes(css_properties_json, parser):
css_properties = json.load(open(css_properties_json, "rb"))
def format_property_description(attribute_name, data):
return " [{maybe_pref}CEReactions, SetterThrows] attribute [TreatNullAs=EmptyString] DOMString {attribute_name};".format(
maybe_pref="" if not data["pref"] else 'Pref="{}", '.format(data["pref"]),
attribute_name=attribute_name,
)
items = []
for (kind, properties_list) in sorted(css_properties.items()):
for (property_name, data) in sorted(properties_list.items()):
for attribute_name in attribute_names(property_name):
items.append(format_property_description(attribute_name, data))
# re: servo/servo#21411
items.append(format_property_description("moz-transition", {"pref": None}))
idl = "partial interface CSSStyleDeclaration {\n%s\n};\n" % "\n".join(items)
parser.parse(idl.encode("utf-8"), "CSSStyleDeclaration_generated.webidl")(when I try to compile, I have this issue) |
|
That's the idea. You just need to add the missing functions to cssstyledeclaration, and it should be possible to just return the value of calling |
|
Unfortunately I'm still getting the same error after these changes. Here's my branch: master...dralley:fix-motionmark |
|
^ means the "prefix is undefined" error, not the compile error -- should have specified
|
|
It would be worth figuring out what the output of that code snippet from earlier is in Gecko, and compare it to the output for Servo. This would help isolate what we should be doing differently. |
|
So it looks like the problem is actually the first line of the function. Using <script>
var styles = window.getComputedStyle(document.documentElement, '');
console.log(styles);
console.log(JSON.stringify(styles));
</script>I get Whereas Firefox will return a CSS2Properties object with hundreds of fields. |
|
Hmm, I wonder if we should have a toJson method in the WebIDL. |
|
However https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface doesn't include one. I wonder what's different about Gecko's IDL? |
|
"CSSStyleDeclaration.webidl" looks basically the same between the two. Gecko) https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/CSSStyleDeclaration.webidl Servo) https://github.com/servo/servo/blob/master/components/script/dom/webidls/CSSStyleDeclaration.webidl Although, going by the type of the object returned by Firefox, what it's actually using is this: https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/CSS2Properties.webidl.in It kinda looks like it's inheriting from CSSStyleDeclaration.webidl and then adding some other magic stuff created by this script https://github.com/mozilla/gecko-dev/blob/master/dom/bindings/GenerateCSS2PropertiesWebIDL.py |
|
Given https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Description and the fact that CSSStyleDeclaration doesn't have a toString or toJSON method, my suspicion is that the DOM proxy code doesn't deal with enumerating properties correctly. One thing that makes this more clear: In Servo, this prints |
|
Unfortunately it's going to be a bit more complicated than that; it looks like the indexed getter on CSSStyleDeclaration is broken for any object returned from |
|
Do you think that deserves its own issue? |
|
Yep! Filed #26178. I think we should go ahead and close this issue, since the original panic has been fixed. |
|
@jdm Sounds good to me, there's already other issues for tracking motionmark problems. A few of them are no longer applicable, I'll comment on those. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This hits an assertion in WR (tested on linux):
ping @gw3583
The text was updated successfully, but these errors were encountered: