All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
###!!! 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
(Reporter)

Comment 1

2 years ago
This is fixed in the latest mozilla-central -> stylo merge.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Reporter)

Comment 2

2 years ago
This didn't get fixed after all (see bug 1334938); sorry for the noise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Blocks: 1337615
(Assignee)

Comment 4

2 years ago
Created attachment 8838198 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → bzbarsky
Status: REOPENED → ASSIGNED
(Assignee)

Updated

2 years ago
Blocks: 1337700
(Assignee)

Updated

2 years ago
Blocks: 1337703
(Assignee)

Updated

2 years ago
Blocks: 1292609
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review-reply
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?
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
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 :)
Attachment #8838615 - Flags: review+

Comment 20

2 years ago
mozreview-review
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
Attachment #8838615 - Flags: review?(bobbyholley) → review+

Comment 21

2 years ago
mozreview-review
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 :)
Attachment #8838616 - Flags: review+

Comment 22

2 years ago
mozreview-review
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
Attachment #8838616 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 23

2 years ago
mozreview-review-reply
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.  ;)
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Created attachment 8838702 [details] [diff] [review]
Part 1 merge on top of autoland, since mozreview won't let me push this becuase it's dumb
(Assignee)

Updated

2 years ago
Attachment #8838198 - Attachment is obsolete: true
Created attachment 8838703 [details] [diff] [review]
Part 2 merged on top of autoland

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
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

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abee64d2e5ef
followup: check our child hint before trying to append it.  r=bholley
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
(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

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ef6204c398
another followup: just simplify the chnagehint thing for now, with a followup filed.  r=bholley
Weird that this doesn't trigger any assertion count mismatch... I'll need to figure out why it doesn't take effect...

Comment 35

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1341052
You need to log in before you can comment on or make changes to this bug.