374297 - [FIX]"Wrong parent style context" involving table pseudo frames

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

Attachment: An example frame tree, with corresponding error message.

"Wrong parent style context" involving table pseudo frames

I don't know if it's the VerifyContextParent() code
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsFrameManager.cpp&rev=1.244&root=/cvsroot&mark=805#804
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrame.cpp&rev=3.712&root=/cvsroot&mark=5479-5485#5461

that is wrong or if it's the frame constructor that needs to resolve the style
using GetParentStyleContextFrame() rather than GetParent(), or something else...
What is the correct style context tree for these frames?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Mats, do you have a testcase that gives you that frame tree?  I wonder whether bug 323656 helps here.

Comment 2

[Mats Palmgren (inactive)]

The testcases in bug 307992 and other tests of that nature does this a lot.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, the patch in bug 323656 fixes the ones in that bug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Now that bug bug 323656 is fixed, is this still a problem?

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase

frame: Table(table)(0) (0x165e9a8) style: 0x165e868 {}
Wrong parent style context:  style: 0x165a408 {}
should be using:  style: 0x165a6c8 :-moz-cell-content {}

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.  The patch for bug 323656 missed a case.  Filed bug 377603 on that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  That should be better, I hope!  Any other cases?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #2

frame: TableCell(th)(0) (0x157cc48) style: 0x157ca98 {}
Wrong parent style context:  style: 0x157c5b0 {}
should be using:  style: 0x1581228 {}

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, that case is interesting.

We resolve style on the <table> first.  This recurses down to the table cell, we resolve style on that, discover that it needs a reframe, do NOT set the new style context on it (because we need the old one to properly tear down the frame) and bail out.

Then we process our restyled frames (of which we have two: te table and the th).  The table is processed first, so we mark it as needing reflow, then verify the style tree.  At this point the table cell still has the "wrong" (old) style context.

Maybe we should just verify the style tree starting at the "rootmost" of the frames we process in one processing loop?

In any case, that all doesn't depend on table pseudos; there aren't any table pseudos in this case, in fact.

Comment 10

[Jesse Ruderman]

This seems to be the last known "Wrong parent style context" bug.  Once it's fixed, I can easily determine whether there are other ways to hit "Wrong parent style context" :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

This is actually a case of the warning being wrong, basically... We're verifying an invariant at a time when there's no reason it should hold.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

This is the last case I know of where we get these warnings incorrectly, so I turned them into asserts.  This patch fixes this bug and bug 382204.

The asserts will fire on the testcase in bug 396645, but that's the only case I know of where they will fire.

Comment 13

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

Comment on attachment 281549 [details] [diff] [review]
Like so

r+sr+a1.9=dbaron.  Sorry for the delay.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.

Would be nice to add the testcases (which would assert if they were still failing, now) to whatever bug 397725 decides on.

Comment 15

[Jesse Ruderman]

Crashtest checked in.  "Wrong parent style context" is now an assertion, so once assertions turn Tinderbox orange, this will be properly regression-tested.