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 race in parallel selector matching #1319
Comments
|
I also tested Yellow River page with other computer at |
|
It seems to crash after 155befe (Rewrite flow construction to be incrementalizable and parallelizable.) I reduced the Yellow River wikipedia page to the following and am still able to reproduce the error mentioned above. <!DOCTYPE html>
<html>
<head>
<style type="text/css">
#baz_id{
float:right;
}
</style>
<script src="real-mini-load_004.php">
</script>
</head>
<body>
<span id="baz_id">
<span>foo</span>
<span>bar</span>
</span>
</body>
</html>The script file is https://gist.github.com/pradeep90/7715923 @pcwalton When you said you got a race, did you mean that you got the error above ("This case makes no sense")? Or was it something else? |
|
Update: <!DOCTYPE html>
<html>
<head>
<style type="text/css">
#baz_id{
float:right;
}
</style>
<script type="text/javascript">
var divChild = document.createElement("div");
var safeFragment = document.createDocumentFragment();
var fragmentDiv = safeFragment.appendChild(divChild);
</script>
</head>
<body>
<span id="baz_id">
<span>foo</span>
<span>bar</span>
</span>
</body>
</html> |
|
Does this still reproduce on master? |
|
I didn't notice that the fragmentDiv is never appended to the main document. It makes sense that #1252 would not have any effect here, now that I see that. |
|
Just noticed that, even with parallel selector matching disabled, Servo gives the Can anybody else confirm this (by using the master branch code)? @pcwalton When you said you got a race, did you mean that you got the error above ("This case makes no sense")? Or was it something else? |
|
I can confirm on master. What an interesting testcase! |
|
There are identical symptoms if the script content is simply var safeFragment = document.createDocumentFragment();
safeFragment.textContent = "hi";I don't know what to make of this. |
|
Looking at the output of RUST_LOG=servo::layout, the difference between two runs in which one appends the DocumentFragment to a div and the other does the reverse is that the second reflow request in the crashing case is a MatchSelectorsDocumentDamage one, while the other is ReflowDocumentDamage (and also counts as a resize). Both runs construct the same flow tree, but the reflowing one measures some boxes and creates float contexts with non-zero sizes, while the selector matching one has zero sizes everywhere. |
|
Ok, I think the initial resize event is racing with the initial document modification events. Window::content_changed calls Page::reflow_all without setting a particular type of damage, so if no resize event comes in we default to matching selectors. |
|
It seems like we might be propagating damage incorrectly, since when a resize is not performed we don't end up calling bubble_widths on the floated elements. |
|
With the latest master code, I don't get the above error anymore on the mini HTML above. It works. However, for the original Yellow River HTML, I get this error and backtrace |
|
We're also seeing that same failure on the mac builders when running test_collections.html sometimes. |
|
Here's a reduced version. Every part of this is required: <a><img/></a> <div>a</div> |
|
Yellow River now displays correctly with master. Can anybody else try the Yellow River page with https://github.com/mozilla/servo/blob/master/src/components/main/css/matching.rs line 50 having let num_tasks = rt::default_sched_threads() * 2;instead of let num_tasks = 1; |
|
I see a bunch of strange crashes with that change (including #1386 regularly), but no FcPatternDestroy. |
|
@jdm Hmm... I see ElementLike::get_attr being used many times in the So, I guess the parallel access of |
|
I'm not sure, since I imagine the tasks would be looking at different elements. |
|
@jdm The |
|
In IRC we discussed using cast::forget in get_attribute's call to map() to see if what's causing this is the Attribute going out of scope, racing on the refcount, and causing an early free. That's not a real fix however. We'll need to change @mut Attr here to JSManaged when we merge the JSManaged branch. I think @jdm plans to do that soon. |
|
I'm not able to reproduce the crash anymore (even without any of the above |
The problems that were caused because of Yellow River seem to be fixed. (See servo#1319).
|
I guess the crash I saw above might have been because of some older build. |
|
#1429 landed so close it |
The problems that were caused because of Yellow River seem to be fixed. (See servo#1319).
Parallel selector matching was disabled in #1315 due to races.
Mainly, the page at http://en.wikipedia.org/wiki/Yellow_River gives the following error for me
Since the original author is not working on this project anymore, I'm trying to locate the commit which introduced the race condition.
Note that at the time when @ILyoan added parallel selector matching, Wikipedia pages weren't displaying properly in Servo.
Last commit where the Yellow River page seems to work: june0cho@ee5ff3f (the general Wikipedia issues were fixed by this time)
So, I'm guessing that in some commit between the above commit and a76f761, the code causing the race condition was introduced.
Are there any suggestions for how to approach this?