1324661 - stylo: several tests non-fatally assert with "Table wrapper frames cannot have borders or paddings"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

###!!! ASSERTION: Table wrapper frames cannot have borders or paddings: 'aBorder.IsAllZero() && aPadding.IsAllZero()', file /home/worker/workspace/build/src/layout/tables/nsTableWrapperFrame.cpp, line 436
###!!! ASSERTION: Wrong parent style context: 'Error', file /home/worker/workspace/build/src/layout/base/RestyleManagerBase.cpp, line 302

  gfx/tests/crashtests/783041-1.html
  layout/base/crashtests/405186-1.xhtml
  layout/generic/crashtests/1281102.html

Comment 1

[Cameron McCormack (:heycam)]

This is fixed in the latest mozilla-central -> stylo merge.

Comment 2

[Cameron McCormack (:heycam)]

This didn't get fixed after all (see bug 1334938); sorry for the noise.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

layout/style/test/test_animations_async_tests.html
layout/style/test/test_animations_effect_timing_enddelay.html
layout/style/test/test_animations_event_order.html
layout/style/test/test_animations_omta_start.html
layout/style/test/test_animations_playbackrate.html
layout/style/test/test_at_rule_parse_serialize.html
layout/style/test/test_unclosed_parentheses.html

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables

This breaks this testcase:

  <table style="position: absolute; top: 200px; left: 0px; border: 10px solid black">
    <tr><td onclick="document.querySelector('table').style.top = '400px'">Hey</td></tr>
  </table>

in terms of the rendering (while fixing the assertion on it), due to bug 1292609
and the table-outer not getting its updated "top" value...  Going to poke at
that a bit.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1324661 part 1. When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

For a table, the primary frame is the table wrapper anonymous box.  That
anonymous box inherits style from its _child_ table frame, which is the frame that
has the actual style for the element.  So we want to use the stylo-computed
style for the table frame, and then compute an updated style for the table
wrapper too, because some things (like absolute positioning) are done for the
table wrapper anonymous box, not the table frame.

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1324661 part 2. Reenable various table reftests that were disabled before. They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review114984

::: layout/base/ServoRestyleManager.cpp:136
(Diff revision 1)
> +static void
> +UpdateStyleContextForTableWrapper(nsIFrame* aTableWrapper,
> +                                  nsStyleContext* aTableStyleContext,
> +                                  ServoStyleSet* aStyleSet)
> +{
> +  // XXXbz Do we need to add any change hints here?

Presumably it may be enough to pass this frame instead of the style frame to the frame change list, since I believe the style changes will be only inherited, right?

Though I don't know if the table frame has some magic on the functions that RestyleManager::ProcessRestyledFrames have to look and update the wrapper frame too?

::: layout/base/ServoRestyleManager.cpp:146
(Diff revision 1)
> +    aStyleSet->ResolveAnonymousBoxStyle(nsCSSAnonBoxes::tableWrapper,
> +                                        aTableStyleContext,
> +                                        0 /* Are these the right flags? */);
> +  aTableWrapper->SetStyleContext(newContext);
> +}
> +                                  

nit: trailing whitespace.

::: layout/base/ServoRestyleManager.cpp:157
(Diff revision 1)
>                                             ServoStyleSet* aStyleSet,
>                                             nsStyleChangeList& aChangeListToProcess)
>  {
> -  nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
> +  nsIFrame* styleFrame;
> +  if (nsIFrame* primaryFrame = aElement->GetPrimaryFrame()) {
> +    styleFrame = nsLayoutUtils::GetStyleFrame(primaryFrame);

Given we're doing this, we could also just use nsLayoutUtils::GetStyleFrame(aElement), right?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review114984

> Presumably it may be enough to pass this frame instead of the style frame to the frame change list, since I believe the style changes will be only inherited, right?
> 
> Though I don't know if the table frame has some magic on the functions that RestyleManager::ProcessRestyledFrames have to look and update the wrapper frame too?

OK, now that I'm more awake I read through the Gecko code here again.  What Gecko does when it detects this case is it restyles the _child_ (that is the table frame).  Then it makes sure that any hints handled for that child frame are applied to the table wrapper as well; that's the "assumeDifferenceHint" bit in ElementRestyler::RestyleSelf.  At least I think that's how it works.

So in terms of our stuff here, I guess we can just add _both_ frames to the change list with the "changeHint" that Servo_TakeChangeHint returned?

> nit: trailing whitespace.

Yes, fixed in the most recent version.

> Given we're doing this, we could also just use nsLayoutUtils::GetStyleFrame(aElement), right?

Oh, good catch.  Done.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review114984

> OK, now that I'm more awake I read through the Gecko code here again.  What Gecko does when it detects this case is it restyles the _child_ (that is the table frame).  Then it makes sure that any hints handled for that child frame are applied to the table wrapper as well; that's the "assumeDifferenceHint" bit in ElementRestyler::RestyleSelf.  At least I think that's how it works.
> 
> So in terms of our stuff here, I guess we can just add _both_ frames to the change list with the "changeHint" that Servo_TakeChangeHint returned?

Ok, yeah, I think that'd work, though presumably Gecko's restyle manager also takes care of the "handled for descendants" bits, right? (so we don't repaint or reflow twice, etc).

I think we should push the parent with the hint that `Servo_TakeChangeHint` returns, and the child with `NS_HintsNotHandledForDescendants(thatHint)`, what do you think?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review114984

> Ok, yeah, I think that'd work, though presumably Gecko's restyle manager also takes care of the "handled for descendants" bits, right? (so we don't repaint or reflow twice, etc).
> 
> I think we should push the parent with the hint that `Servo_TakeChangeHint` returns, and the child with `NS_HintsNotHandledForDescendants(thatHint)`, what do you think?

Yes, that makes sense.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review115046

::: layout/base/ServoRestyleManager.cpp:160
(Diff revision 5)
>    // Although we shouldn't generate non-ReconstructFrame hints for elements with
>    // no frames, we can still get them here if they were explicitly posted by
>    // PostRestyleEvent, such as a RepaintFrame hint when a :link changes to be
>    // :visited.  Skip processing these hints if there is no frame.
> -  if ((primaryFrame || changeHint & nsChangeHint_ReconstructFrame) && changeHint) {
> -    aChangeListToProcess.AppendChange(primaryFrame, aElement, changeHint);
> +  if ((styleFrame || (changeHint & nsChangeHint_ReconstructFrame)) && changeHint) {
> +    if (isTable) {

Probably `MOZ_UNLIKELY`? But not a big deal.

::: layout/base/ServoRestyleManager.cpp:226
(Diff revision 5)
> -    for (nsIFrame* f = primaryFrame; f;
> +    for (nsIFrame* f = styleFrame; f;
>           f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
>        f->SetStyleContext(newContext);
>      }
>  
> +    MOZ_ASSERT(styleFrame,

I'd move this assert down given I'm writing a patch for display: contents right now. Though I can also remove it myself when rebasing, I guess :)

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review115030

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

https://reviewboard.mozilla.org/r/113462/#review115050

nice :)

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

https://reviewboard.mozilla.org/r/113462/#review115052

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review115046

> Probably `MOZ_UNLIKELY`? But not a big deal.

I don't think it's worth it, esp because on some pages it's not too unlikely.  ;)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

https://reviewboard.mozilla.org/r/113460/#review115046

> I'd move this assert down given I'm writing a patch for display: contents right now. Though I can also remove it myself when rebasing, I guess :)

Good catch, fixed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838615 [details]
Bug 1324661 part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838616 [details]
Bug 1324661 part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merge on top of autoland, since mozreview won't let me push this becuase it's dumb

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged on top of autoland

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a300522006bc
part 1.  When recreating style contexts for elements in stylo, use the right frame (not the primary frame!) for tables.  r=bholley,emilio
https://hg.mozilla.org/integration/autoland/rev/c69baa105d70
part 2.  Reenable various table reftests that were disabled before.  They all seem to be passing, or at least failing for unrelated reasons, with the part 1 patch applied.  r=bholley,emilio

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abee64d2e5ef
followup: check our child hint before trying to append it.  r=bholley

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

You would also need to remove the corresponding failure patterns here: https://hg.mozilla.org/mozilla-central/file/92a932f9be3f/layout/style/test/stylo-failures.md#l584

Comment 32

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #31)
> You would also need to remove the corresponding failure patterns here:
> https://hg.mozilla.org/mozilla-central/file/92a932f9be3f/layout/style/test/
> stylo-failures.md#l584

Assuming they turn the job orange as unexpected passes, I'll do this in an hour or two when the results come in after the merge and I update test expectations.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67ef6204c398
another followup: just simplify the chnagehint thing for now, with a followup filed.  r=bholley

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Weird that this doesn't trigger any assertion count mismatch... I'll need to figure out why it doesn't take effect...

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a300522006bc
https://hg.mozilla.org/mozilla-central/rev/c69baa105d70
https://hg.mozilla.org/mozilla-central/rev/abee64d2e5ef
https://hg.mozilla.org/mozilla-central/rev/67ef6204c398