Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix crash in DL building #20752
Fix crash in DL building #20752
Conversation
Always use border_padding and writing_mode from the same fragment. Mixing them will trigger a debug assertion if the writing modes are different. Cleanup and use compute_background_clip
Style change in display_list/builder.rs to reduce the number of & and * found in the code. Rect<Au> are basically 4 integers so there is no need to pass by reference.
highfive
commented
May 5, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
May 5, 2018
|
|
This is not true, and rust will |
|
Code change itself looks good, but yeah, let's add a WPT test for this so it doesn't regress. |
| let (bounds, border_radii) = compute_background_clip( | ||
| color_clip, | ||
| *absolute_bounds, | ||
| style.logical_border_width().to_physical(style.writing_mode), |
This comment has been minimized.
This comment has been minimized.
emilio
May 6, 2018
Member
As a followup, let's double-check that computing the background-clip in terms of the padding of a fragment, but the border of another one is the right thing to do. Looks fishy.
This comment has been minimized.
This comment has been minimized.
pyfisch
May 6, 2018
Author
Contributor
Looks fishy.
It probably is. For block fragments it does not make a difference as it is always the same fragment but for inlines it is probably wrong. However I don't know how to correctly compute border_padding for inlines. I assumed it was padding + border for all four sides but there is more going on.
|
Added the test. |
| @@ -0,0 +1,14 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
| <title>Assure that different writing modes do not crash background building in Servo</title> | |||
This comment has been minimized.
This comment has been minimized.
emilio
May 7, 2018
Member
This isn't run as a test. This needs a testharness <script> and something like:
async_test(function(t) {
window.onload = t.step_func_done();
}, "Didn't crash");
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pyfisch
May 18, 2018
Author
Contributor
@jdm How do I best convert this to a reference test as I don't have a reference file?
This comment has been minimized.
This comment has been minimized.
|
Ok. I've added a reference to the test. Can you have a look? |
|
@bors-servo r=emilio |
|
|
Fix crash in DL building
Fix one crash and some style changes.
This HTML crashed servo before. Key parts are `background-clip: content-box` and `direction: rtl`
```html
<!DOCTYPE html>
<html>
<style>
#span1 {
background-clip: content-box;
}
#span2
{
direction: rtl;
}
</style>
<span id="span1">Filler Text <span id="span2">txeT relliF</span></span>
</html>
```
Should I add this as a test? And where do I put this "does-it-crash?" test?
I find always passing rectangles by value a lot easier as it avoids many references and dereferences and I assume that the compiler will always use the faster one either way. If you don't like the change feel free to only merge the first commit.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20752)
<!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Fix crash in DL building
Fix one crash and some style changes.
This HTML crashed servo before. Key parts are `background-clip: content-box` and `direction: rtl`
```html
<!DOCTYPE html>
<html>
<style>
#span1 {
background-clip: content-box;
}
#span2
{
direction: rtl;
}
</style>
<span id="span1">Filler Text <span id="span2">txeT relliF</span></span>
</html>
```
Should I add this as a test? And where do I put this "does-it-crash?" test?
I find always passing rectangles by value a lot easier as it avoids many references and dereferences and I assume that the compiler will always use the faster one either way. If you don't like the change feel free to only merge the first commit.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20752)
<!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Fix crash in DL building
Fix one crash and some style changes.
This HTML crashed servo before. Key parts are `background-clip: content-box` and `direction: rtl`
```html
<!DOCTYPE html>
<html>
<style>
#span1 {
background-clip: content-box;
}
#span2
{
direction: rtl;
}
</style>
<span id="span1">Filler Text <span id="span2">txeT relliF</span></span>
</html>
```
Should I add this as a test? And where do I put this "does-it-crash?" test?
I find always passing rectangles by value a lot easier as it avoids many references and dereferences and I assume that the compiler will always use the faster one either way. If you don't like the change feel free to only merge the first commit.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20752)
<!-- Reviewable:end -->
|
|
|
@bors-servo retry
|
Fix crash in DL building
Fix one crash and some style changes.
This HTML crashed servo before. Key parts are `background-clip: content-box` and `direction: rtl`
```html
<!DOCTYPE html>
<html>
<style>
#span1 {
background-clip: content-box;
}
#span2
{
direction: rtl;
}
</style>
<span id="span1">Filler Text <span id="span2">txeT relliF</span></span>
</html>
```
Should I add this as a test? And where do I put this "does-it-crash?" test?
I find always passing rectangles by value a lot easier as it avoids many references and dereferences and I assume that the compiler will always use the faster one either way. If you don't like the change feel free to only merge the first commit.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20752)
<!-- Reviewable:end -->
|
|
pyfisch commentedMay 5, 2018
•
edited
Fix one crash and some style changes.
This HTML crashed servo before. Key parts are
background-clip: content-boxanddirection: rtlShould I add this as a test? And where do I put this "does-it-crash?" test?
I find always passing rectangles by value a lot easier as it avoids many references and dereferences and I assume that the compiler will always use the faster one either way. If you don't like the change feel free to only merge the first commit.
This change is