1375674 - stylo: Table change hint computation is probably wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 1374752 added code to nsTableFrame::UpdateStyleOfOwnedAnonBoxesForTableWrapper that subtracts changehints for the table from those for the wrapper.  That seems wrong to me, because the wrapper is an _ancestor_ of the table, not the other way around.

That patch also left the comments about "aHintForThisFrame" (which don't exist), which explain _why_ this is not an OK thing to do, so I'm pretty confused about what happened there, exactly.

I don't have time right now to sort through the various other changes made in that patch to check whether they're right or not, nor whether the changes in the patch it was trying to fix fallout from are right or not; I would really appreciate it if someone could check all that stuff carefully.

On a separate note, I find the ServoRestyleState thing pretty confusing; it makes it far too easy to pass along the wrong changehint because you don't realize there's one hiding in there.  :(

Comment 1

[Emilio Cobos Álvarez [:emilio]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0)
> Bug 1374752 added code to
> nsTableFrame::UpdateStyleOfOwnedAnonBoxesForTableWrapper that subtracts
> changehints for the table from those for the wrapper.  That seems wrong to
> me, because the wrapper is an _ancestor_ of the table, not the other way
> around.
> 
> That patch also left the comments about "aHintForThisFrame" (which don't
> exist), which explain _why_ this is not an OK thing to do, so I'm pretty
> confused about what happened there, exactly.

Ouch, right, damn table wrapper... Now about how easy would be to do a reftest for this, but I'll try :(.

> I don't have time right now to sort through the various other changes made
> in that patch to check whether they're right or not, nor whether the changes
> in the patch it was trying to fix fallout from are right or not; I would
> really appreciate it if someone could check all that stuff carefully.

I will, sorry for rushing on it.

> On a separate note, I find the ServoRestyleState thing pretty confusing; it
> makes it far too easy to pass along the wrong changehint because you don't
> realize there's one hiding in there.  :(

The idea is that worst case you pase a change handled for an ancestor, which isn't wrong, but I can also split it if you want.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Given I'm failing really badly to build a reftest for this, I'll at least add proper assertions...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> The idea is that worst case you pase a change handled for an ancestor

Ah, good point.  Given that, keeping it in the struct makes sense.

I'm also not sure how easy it is to write a test for this; I recall running into test failures before when I got it wrong, but we're not getting those now, which is a bit confusing to me....

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> > The idea is that worst case you pase a change handled for an ancestor
> 
> Ah, good point.  Given that, keeping it in the struct makes sense.
> 
> I'm also not sure how easy it is to write a test for this; I recall running
> into test failures before when I got it wrong, but we're not getting those
> now, which is a bit confusing to me....

I'm writing a bunch of assertions, and they're catching a few bugs so far.

These assertions rely on the change hint being accessible only through an accessor that gets the frame being handled, so I'll keep it for now. Hopefully this is the last time we have to worry about this stuff :P

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #0)
> Bug 1374752 added code to
> nsTableFrame::UpdateStyleOfOwnedAnonBoxesForTableWrapper that subtracts
> changehints for the table from those for the wrapper.  That seems wrong to
> me, because the wrapper is an _ancestor_ of the table, not the other way
> around.

Totally an oversight on my part reviewing.  Thanks for noticing!

Comment 6

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5561b3cd11ac1635116eb034579a69cb346a301

I think things that are buggy on current central are:

 * The table wrapper and viewport frames (introduced by bug 1374752).
 * display: contents (pre-existing).
 * out of flows (pre-existing).

Re. display: contents, we're skipping posting non-reconstruct change hints for them (since they have no frames), yet we're adding it to the "handled for descendants" bucket. We're getting saved in most cases because we don't add changes for most of the style structs, but the following test-case still fails:

<!doctype html>
<style>
  .contents { color: red; display: contents; }
  .contents:hover { will-change: opacity; color: green; }
</style>
<div class="contents">
  Test
</div>

I'm not totally sure how the code could (can?) work for out of flows, given they can be reordered in the frame tree, so if I had a structure like:

<div id=d1 style="position: relative">
   <div id=d2>
     <div id=d3 style="position: absolute">
     </div>
   </div>
</div>

And we had something like a reflow hint in d2, and another in d3, we'll avoid posting the hint for d3 thinking that it's been handled already, but it really wouldn't be the case, because the parent frame of d3 is d1. Unless of course all the handled hints visit placeholder frames, which I don't know why would they.

Cameron, is the out-of-flow bit right?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e37067b1ee3f5a617e35352e286d08e77f04cdcd

Comment 8

[Emilio Cobos Álvarez [:emilio]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b5561b3cd11ac1635116eb034579a69cb346a301
> 
> I think things that are buggy on current central are:
> 
>  * The table wrapper and viewport frames (introduced by bug 1374752).
>  * display: contents (pre-existing).
>  * out of flows (pre-existing).
> 
> Re. display: contents, we're skipping posting non-reconstruct change hints
> for them (since they have no frames), yet we're adding it to the "handled
> for descendants" bucket. We're getting saved in most cases because we don't
> add changes for most of the style structs, but the following test-case still
> fails:
> 
> <!doctype html>
> <style>
>   .contents { color: red; display: contents; }
>   .contents:hover { will-change: opacity; color: green; }
> </style>
> <div class="contents">
>   Test
> </div>
> 
> I'm not totally sure how the code could (can?) work for out of flows, given
> they can be reordered in the frame tree, so if I had a structure like:
> 
> <div id=d1 style="position: relative">
>    <div id=d2>
>      <div id=d3 style="position: absolute">
>      </div>
>    </div>
> </div>
> 
> And we had something like a reflow hint in d2, and another in d3, we'll
> avoid posting the hint for d3 thinking that it's been handled already, but
> it really wouldn't be the case, because the parent frame of d3 is d1. Unless
> of course all the handled hints visit placeholder frames, which I don't know
> why would they.
> 
> Cameron, is the out-of-flow bit right?

Other things that were wrong:

 * ::backdrop
 * <area> elements (that goes as far back as I know I think).

The assertions cover all of them. Though I could only build a reftest for the display: contents thing.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1375674: Don't use the owner's changes handled for the backdrop frame.

Review commit: https://reviewboard.mozilla.org/r/152244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152244/

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1375674: Don't use the table change hints for the table wrapper frame.

Review commit: https://reviewboard.mozilla.org/r/152246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152246/

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

In particular, this fixes:

 * Restyling of <area> elements that reuse the primary frame mapping for the
   image map.

 * Restyling of children of display: contents elements when the display:
   contents element generates a hint.

 * Restyling of out-of-flows (if my analysis in the bug is right).

 * Restyling of the ::backdrop pseudo-element.

 * Restyling of the Viewport frame.

I only managed to do a reftest for the second one, but the rest of them are
covered by the assertions added.

Review commit: https://reviewboard.mozilla.org/r/152248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152248/

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1375674: Reftest for the display: contents issue.

Review commit: https://reviewboard.mozilla.org/r/152250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152250/

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880878 [details]
Bug 1375674: Don't use the owner's changes handled for the backdrop frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152244/diff/1-2/

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152246/diff/1-2/

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152248/diff/1-2/

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880881 [details]
Bug 1375674: Reftest for the display: contents issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152250/diff/1-2/

Comment 17

[Boris Zbarsky [:bzbarsky]]

> * display: contents (pre-existing).
> * out of flows (pre-existing).

Oh, good catch!

> * <area> elements (that goes as far back as I know I think).

Do your patches fix bug 1375390 by any chance?

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5478cc1731cc29441cabc841936cde4a6e22bd77

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #17)
> > * display: contents (pre-existing).
> > * out of flows (pre-existing).
> 
> Oh, good catch!
> 
> > * <area> elements (that goes as far back as I know I think).
> 
> Do your patches fix bug 1375390 by any chance?

Gah, I just rebased, so I'll need to wait a bit for a build... But it's likely.

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Re. display: contents, we're skipping posting non-reconstruct change hints
> for them (since they have no frames), yet we're adding it to the "handled
> for descendants" bucket. We're getting saved in most cases because we don't
> add changes for most of the style structs, but the following test-case still
> fails:
> 
> <!doctype html>
> <style>
>   .contents { color: red; display: contents; }
>   .contents:hover { will-change: opacity; color: green; }
> </style>
> <div class="contents">
>   Test
> </div>

That is a good find.  (Looks like Gecko is not touching mHintsHandledBySelf for display:contents elements, even if we generated ReconstructFrame for it.)

> I'm not totally sure how the code could (can?) work for out of flows, given
> they can be reordered in the frame tree, so if I had a structure like:
> 
> <div id=d1 style="position: relative">
>    <div id=d2>
>      <div id=d3 style="position: absolute">
>      </div>
>    </div>
> </div>
> 
> And we had something like a reflow hint in d2, and another in d3, we'll
> avoid posting the hint for d3 thinking that it's been handled already, but
> it really wouldn't be the case, because the parent frame of d3 is d1. Unless
> of course all the handled hints visit placeholder frames, which I don't know
> why would they.
> 
> Cameron, is the out-of-flow bit right?

Indeed.  Gecko handles this by assuming no hints have been handled by ancestors when it traverses into an out-of-flow frame:

http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/GeckoRestyleManager.cpp#1100-1114

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8880878 [details]
Bug 1375674: Don't use the owner's changes handled for the backdrop frame.

https://reviewboard.mozilla.org/r/152244/#review157450

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

https://reviewboard.mozilla.org/r/152246/#review157452

::: layout/tables/nsTableFrame.cpp:8029
(Diff revision 2)
>    // Figure out whether we have an actual change.  It's important that we do
>    // this, even though all the wrapper's changes are due to properties it
>    // inherits from us, because it's possible that no one ever asked us for those
>    // style structs and hence changes to them aren't reflected in
> -  // aHintForThisFrame at all.
> +  // the handled changes at all.

A similar change needs to be made in the comment in nsIFrame::UpdateStyleOfOwnedChildFrame.

Actually, the comment in that function looks like it's saying the same thing as the one here, but with the additional point about addons being able to change anon box styles.  I guess that applies here too, since *|*::-moz-table-wrapper styles could be changed by addons.  So maybe sync these comments up?

::: layout/tables/nsTableFrame.cpp:8039
(Diff revision 2)
>    if (wrapperHint) {
>      aRestyleState.ChangeList().AppendChange(
>        aWrapperFrame, aWrapperFrame->GetContent(), wrapperHint);
>    }

In GeckoRestyleManager, we do this thing where we force the hints generated by the table frame to be applied to the wrapper frame too:

http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/GeckoRestyleManager.cpp#2403-2413

Do we need something similar here, and if not, why not?

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

https://reviewboard.mozilla.org/r/152246/#review157452

> A similar change needs to be made in the comment in nsIFrame::UpdateStyleOfOwnedChildFrame.
> 
> Actually, the comment in that function looks like it's saying the same thing as the one here, but with the additional point about addons being able to change anon box styles.  I guess that applies here too, since *|*::-moz-table-wrapper styles could be changed by addons.  So maybe sync these comments up?

Well, I think the comments should be slightly different. UpdateStyleOfOwnedChildFrame _can_ use the changes handled by the ancestor frame, because we're updating a frame down in the frame tree. That's not the case for the wrapper frame. Will add a comment to note that.

> In GeckoRestyleManager, we do this thing where we force the hints generated by the table frame to be applied to the wrapper frame too:
> 
> http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/GeckoRestyleManager.cpp#2403-2413
> 
> Do we need something similar here, and if not, why not?

I'm not sure why would it be needed. That code seems to go back to bug 219444, and the test-case (https://bug219444.bmoattachments.org/attachment.cgi?id=131592) definitely passes with stylo and these patches.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152246/diff/2-3/

Comment 24

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152248/diff/2-3/

Comment 25

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880881 [details]
Bug 1375674: Reftest for the display: contents issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152250/diff/2-3/

Comment 26

[Cameron McCormack (:heycam)]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

https://reviewboard.mozilla.org/r/152248/#review157454

::: layout/base/ServoRestyleManager.h:61
(Diff revision 2)
> +  // This is used as of right now only for out of flow frames, since they don't
> +  // necessarily are parented in DOM order, and thus a change handled by a DOM

s/don't necessarily are/aren't necessarily/

::: layout/base/ServoRestyleManager.h:103
(Diff revision 2)
> +  // generated the last change that is stored in mChangesHandled, in order to
> +  // verify that we only use mChangesHandled for actual descendants of that
> +  // frame (given DOM order isn't always frame order, and that there are a few
> +  // special cases for stuff like wrapper frames, ::backdrop, and so on).
> +#ifdef DEBUG
> +  const nsIFrame* mOwner { nullptr };

Nit: please just use a regular |= nullptr| here rather than the uniform initialization syntax.

::: layout/base/ServoRestyleManager.cpp:37
(Diff revision 2)
> +const nsIFrame*
> +FirstContinuationOrPartOfIBSplit(const nsIFrame* aFrame)
> +{
> +  if (!aFrame) {
> +    return nullptr;
> +  }
> +
> +  while (const nsIFrame* continuation = aFrame->GetPrevContinuation()) {
> +    aFrame = continuation;
> +  }
> +
> +  while (auto* prevIBSplitSibling =
> +           aFrame->GetProperty(nsIFrame::IBSplitPrevSibling())) {
> +    aFrame = prevIBSplitSibling;
> +  }
> +
> +  return aFrame;
> +}

You can use nsLayoutUtils::FirstContinuationOrIBSplitSibling.

::: layout/base/ServoRestyleManager.cpp:56
(Diff revision 2)
> +static const nsIFrame*
> +ExpectedOwnerForChild(const nsIFrame& aFrame)

I find this function a bit hard to review for correctness.  If there is anything simpler you can do to assert the thing we care about here (that mChangesHandled only includes changes that would apply to the child), that would be good.

Could we instead just (more weakly) check that FirstContinuationOrIBSplit(mOwner) has aParent.mOwner somewhere in its ancestor chain?  (Aside from some special cases around the viewport frame?)

::: layout/base/ServoRestyleManager.cpp:59
(Diff revision 2)
> +  if (IsAnonBox(aFrame) || aFrame.IsBulletFrame()) {
> +    return aFrame.GetParent()->IsViewportFrame() ? nullptr : aFrame.GetParent();
> +  }

When can Bullet frames be children of Viewport frames?

::: layout/base/ServoRestyleManager.cpp:69
(Diff revision 2)
> +  if (aFrame.IsTableFrame()) {
> +    MOZ_ASSERT(parent->IsTableWrapperFrame());
> +    parent = FirstContinuationOrPartOfIBSplit(parent->GetParent());
> +  }
> +
> +  while (parent && (IsAnonBox(*parent) || parent->IsLineFrame() ||

Why loop while we see IsAnonBox?  Couldn't that loop past the actual parent ServoRestyleState::mOwner?

::: layout/base/ServoRestyleManager.cpp:70
(Diff revision 2)
> +    MOZ_ASSERT(parent->IsTableWrapperFrame());
> +    parent = FirstContinuationOrPartOfIBSplit(parent->GetParent());
> +  }
> +
> +  while (parent && (IsAnonBox(*parent) || parent->IsLineFrame() ||
> +                    parent->IsPopupSetFrame())) {

Why do we need this?

::: layout/base/ServoRestyleManager.cpp:78
(Diff revision 2)
> +      parent = parent->GetProperty(nsIFrame::IBSplitPrevSibling());
> +      MOZ_ASSERT(parent);
> +    } else if (pseudo == nsCSSAnonBoxes::tableWrapper) {
> +      const nsIFrame* tableFrame = parent->PrincipalChildList().FirstChild();
> +      MOZ_ASSERT(tableFrame->IsTableFrame());
> +      parent = IsAnonBox(*tableFrame) ? parent->GetParent() : tableFrame;

Why do this IsAnonBox(*tableFrame) check?

::: layout/base/ServoRestyleManager.cpp:395
(Diff revision 2)
>    // Grab the change hint from Servo.
>    nsChangeHint changeHint = Servo_TakeChangeHint(aElement);
> -  changeHint =
> -    NS_RemoveSubsumedHints(changeHint, aRestyleState.ChangesHandled());
>  
> +  // We should really fix bug 135040...

Nit: might be good to preserve the part of the comment about this specifically being about image maps, so that we don't need to look up the bug from the number here.

::: layout/base/ServoRestyleManager.cpp:408
(Diff revision 2)
>    if (aElement->HasFlag(NODE_NEEDS_FRAME)) {
>      changeHint |= nsChangeHint_ReconstructFrame;
> -    // The only time the primary frame is non-null is when image maps do hacky
> -    // SetPrimaryFrame calls.
> -    MOZ_ASSERT(!styleFrame || styleFrame->IsImageFrame());
> -    styleFrame = nullptr;
> +    MOZ_ASSERT(!styleFrame);
> +  }
> +
> +  if (styleFrame && !styleFrame->StyleDisplay()->IsOutOfFlowStyle()) {

I think we should use !styleFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW).  That should include display:-moz-popup things, which is the other case that is out of flow:

http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/nsCSSFrameConstructor.cpp#1214-1227

Please also add a comment here explaining why we can't do this for out-of-flow frames.

::: layout/base/ServoRestyleManager.cpp:470
(Diff revision 2)
> +    if (styleFrame->StyleDisplay()->IsOutOfFlowStyle()) {
> +      thisFrameRestyleState.emplace(*styleFrame,
> +                                    aRestyleState,
> +                                    changeHint,
> +                                    ServoRestyleState::OutOfFlowTag);
> +    } else {
> +      thisFrameRestyleState.emplace(*styleFrame, aRestyleState, changeHint);
> +    }

Same comment applies here about using NS_FRAME_OUT_OF_FLOW.

Also, given that we have to switch here, it seems like we may as well use an enum with two values and just do:

  ServoRestyleState::Type = styleFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)
    ? ServoRestyleState::Type::OutOfFlow
    : SerovRestyleState::Type::Normal;
  thisFrameRestyleState.emplace(*styleFrame, aRestyleState, changeHint, type);

or something like that.

::: layout/base/ServoRestyleManager.cpp:480
(Diff revision 2)
> +  ServoRestyleState& childrenRestyleState =
> +    thisFrameRestyleState ? *thisFrameRestyleState : aRestyleState;

Add a comment here saying why why do this.  (It's for both display:contents and for the silly image map thing, right?)

::: layout/base/ServoRestyleManager.cpp:522
(Diff revision 2)
> -        viewport->UpdateStyle(childrenRestyleState);
> +        // NB: The root restyle state, not the one for our children!
> +        viewport->UpdateStyle(aRestyleState);

Maybe assert here that aRestyleState has nothing in its mChangesHandled?  Unless the other assertions catch this.

Comment 27

[Cameron McCormack (:heycam)]

Comment on attachment 8880881 [details]
Bug 1375674: Reftest for the display: contents issue.

https://reviewboard.mozilla.org/r/152250/#review157458

Comment 28

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> I'm not sure why would it be needed. That code seems to go back to bug
> 219444, and the test-case
> (https://bug219444.bmoattachments.org/attachment.cgi?id=131592) definitely
> passes with stylo and these patches.

I guess the way it is doing things, it manages to still remove subsumed hints generated by ancestors of the table wrapper frame, when computing the change hint for the table inner frame (by removing subsumed hints generated by the table wrapper frame or ancestors, and duplicating the hints the table wrapper generates onto the table inner frame).  Not sure that's really possible with the current anon box styling setup we have here.

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

https://reviewboard.mozilla.org/r/152246/#review157462

Comment 30

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

https://reviewboard.mozilla.org/r/152248/#review157454

> You can use nsLayoutUtils::FirstContinuationOrIBSplitSibling.

Oh, cool, thanks!

> I find this function a bit hard to review for correctness.  If there is anything simpler you can do to assert the thing we care about here (that mChangesHandled only includes changes that would apply to the child), that would be good.
> 
> Could we instead just (more weakly) check that FirstContinuationOrIBSplit(mOwner) has aParent.mOwner somewhere in its ancestor chain?  (Aside from some special cases around the viewport frame?)

Yeah, we could, but I'd really prefer to keep it if it's not a big deal, it's a bit more strict, but I think it's worth it.

> When can Bullet frames be children of Viewport frames?

They can't, but for bullets we know we want to return the parent frame, so it seemed slightly easier to just merge the condition with the anon box case. I'll split them.

> Why loop while we see IsAnonBox?  Couldn't that loop past the actual parent ServoRestyleState::mOwner?

No, the key here is that you check whether it's an anon-box at the top (in that case you know we have just the parent frame, due to how the UpdateStyleForAnonBoxes machinery works).

Otherwise, you're checking for an element style (or first-line, etc). In that case, given we use the relevant frames from the DOM, the parent we expect is the style frame from our DOM parent, so we need to hop through anon-boxes.

I can add comments for these.

If this is for some reason too hard to maintain, then we can always reconsider and go back to the "it's somewhere in our parent chain".

> Why do we need this?

Hmm... I don't think I have a better answer for this than "the popup set is weird re. frame construction, and it appeared between frames belonging to two different contents in a crashtest".

> Why do this IsAnonBox(*tableFrame) check?

:-moz-table and :-moz-inline-table (in that case we want to still go through it).

> I think we should use !styleFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW).  That should include display:-moz-popup things, which is the other case that is out of flow:
> 
> http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/layout/base/nsCSSFrameConstructor.cpp#1214-1227
> 
> Please also add a comment here explaining why we can't do this for out-of-flow frames.

Oh, good catch! I guess that will make the IsPopupSetFrame thing go away, which is nice.

> Same comment applies here about using NS_FRAME_OUT_OF_FLOW.
> 
> Also, given that we have to switch here, it seems like we may as well use an enum with two values and just do:
> 
>   ServoRestyleState::Type = styleFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)
>     ? ServoRestyleState::Type::OutOfFlow
>     : SerovRestyleState::Type::Normal;
>   thisFrameRestyleState.emplace(*styleFrame, aRestyleState, changeHint, type);
> 
> or something like that.

Yup, that sounds fine, will do.

> Add a comment here saying why why do this.  (It's for both display:contents and for the silly image map thing, right?)

Right, will do. I'm not sure it's needed for the area thing, since I don't think descendant of `<area>` elements are rendered? But yeah, worth noting that.

> Maybe assert here that aRestyleState has nothing in its mChangesHandled?  Unless the other assertions catch this.

The !mChangesHandledAssertions assertion in `ChangesHandledFor` should catch this.

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

https://reviewboard.mozilla.org/r/152248/#review157470

::: layout/base/ServoRestyleManager.cpp:33
(Diff revision 3)
> +  return aFrame.StyleContext()->GetPseudo() &&
> +         nsCSSAnonBoxes::IsAnonBox(aFrame.StyleContext()->GetPseudo());

Oh, this should just be:

  return aFrame.StyleContext()->IsAnonBox();

to avoid the search that nsCSSAnonBoxes::IsAnonBox does.

Comment 32

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1375674: Constify and simplify nsLayoutUtils::{First,Last}ContinuationOrIBSplitSibling.

Review commit: https://reviewboard.mozilla.org/r/152420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152420/

Comment 33

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880878 [details]
Bug 1375674: Don't use the owner's changes handled for the backdrop frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152244/diff/2-3/

Comment 34

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152246/diff/3-4/

Comment 35

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152248/diff/3-4/

Comment 36

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880881 [details]
Bug 1375674: Reftest for the display: contents issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152250/diff/3-4/

Comment 37

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

https://reviewboard.mozilla.org/r/152248/#review157490

::: layout/base/ServoRestyleManager.h:99
(Diff revision 4)
> +  // generated the last change that is stored in mChangesHandled, in order to
> +  // verify that we only use mChangesHandled for actual descendants of that
> +  // frame (given DOM order isn't always frame order, and that there are a few
> +  // special cases for stuff like wrapper frames, ::backdrop, and so on).
> +#ifdef DEBUG
> +  const nsIFrame* mOwner { nullptr };

Forgot to address this one, done locally.

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

https://reviewboard.mozilla.org/r/152248/#review157668

::: layout/base/ServoRestyleManager.cpp:36
(Diff revision 4)
> +const nsIFrame*
> +FirstContinuationOrPartOfIBSplit(const nsIFrame* aFrame)

static

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8881112 [details]
Bug 1375674: Constify and simplify nsLayoutUtils::{First,Last}ContinuationOrIBSplitSibling.

https://reviewboard.mozilla.org/r/152420/#review157674

Comment 40

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8881112 [details]
Bug 1375674: Constify and simplify nsLayoutUtils::{First,Last}ContinuationOrIBSplitSibling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152420/diff/1-2/

Comment 41

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880878 [details]
Bug 1375674: Don't use the owner's changes handled for the backdrop frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152244/diff/3-4/

Comment 42

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880879 [details]
Bug 1375674: Don't use the table change hints for the table wrapper frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152246/diff/4-5/

Comment 43

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880880 [details]
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152248/diff/4-5/

Comment 44

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8880881 [details]
Bug 1375674: Reftest for the display: contents issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152250/diff/4-5/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aa6a89faab4
Constify and simplify nsLayoutUtils::{First,Last}ContinuationOrIBSplitSibling. r=heycam
https://hg.mozilla.org/integration/autoland/rev/87dac0783b76
Don't use the owner's changes handled for the backdrop frame. r=heycam
https://hg.mozilla.org/integration/autoland/rev/93f5483053a3
Don't use the table change hints for the table wrapper frame. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1b05406b2248
Track the owner that represents the changes handled, and fix a bunch of issues while at it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/15fc474cbc38
Reftest for the display: contents issue. r=heycam

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7aa6a89faab4
https://hg.mozilla.org/mozilla-central/rev/87dac0783b76
https://hg.mozilla.org/mozilla-central/rev/93f5483053a3
https://hg.mozilla.org/mozilla-central/rev/1b05406b2248
https://hg.mozilla.org/mozilla-central/rev/15fc474cbc38