Closed Bug 1322570 Opened 8 years ago Closed 7 years ago

stylo: assert against calling GetParent for stylo-backed style contexts

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: TYLin)

References

Details

Attachments

(9 files, 1 obsolete file)

Bug 1322570 Part 1 - Add MOZ_ASSERT in nsStyleContext::GetParent() to disallow usage by stylo.
59 bytes, text/x-review-board-request
Details
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1322570 Part 3 - Get StyleContext from parent frame in nsFlexContainerFrame::Init().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1322570 Part 4 - Use GetParentAllowServo() in KeyframeEffectReadOnly::UpdateProperties.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1322570 Part 5 - Use GetParentAllowServo() related to first letter frame.
59 bytes, text/x-review-board-request
Details
Bug 1322570 Part 6 - Use GetParentAllowServo() in RestyleManager.
59 bytes, text/x-review-board-request
Details
Bug 1322570 Part 7 - Use GetParentAllowServo() in nsMathMLChar.
59 bytes, text/x-review-board-request
Details
Bug 1322570 Part 8 - Run debug code only if the style source is a gecko rule node.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1322570 Part 9 - Print StyleContext parents in frame tree dump only if they're gecko rule nodes.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
In bug 1322568, I'm disabling some style context tree structure verification assertions, but we might want to re-enable and adjust/loosen them.
Moving the bug 1322568 needinfo to here.
Flags: needinfo?(bzbarsky)
So I've looked at this a bit.  Quoting an email discussion about this stuff...

Quite apart from fixing the various anon box and pseudo-element bugs we have, here's a case in which this assertion fires for "legitimate" reasons:

    <body>
        document.body.offsetWidth;
        var s = document.createElement("span");
        span.style.borderColor = "inherit"; // Not actually needed
        document.body.appendChild(s);
        document.body.style.borderColor = "green";
    </body>

What happens here?  We create and append the span.  Then we change the color on the <body>.  The next time we land in ServoRestyleManager::ProcessPendingRestyles, we call StyleDocument() to compute all the servo-side stuff.  Then we do PresContext()->FrameConstructor()->CreateNeededFrames() which creates a frame for the <span>.  This frame has a style context whose parent is the current style context of the <body> (with black border-color).  But its style data comes from the servo side, which has already recomputed everything, so it has a green border-color, for example.  I think, at least... 

Anyway, now we go to RecreateStyleContexts, get to the <span> (and yes, we do reach it; the <body> thinks it HasDirtyDescendantsForServo()).  We get its current style context, check its computed style against the "new" computed style, and of course they're identical.  So we leave its frankenstein-ish style context in place and hit the assert.

I'm not sure we even need the lazy frame construction bit.  We just need a situation in which <body> has a dirty descendant and the <span>'s style doesn't change.
OK, so in practical terms the stylo behavior is good: the <span>'s style didn't actually change, so we have no reason to create a new nsStyleContext for it, much less its descendants.   We can just prune out that whole part of the subtree traversal.  That's great, because that "no-op" walk (reparenting style contexts in the Gecko case when we manage to optimize it) totally shows up in profiles.

But it does trigger the tree verification assertions.

I don't see a clear way to reenable even a weakened version of the assertions given the above.  But it does mean that there's a minefield there: Anyone who calls GetParent() on that <span>'s style context and then uses its structs to inherit from will get incorrect style.

What we should probably try to do is get rid of such callers.  In other words, get rid of nsStyleContext::GetParent in code that can run when using servo.  Most simply, we should assert if GetParent() is called when mSource.IsServoComputedValues() and then fix all the resulting fallout.

And there will certainly be fallout.  For example, KeyframeEffectReadOnly::UpdateProperties uses GetParent() and then styles from there....  I wonder whether we can write a failing testcase for it based on the above.  

And there's lots of GetParent() uses in the frame constructor to get a thing to inherit from too.  :(
Flags: needinfo?(bzbarsky)
Priority: -- → P1
Summary: stylo: maybe re-enable style context tree verification assertions → stylo: assert against calling GetParent for stylo-backed style contexts
Ting Yu, would you be interested in looking into this?

The basic issue here is that the Servo-backed style system doesn't nsStyleContext::mParent (it does all the property inheritance eagerly during the styling algorithm, and so the ServoComputedValues in mSource have all the information).

Eventually we'd like to remove the tree state (mParent etc) from nsStyleContext to save memory, but we can't do that yet because we still need to support the gecko style system. However, during restyle we can save a bunch of work by not updating mParent, and so it can become invalid with Stylo. See comment 3.

Given that, we'd like to MOZ_ASSERT(mSource.IsGeckoRuleNode()) in nsStyleContext::GetParent() - i.e. mParent should never be used with the servo style system. There's probably some stuff that relies on this, and so we may need to do some refactoring.

As a first step, it's probably worth doing some investigation and making a list of all of the potential callers and brainstorming how we might fix them (searchfox could work well, or a non-fatal NS_ASSERTION to get callstacks). If it turns out that we _really_ need to use mParent, we may need to change our approach. Otherwise, the next step would be to fix those, and add the MOZ_ASSERT.

Let me know if that all makes sense and if you have any other questions. Thanks for helping us out!
Assignee: nobody → tlin
There's a known callsite in http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/layout/generic/nsFirstLetterFrame.cpp#389 that we need to address.  For now we should probably just whitelist it from the assertions, until we figure out how to address it.
To elaborate on comment 5, bz and I spoke a bit about this today. There are a few known callsites:

(1) nsFirstLetterFrame. We haven't implemented proper ::first-letter support yet, so we should just whitelist this one (for example, with a GetParentAllowServo() getter which returns mParent without asserting).
(2) Some keyframe stuff for animations - probably worth talking with Boris Chiou about these.
(3) A bunch of stuff in the frame constructor that we should take a look at.

And then a bunch of other stuff. We shouldn't spend too much time stuck on any single callsite, since some of the fixes may depend on work we haven't started yet (like proper ::first-letter / ::first-line support). The goal here is just to do an audit, fix up the easy ones, and make a list of the hard ones.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> (1) nsFirstLetterFrame. We haven't implemented proper ::first-letter support
> yet, so we should just whitelist this one (for example, with a
> GetParentAllowServo() getter which returns mParent without asserting).
> (2) Some keyframe stuff for animations - probably worth talking with Boris
> Chiou about these.

As for this call site, I think you refer to KeyframeEffectReadOnly::UpdateProperties() [1], we can drop the call when we invoke UpdateProperties() from a SquentialTask (We will need to do this, for example, if we have width:10em -> 20em animation when we change font-size of its target element). 

[1] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/dom/animation/KeyframeEffectReadOnly.cpp#310
Try to summarize what I need to do here.

The problem is nsStyleContext::mParent in servo-backed style system should not be needed, so GetParent() should only be used in Gecko-backed style system. But we still have some call sites of GetParent() mentioned in comment 6. 

Thus, we need to

1) Add MOZ_ASSERT(mSource.IsGeckoRuleNode()) in nsStyleContext::GetParent(), and see what it breaks.
2) If GetParent() is not needed, fix it.
3) If it breaks because something is not supported by stylo or still needed by now, whitelist it with GetParentAllowServo(), which is a GetParent() without assertion so that we could revise those GetParentAllowServo() calls later.

I guess with all the above steps done, my patches should pass linux64-stylo.
Yes, that sounds perfect!
Awesome work Ting-Yu, thank you!

NI bz for the review here, since he's driving this part of the stylo work.
Flags: needinfo?(bzbarsky)
See Also: → 1349004
Comment on attachment 8849090 [details]
Bug 1322570 Part 4 - Use GetParentAllowServo() in KeyframeEffectReadOnly::UpdateProperties.

https://reviewboard.mozilla.org/r/121886/#review124166

Thank you Ting-Yu!
I just opened bug 1349004 for dropping nsStyleContext for UpdateProperties() in stylo.  Please leave FIXME comment with this bug number there.
Thanks!
Attachment #8849090 - Flags: review?(hikezoe) → review+
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124158

::: commit-message-0865d:3
(Diff revision 1)
> +Bug 1322570 Part 2 - Rewrite nsStylePosition methods to stop using nsStyleContext::GetParent().
> +
> +We assume the StyleContext's parent equals to the parent frame's StyleContext.

BAD NEWS: your stated assumption here (that StyleContext parent == parent frame's stylecontext) does *not* hold up in general. nsTableFrame/nsTableOuterFrame is one example where the frametree parentage is the opposite of the nsStyleContext parentage.

GOOD NEWS: When this assumption doesn't hold up, the parent-nsFrame is actually more likely to be correct than the parent-nsStyleContext.  This was clarified https://github.com/w3c/csswg-drafts/commit/d58f54955dd2528832be6ae83a50631c2e9fef12 .

So anyway -- you probably need to rewrite this extended-commit message in light of the above.

::: layout/style/nsStyleStruct.h:1757
(Diff revision 1)
>    /**
> -   * Return the used value for 'align-self' given our parent StyleContext
> +   * Return the used value for 'align-self' given a parent frame
>     * aParent (or null for the root).
>     */
> -  uint8_t UsedAlignSelf(nsStyleContext* aParent) const;
> +  uint8_t UsedAlignSelf(const nsIFrame* aParent) const;

Why do we need to change the function signature here?

If I understand correctly, we're just trying to remove callsites that currently do:
 UsedAlignSelf(frame->StyleContext()->GetParent())

Can't we just fix those up to:
 UsedAlignSelf(frame->GetParent()->StyleContext())
and leave this function-signature intact? (still taking a nsStyleContext* which corresponds to the parent frame)

If so & if you leave the function signature untouched, I think a lot of this patch can be reverted -- i.e. a lot of this patch is unnecessary changes right now (the parts where we *are* the parent and we're just passing in "StyleContext()" directly with no explicit GetParent calls)
Attachment #8849088 - Flags: review?(dholbert) → review-
Comment on attachment 8849089 [details]
Bug 1322570 Part 3 - Get StyleContext from parent frame in nsFlexContainerFrame::Init().

https://reviewboard.mozilla.org/r/121884/#review124176

r=me on this part with one nit:

::: layout/generic/nsFlexContainerFrame.cpp:2366
(Diff revision 1)
>    // If this frame is for a scrollable element, then it will actually have
>    // "display:block", and its *parent* will have the real flex-flavored display
>    // value. So in that case, check the parent to find out if we're legacy.
>    if (!isLegacyBox && styleDisp->mDisplay == mozilla::StyleDisplay::Block) {
> -    nsStyleContext* parentStyleContext = mStyleContext->GetParent();
> +    nsStyleContext* parentStyleContext = GetParent()->StyleContext();

Please tweak the (preexisting) comment before this line as follows, to make this change more clearly-correct:

s/*parent* will have/*parent-frame* will have/

(And rewrap as-needed to keep the comment lines <= 80 characters long.)
Attachment #8849089 - Flags: review?(dholbert) → review+
Comment on attachment 8849087 [details]
Bug 1322570 Part 1 - Add MOZ_ASSERT in nsStyleContext::GetParent() to disallow usage by stylo.

https://reviewboard.mozilla.org/r/121880/#review124278

::: commit-message-1b929:1
(Diff revision 1)
> +Bug 1322570 Part 1 - Add MOZ_ASSERT in nsStyleContext::GetParent() to disallow uasge by stylo.

"usage"
Attachment #8849087 - Flags: review+
Comment on attachment 8849091 [details]
Bug 1322570 Part 5 - Use GetParentAllowServo() related to first letter frame.

https://reviewboard.mozilla.org/r/121888/#review124280
Attachment #8849091 - Flags: review+
Comment on attachment 8849092 [details]
Bug 1322570 Part 6 - Use GetParentAllowServo() in RestyleManager.

https://reviewboard.mozilla.org/r/121890/#review124282

r=me, though we will need to figure out some better fix here at some point...
Attachment #8849092 - Flags: review+
Comment on attachment 8849093 [details]
Bug 1322570 Part 7 - Use GetParentAllowServo() in nsMathMLChar.

https://reviewboard.mozilla.org/r/121892/#review124284
Attachment #8849093 - Flags: review+
Comment on attachment 8849094 [details]
Bug 1322570 Part 8 - Run debug code only if the style source is a gecko rule node.

https://reviewboard.mozilla.org/r/121894/#review124288

For this one, I'm going to punt to Bobby, actually.  The thing this is trying to assert is that if we're not reframing colFrame then it's style context is the same as "col".  The parent check is an attempt to detect "we're reframing colFrame".  If we can detect it in some other way, great.  If not, I think we can just remove this entire ifdef DEBUG block.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #27)
> For this one, I'm going to punt to Bobby, actually.  The thing this is
> trying to assert is that if we're not reframing colFrame then it's style
> context is the same as "col".  The parent check is an attempt to detect
> "we're reframing colFrame".  If we can detect it in some other way, great. 
> If not, I think we can just remove this entire ifdef DEBUG block.

If not, perhaps we'd like to wrap all the code in DEBUG [1] in `if (mSource.IsGeckoRuleNode())` because it could still useful to verify them in gecko.

[1] http://searchfox.org/mozilla-central/rev/ee7cfd05d7d9f83b017dd54c2052a2ec19cbd48b/layout/tables/nsTableColGroupFrame.cpp#297-307
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #28)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #27)
> > For this one, I'm going to punt to Bobby, actually.  The thing this is
> > trying to assert is that if we're not reframing colFrame then it's style
> > context is the same as "col".  The parent check is an attempt to detect
> > "we're reframing colFrame".  If we can detect it in some other way, great. 
> > If not, I think we can just remove this entire ifdef DEBUG block.
> 
> If not, perhaps we'd like to wrap all the code in DEBUG [1] in `if
> (mSource.IsGeckoRuleNode())` because it could still useful to verify them in
> gecko.

Yes, let's do that.
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124158

> BAD NEWS: your stated assumption here (that StyleContext parent == parent frame's stylecontext) does *not* hold up in general. nsTableFrame/nsTableOuterFrame is one example where the frametree parentage is the opposite of the nsStyleContext parentage.
> 
> GOOD NEWS: When this assumption doesn't hold up, the parent-nsFrame is actually more likely to be correct than the parent-nsStyleContext.  This was clarified https://github.com/w3c/csswg-drafts/commit/d58f54955dd2528832be6ae83a50631c2e9fef12 .
> 
> So anyway -- you probably need to rewrite this extended-commit message in light of the above.

Sure. Rewritten the commit message according to this.

> Why do we need to change the function signature here?
> 
> If I understand correctly, we're just trying to remove callsites that currently do:
>  UsedAlignSelf(frame->StyleContext()->GetParent())
> 
> Can't we just fix those up to:
>  UsedAlignSelf(frame->GetParent()->StyleContext())
> and leave this function-signature intact? (still taking a nsStyleContext* which corresponds to the parent frame)
> 
> If so & if you leave the function signature untouched, I think a lot of this patch can be reverted -- i.e. a lot of this patch is unnecessary changes right now (the parts where we *are* the parent and we're just passing in "StyleContext()" directly with no explicit GetParent calls)

Besides fixing the callsites, those methods also call nsStyleContext::GetParent(). What's why I change the parameter into `nsIFrame*` so that those `GetParent()` calls inside them becomes `nsIFrame::GetParent()`. Per your commet 21, it's more likely to be correct :)
Please be very very careful with assuming that the frame tree has anything to do with either the style context tree Gecko has right now or the box tree in the spec.  It very often doesn't.

What do you know about the frames that come into this algorithm and their parents, exactly?
Flags: needinfo?(bzbarsky)
"this algorithm" being "UsedAlignSelf".  Whatever assumptions we're making here, we should add assertions for.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #38)
> > Why do we need to change the function signature here?
[...]
> > Can't we just fix those up to:
> >  UsedAlignSelf(frame->GetParent()->StyleContext())
> > and leave this function-signature intact? (still taking a nsStyleContext* which corresponds to the parent frame)
> 
> Besides fixing the callsites, those methods also call
> nsStyleContext::GetParent(). What's why I change the parameter into
> `nsIFrame*` so that those `GetParent()` calls inside them becomes
> `nsIFrame::GetParent()`. Per your commet 21, it's more likely to be correct

Two things:
 (1) UsedAlignSelf() does *not* call GetParent() internally, so that one does not need a function-signature change. (Though hypothetically if the others change, I can see an argument for changing that one as well for symmetry. But I'm still not sure we want any of them to change.)

 (2) However, you are correct that *some* of these functions do call GetParent() internally -- specificially the UsedJustify***() ones -- but these GetParent() calls are unrelated to the "more likely to be correct" thing that I was talking about in comment 21.  These internal GetParent calls are trying to look up the value of "justify-items" *that would have inherited to the parent, if the parent had inherit* [hence needing to inspect a grandparent], and for "inherit", the style-context tree is the Source Of Truth. This is to implement the following (somewhat crazy) spec text for "justify-items:auto" (which is distinct from justify-self:auto):
 # justify-items
 #   [...]
 #   'auto'
 #    * If the inherited value of 'justify-items' includes
 #      the 'legacy' keyword, 'auto' computes to the inherited value.
 #    * Otherwise, 'auto' computes to 'normal'.
https://drafts.csswg.org/css-align/#valdef-justify-items-auto
[Sorry, just noticed a confusing typo in Comment 42; hiding it & reposting for clarity]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #39)
> What do you know about the frames that come into this algorithm [UsedAlignSelf/UsedJustifySelf/UsedJustifyItems]
> and their parents, exactly?

Answering this question:
(a) Inside of nsStyleContext::UsedJustifyItems, if our representation of the computed value is "auto", then we need to know the hypothetically-inherited value of "justify-items" (the same property), because the spec says we actually COMPUTE to that hypothetically-inherited value in some cases. https://drafts.csswg.org/css-align/#valdef-justify-items-auto  (I use "hypothetically-inherited value" here to mean "the value we'd compute to if "inherit" were our specified value.)

(b) Inside of nsStyleContext::UsedAlignSelf, if our computed value is "auto", we need to know the parent box's "align-items" value (a different property), so we can "behave as" that value (though we still compute to "auto") https://drafts.csswg.org/css-align/#valdef-align-self-auto

(c) Inside of nsStyleContext::UsedJustifySelf, if our computed value is "auto", we need to know the parent box's "justify-items" value (a different property), with the "legacy" for that parent-value -- so we can "behave as" that value (though we still compute to "auto") https://drafts.csswg.org/css-align/#valdef-justify-self-auto

I think the spec was different when we implemented this, which is why it's so crazy right now.  I suspect we can & should handle part "(a)" in nsRuleNode.cpp when we actually generate the computed value -- and that will mean we don't need a UsedJustifyItems() function anymore.  We can't handle (b) and (c) there, though, because nsRuleNode.cpp doesn't know anything about box parentage.  But I think handling (a) would be sufficient to avoid needing to change the UsedAlignSelf/UsedJustifySelf function signatures, because we wouldn't need to care about lazy-computation hackery for the "legacy" value anymore...
Depends on: 1349326
(In reply to Daniel Holbert [:dholbert] from comment #43)
> (a) Inside of nsStyleContext::UsedJustifyItems, [...]
> I think the spec was different when we implemented this, which is why it's
> so crazy right now.  I suspect we can & should handle part "(a)" in
> nsRuleNode.cpp when we actually generate the computed value -- and that will
> mean we don't need a UsedJustifyItems()

Sorry, I meant "ComputedJustifyItems" here. In any case, I spun off bug 1322570 for fixing up "justify-items:auto" resolution. And with that fixed, I think that will mean that this bug here won't need to adjust any nsStyleStruct function signatures. (i.e. it'll make my hunch at the end of comment 21 correct).
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124590

Part 2 is still r- per comment 41 (particularly for the fact that the nsIFrame parentage is *not* "more likely to be correct" for justify-items:auto, as I noted there.)

Happily though, I think bug 1349326 will make most of this patch unnecessary.  Would you be up for taking that bug? (It's really just an alternate strategy for attacking this part of this bug, but I thought it'd be simpler to spin it off into a helper since it can be fixed/discussed independently from the rest of the changes here.)

::: commit-message-0865d:6
(Diff revision 2)
> +
> +In general, StyleContext's parent does not always equal to the parent
> +frame's StyleContext. Fortunately, per bug 1322570 comment 21, when this is
> +not hold up, getting the StyleContext() from the parent frame is more likely
> +to be correct than the parent StyleContext(). So we don't loose correctness

As noted above & in comment 41, the "more likely to be correct" is not true here -- that was only for the specific scenario of resolving *-self:auto from the parent-box's *-items value. And we can handle that by simply changing how we select the "nsStyleContext* aParent" that we pass into these Used***Self methods -- no need to adjust the methods themselves.

So this prose still needs some rewriting in the final version of this patch.

(Also, s/loose/lose/ -- though this sentence will probably disappear in the rewrite anyway.)
Attachment #8849088 - Flags: review?(dholbert) → review-
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124590

Drat, the simplification that I was hoping for in bug 1349326 is not actually possible.  (For more details, see the comment I left on that bug when closing it.)

So: we're stuck with the "ComputedJustifyItems(nsStyleContext* aParentContext)" lazy accessor for the time being, and *it* is stuck needing to call GetParent on its passed-in nsStyleContext, to recursively look up the hypothetically-inherited data, to resolve "auto" in case we have justify-items:auto all the way up to some distant ancestor.

And also, we're stuck with UsedJustifySelf needing to query its passed-in parent-box-style-context for its GetParent() (in the style context tree, not in the frame tree) so that it can correctly make a call to ComputedJustifyItems with the correct arg (in order to be able to look up the parent's hypothetically-inherited justify-items value).

So basically: I suspect there would be a correctness penalty if we changed the internals of *any* of these nsStyleStruct methods to avoid walking the style context tree for parent-determination. :-/

For the purposes of this bug, I suspect we should just replace ComputedJustifyItems & UsedJustifySelf's internal nsStyleContext::GetParent() calls with GetParentAllowServo() calls... (which you already do for ComputedJustifyItems), and leave nsStyleStruct untouched beyond that change.  That may not be a long-term fix, but it sounds like it might be sufficient for pushing this bug forward while still keeping these methods working the way they do now -- and we can sort out how to *actually* resolve the hypothetically-inherited value in cases where servo can't query for style context parents in a later bug...
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124980

r=me with nits

::: commit-message-0865d:1
(Diff revision 3)
> +Bug 1322570 Part 2 - Get StyleContext from parent frame in ReflowInput::InitConstraints().

Two clarifications:
 (1) s/Get/Resolve {align,justify}-self using/
 (2) s/parent frame/alignment container frame/  (per my ReflowInput.cpp nit below.)

::: commit-message-0865d:3
(Diff revision 3)
> +In general, StyleContext's parent does not always equal to the parent
> +frame's StyleContext. Per bug 1322570 comment 21 and comment 45, it only
> +applies for the specific scenario here.

Now that we won't necessarily be using the parent frame (per my ReflowInput.cpp nit below), this paragraph should perhaps just go away, or be tweaked a bit.

::: layout/generic/ReflowInput.cpp:2402
(Diff revision 3)
> -        auto inlineAxisAlignment = wm.IsOrthogonalTo(cbwm) ?
> -          mStylePosition->UsedAlignSelf(mFrame->StyleContext()->GetParent()) :
> -          mStylePosition->UsedJustifySelf(mFrame->StyleContext()->GetParent());
> +        auto inlineAxisAlignment =
> +          wm.IsOrthogonalTo(cbwm)
> +            ? mStylePosition->UsedAlignSelf(mFrame->GetParent()->StyleContext())
> +            : mStylePosition->UsedJustifySelf(
> +                mFrame->GetParent()->StyleContext());

Sorry, the contextual code (before this) reminds me that this is one place where we *don't always* want the parent frame. (The code above skips over it to the grandparent in one special case. We want the result of whatever skipping is done in the code above us -- i.e. we want to use "alignCB", which is the nsIFrame for our alignment container.)

SO: please use "alignCB" instead of mFrame->GetParent() here. (Twice -- in both UsedAlign*** calls.)

Otherwise I suspect this change would make us fail the reftest from bug 1334403, i.e. neglect to stretch tables when they're grid items, in some cases.
Attachment #8849088 - Flags: review?(dholbert) → review+
Comment on attachment 8849094 [details]
Bug 1322570 Part 8 - Run debug code only if the style source is a gecko rule node.

https://reviewboard.mozilla.org/r/121894/#review125040
Attachment #8849094 - Flags: review?(bobbyholley) → review+
Comment on attachment 8849921 [details]
Bug 1322570 Part 9 - Print StyleContext parents in frame tree dump only if they're gecko rule nodes.

https://reviewboard.mozilla.org/r/122670/#review125042
Attachment #8849921 - Flags: review?(bobbyholley) → review+
Comment on attachment 8849088 [details]
Bug 1322570 Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints().

https://reviewboard.mozilla.org/r/121882/#review124980

> Sorry, the contextual code (before this) reminds me that this is one place where we *don't always* want the parent frame. (The code above skips over it to the grandparent in one special case. We want the result of whatever skipping is done in the code above us -- i.e. we want to use "alignCB", which is the nsIFrame for our alignment container.)
> 
> SO: please use "alignCB" instead of mFrame->GetParent() here. (Twice -- in both UsedAlign*** calls.)
> 
> Otherwise I suspect this change would make us fail the reftest from bug 1334403, i.e. neglect to stretch tables when they're grid items, in some cases.

OK. I'll change it to `alignCB`. I notice that the reftest added in bug 1334403 [1] incorrectly wrote test page as ref page. Perhaps that's why the try run in comment 56 didn't fail on this. I'll fix it in this patch.

[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/layout/reftests/flexbox/reftest.list#118
(In reply to Daniel Holbert [:dholbert] from comment #57)
> SO: please use "alignCB" instead of mFrame->GetParent() here. (Twice -- in
> both UsedAlign*** calls.)
> 
> Otherwise I suspect this change would make us fail the reftest from bug
> 1334403, i.e. neglect to stretch tables when they're grid items, in some
> cases.

Oh wait, use "alignCB" fails layout/reftests/css-grid/grid-item-table-stretch-002.html

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88ce6392acf1a4087ca34c7c4a4086e8990bcbe&selectedJob=85825987

The code calling UsedAlignSelf() and UsedJustifySelf() was conditioned on alignCBType of a grid container, not a flex container, which is unlikely to fails reftest in bug 1334403.  Perhaps we still want to use mFrame->GetParent() instead of alignCB?
Flags: needinfo?(dholbert)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #60)
> OK. I'll change it to `alignCB`. I notice that the reftest added in bug
> 1334403 [1] incorrectly wrote test page as ref page. Perhaps that's why the
> try run in comment 56 didn't fail on this. I'll fix it in this patch.

Oops, thanks for catching that!  Assuming that test actually passes on current trunk, feel free to push that tweak directly to inbound as a standalone "bug 1334403 followup". (rs=me on that if you choose to do that)  Or it's fine lumped into this bug, too.

> alignCBType of a grid container, not a flex container,
> which is unlikely to fails reftest in bug 1334403.

Right, sorry - I got my bugs mixed up. I meant to say that I thought GetParent() would break the reftest *for the grid version of that codepath*.  (whatever touched that code *just before* bug 1334403 touched it, I think.)

> Oh wait, use "alignCB" fails layout/reftests/css-grid/grid-item-table-stretch-002.html

Right, this is the sort of test I was thinking of. Though I'm surprised that it breaks with the alignCB version of the fix!
From a quick glance at the reftest snapshot from the try run, it looks like the *reference* case is broken by the patch -- not the testcase.

This doesn't make sense to me... I'll take a look in a debugger in the morning and see what I can figure out.
I think this is indicative of a subtle preexisting bug in our handling of <caption> within a table within a grid.

I think it'll work if you add this to the reference case layout/reftests/css-grid/grid-item-table-stretch-002-ref.html :
> caption { justify-self: stretch }

That shouldn't be necessary, but it is right now due to this subtle bug. I filed bug 1350037 for that issue (which we should fix soon but doesn't necessarily need to block this bug, IMO), and we should reference that followup bug in the caption {} CSS comment that ^^ caption CSS hackaround.  (so that we remove the hackaround when bug 1350037 is fixed)
The misrendering file (grid-item-table-stretch-002-ref.html) is a fancier version of the third testcase that I've just posted over on bug 1350037 (direct link: attachment 8850675 [details]).  But my other two testcases there show that this is just a version of a more general problem that we already have.

(TYLin: if you're interested, I've got an idea for a fix strategy over on that bug -- just test ReflowInput::mFrame for being a table frame, rather than testing its GetParent() for being a table-wrapper frame.  Ideally it'd be great to fix that bug before fixing this one, to avoid introducing a regression here -- but if there's pressure to get this bug here landed soon, I'm also OK with the hackaround noted in comment 63 for the time being, as long as it comes with a code-comment pointing to bug 1350037.)
Flags: needinfo?(dholbert)
Daniel, thanks for the investigation.

I added `caption { justify-self: stretch }` in layout/reftests/css-grid/grid-item-table-stretch-002-ref.html in Part 2 with a comment pointing to bug 1350037, and the try results look good, so let's and this bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a8ae2fb3faf634901fb65b8dfd4ca0c99749c46
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e845604ca67d
Part 1 - Add MOZ_ASSERT in nsStyleContext::GetParent() to disallow usage by stylo. r=bz
https://hg.mozilla.org/integration/autoland/rev/67c909cb9dab
Part 2 - Resolve {align,justify}-self using StyleContext from alignment container frame in ReflowInput::InitConstraints(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/5ab4d717fa73
Part 3 - Get StyleContext from parent frame in nsFlexContainerFrame::Init(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d3779f116482
Part 4 - Use GetParentAllowServo() in KeyframeEffectReadOnly::UpdateProperties. r=hiro
https://hg.mozilla.org/integration/autoland/rev/e520178e739b
Part 5 - Use GetParentAllowServo() related to first letter frame. r=bz
https://hg.mozilla.org/integration/autoland/rev/70e7583ef542
Part 6 - Use GetParentAllowServo() in RestyleManager. r=bz
https://hg.mozilla.org/integration/autoland/rev/5dad7cc3ef39
Part 7 - Use GetParentAllowServo() in nsMathMLChar. r=bz
https://hg.mozilla.org/integration/autoland/rev/77ccceb898f8
Part 8 - Run debug code only if the style source is a gecko rule node. r=bholley
https://hg.mozilla.org/integration/autoland/rev/7c770e01c8fc
Part 9 - Print StyleContext parents in frame tree dump only if they're gecko rule nodes. r=bholley
Attachment #8850858 - Attachment is obsolete: true
Depends on: 1351205
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: