Closed
Bug 1330401
Opened 8 years ago
Closed 8 years ago
stylo: crash when appending child to bound element with no insertion point
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: bholley, Unassigned)
References
Details
Attachments
(1 file)
1.40 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
This is the cause of the crash in layout/style/crashtests/469432-1.xhtml.
In a nutshell, we seem to be trying to do frame construction for a newly-appended explicit child of a bound element with no insertion points, and it's not clear to me how stylo should handle it.
The crash comes when we append a child to a bound element whose binding (menu#menuitem) doesn't have a <children> element and thus has a null mDefaultInsertionPoint. This causes nsBindingManager::ContentAppended to bail out at [1] before setting any kind of XBL binding parent on the newly-appended children.
Next we end up in nsCSSFrameConstructor::ContentAppended. We don't take the lazy frame construction path, and so we explicitly style the subtree of the bound element [2]. But the newly appended content isn't in the flattened tree at all, so it's never reached, and never styled by servo.
We then call GetRangeInsertionPoint [3]. There are lots of checks in there for various binding-related things that cause us to bail out of frame construction, but none of them seem to handle the case where the container has no insertion point. Instead, we end up just setting the insertion point to the container [4].
We then proceed along with frame construction, and eventually reach the point where we expect the stuff to be styled but it's not. I haven't traced the rest of it too much, since it seems to me like we ought to be bailing out earlier. But I'm not totally sure what the expected behavior is.
[1] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/dom/xbl/nsBindingManager.cpp#867
[2] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/base/nsCSSFrameConstructor.cpp#7387
[3] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/base/nsCSSFrameConstructor.cpp#7401
[4] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/base/nsCSSFrameConstructor.cpp#9256
Comment 2•8 years ago
|
||
I don't actually recall the expected "observable" behavior here in terms of what the resulting frame tree should look like. What does it end up looking like in Gecko without stylo?
But in general, two things:
1) If it's not in the flattened tree, it should not have frames. We should
be able to detect this and bail out as needed. Again, if Gecko without
stylo _doesn't_ create frames for this case, when does it detect that it
should not create them?
2) Things not in the flattened tree can totally have or have had style
(getComputedStyle, a different binding being attached to the parent that
doesn't have an insertion point for our kid, whatever happens with Shadow
DOM in terms of whether element mutations can cause an element to change
whether it matches existing insertion points, etc, etc). So we should make
sure those pieces end up working too. The "never styled by servo" for things
not in the flattened tree is probably ok for frame construction purposes,
of course.
Flags: needinfo?(bzbarsky)
Comment 3•8 years ago
|
||
Well, ok, I just tested Gecko. It creates a frame for the <select>.
For simplicity of testing, just set the "dom.allow_XUL_XBL_for_file" preference to true in about:config and load the crashtest file directly...
So either this thing is in the flattened tree (and whatever thinks it's not needs fixing) or it's not and then we should fix the "it gets a frame anyway" behavior.
Note that if I modify the testcase by placing this before the <menuitem>:
<input type="button" onclick="f()"/>
<script>
function f() {
var item = document.querySelector("menuitem");
item.style.display = "none";
document.documentElement.offsetWidth;
setTimeout(function() {
item.style.display = "";
}, 1000);
}
</script>
and clicking the button sure makes it look like the <select> is in the flattened tree, in the sense that doing frame construction starting at the <menuitem> still creates a frame for the <select>. On the other hand, if I put this button _after_ the <menuitem> then I don't obviously see a <select> after the button is clicked and the new frames are constructed.... wtf?
Reporter | ||
Comment 4•8 years ago
|
||
Attachment #8825987 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 5•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Gecko try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3ac98e685cb5c34bb2092a1d52e6002145092627
> Stylo try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=56285508752e490b16e50c05a1a6a019e523c3a6
These look green, modulo a couple of assertion expectations that need to be adjusted for stylo.
Comment 7•8 years ago
|
||
OK. So just to summarize:
1) We already have this bizarre difference between static and dynamic behavior: bug 474324.
2) The static behavior likely can't change. See 888787 comment 11.
3) This patch is only changing the dynamic behavior. The change is from
"insert it in some random place, or maybe don't show it"
(see bug 474324) to "don't show it". At least the new behavior is consistent...
I guess given the fragility of the current behavior the chance of someone depending on it is fairly low. We hope.
Comment 8•8 years ago
|
||
Comment on attachment 8825987 [details] [diff] [review]
Don't render explicit children of bound elements with no insertion points. v1
r=me, and fingers crossed.
Attachment #8825987 -
Flags: review?(bzbarsky) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e8ab4165a1
Don't render explicit children of bound elements with no insertion points. r=bz
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•