1142531 - nsStyleContext::MoveTo doesn't recompute flags in mBits when it should

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Discovered while writing a patch for bug 962594, see comments 60-64 for backstory.

Basically the nsStyleContext::mBits field is computed in ApplyStyleFixups and requires walking up the parent chain in order figure out some of the flags that need setting. Therefore the flags in mBits is dependent on the ancestor chain of the nsStyleContext. The MoveTo function reparents the style context, and consequently invalidates some/all of mBits. The mBits therefore need to be recomputed, but are not. In bug 962594 comment 58 :heycam said that the MoveTo function should not be getting called if the bits differ; checks to prevent this exist in ElementRestyler::ComputeRestyleResultFromNewContext. However, when I added an assert in MoveTo to assert this, the assertion caused test failures. Therefore, we should either recompute mBits in MoveTo, or investigate/fix the test failures and add the assert in MoveTo.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I agree that we probably want the assertion in MoveTo, and then need to figure out how to make it not fire.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I debugged the assertion failure in the web-platform-test editing/conformancetest/test_runtest.html .

It looks (although I didn't check all that carefully) like what's happening is that we're returning eRestyleResult_Stop (i.e., leave the old context) for an element that has text-decoration specified.  The element's old parent did not have NS_STYLE_HAS_TEXT_DECORATION_LINES, but the new parent does.  (However, both old and new child have it!)

I need to think about whether this is a problem..

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e86a980983&group_state=expanded

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll post the patch for review once heycam is back, since there's no rush.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1142531: Check more bits in nsStyleContext::MoveTo assertion. r?heycam

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8730063 [details]
MozReview Request: Bug 1142531:  Check more bits in nsStyleContext::MoveTo assertion.  r?heycam

https://reviewboard.mozilla.org/r/39647/#review36307

::: layout/style/nsStyleContext.h:475
(Diff revision 1)
> +  bool ContextHasTextDecorationLine() {
> +    const nsStyleTextReset* text = StyleTextReset();
> +    uint8_t decorationLine = text->mTextDecorationLine;
> +    return decorationLine != NS_STYLE_TEXT_DECORATION_LINE_NONE &&
> +           decorationLine != NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL;
> +  }

I think this would be better as a method on nsStyleTextReset instead, since we typically put convenience methods for checking property values on the structs themselves rather than on nsStyleContext.

::: layout/style/nsStyleContext.cpp:321
(Diff revision 1)
> +  MOZ_ASSERT((mParent->mBits & NS_STYLE_HAS_TEXT_DECORATION_LINES) ==
> +             (aNewParent->mBits & NS_STYLE_HAS_TEXT_DECORATION_LINES) ||
> +             ContextHasTextDecorationLine());

This check looks right to me, but we should never currently get called if the parent NS_STYLE_HAS_TEXT_DECORATION_LINES bits differ, since we check that in ElementRestyler::ComputeRestyleResultFromNewContext, right?

Should we be loosening that check in ElementRestyler so that we can stop restyling if the new parent's bit differs, but the current newly resolved style context has the bit set anyway?  (In a followup bug.)

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8730063 [details]
MozReview Request: Bug 1142531:  Check more bits in nsStyleContext::MoveTo assertion.  r?heycam

https://reviewboard.mozilla.org/r/39647/#review36309

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) (busy Mar 14–18) from comment #6)
=> ::: layout/style/nsStyleContext.cpp:321
> (Diff revision 1)
> > +  MOZ_ASSERT((mParent->mBits & NS_STYLE_HAS_TEXT_DECORATION_LINES) ==
> > +             (aNewParent->mBits & NS_STYLE_HAS_TEXT_DECORATION_LINES) ||
> > +             ContextHasTextDecorationLine());
> 
> This check looks right to me, but we should never currently get called if
> the parent NS_STYLE_HAS_TEXT_DECORATION_LINES bits differ, since we check
> that in ElementRestyler::ComputeRestyleResultFromNewContext, right?

No, this currently happens, since ComputeRestyleResultFromNewContext checks oldContext->HasTextDecorationLines() != aNewContext->HasTextDecorationLines(), which can be true even when the bits on the old and new parents differ.  See comment 2.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0fa0d3838807de31e2d4dbb1deb6cfd981bcb82
Bug 1142531:  Check more bits in nsStyleContext::MoveTo assertion.  r=heycam

Comment 10

[Carsten Book [:Tomcat]]

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