Cameron McCormack (:heycam) Reporter
	Description • 8 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

	Cameron McCormack (:heycam) Reporter
	Comment 1 • 8 years ago

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

	Cameron McCormack (:heycam) Reporter
	Comment 2 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 3 • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 8 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 9 • 8 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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 8 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.

	Emilio Cobos Álvarez (:emilio)
	Comment 15 • 8 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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 8 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.

	Emilio Cobos Álvarez (:emilio)
	Comment 19 • 8 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 :)

	Bobby Holley (:bholley)
	Comment 20 • 8 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

	Emilio Cobos Álvarez (:emilio)
	Comment 21 • 8 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 :)

	Bobby Holley (:bholley)
	Comment 22 • 8 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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 8 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.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 8 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.

	Pulsebot
	Comment 29 • 8 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

	Pulsebot
	Comment 30 • 8 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

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 8 years ago

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

	Bobby Holley (:bholley)
	Comment 32 • 8 years ago

(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.

	Pulsebot
	Comment 33 • 8 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

	Xidorn Quan [:xidorn] UTC+11
	Comment 34 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 35 • 8 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