1330401 - stylo: crash when appending child to bound element with no insertion point

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

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 1

[Bobby Holley (:bholley)]

Boris, what's the right behavior here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Bobby Holley (:bholley)]

Attachment: Don't render explicit children of bound elements with no insertion points. v1

Comment 5

[Bobby Holley (:bholley)]

Gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac98e685cb5c34bb2092a1d52e6002145092627
Stylo try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56285508752e490b16e50c05a1a6a019e523c3a6

Comment 6

[Bobby Holley (:bholley)]

(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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2e8ab4165a1