847130 - "ASSERTION: value should always be stored and non-empty when state set" with nested -moz-column, <td>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: value should always be stored and non-empty when state set: 'prop && !prop->mLines.empty() && prop->mLines.front()->GetChildCount() == 0 ? prop->mFrames.IsEmpty() : prop->mLines.front()->mFirstChild == prop->mFrames.FirstChild()', file /Users/jruderman/trees/mozilla-central/layout/generic/nsBlockFrame.cpp, line 4545

(Previous bug on this assertion: bug 840818)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[David Baron :dbaron:]

Scott and I decided this assertion isn't quite valid; the latest patch in bug 810726 modifies it.  (In particular, in that case, the first line was completely empty, but there were later, non-empty lines.)

Comment 3

[Mats Palmgren (inactive)]

The invariant that is asserted is valid and I don't want to relax it.
We should just fix the code that violates it...

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix+test

Do DestroyOverflowLines() if the last line was removed instead of always RemoveOverflowLines() and SetOverflowLines() when there's lines remaining.
(similar to the change in bug 840818)

I don't see any further occurrences of this code pattern.

https://tbpl.mozilla.org/?tree=Try&rev=2aac7ccffa5e

Comment 5

[David Baron :dbaron:]

(In reply to Mats Palmgren [:mats] from comment #3)
> The invariant that is asserted is valid

An invariant isn't valid or invalid as a matter of absolute truth.  We choose to maintain invariants because they make things easier; we shouldn't maintain invariants when they make things harder.

> and I don't want to relax it.

That is a reasonable argument, particularly given that this patch simplifies the code.



For the record, in the case Scott was debugging, the condition that was failing was the final one:
  prop->mLines.front()->GetChildCount() == 0 ? prop->mFrames.IsEmpty() :
    prop->mLines.front()->mFirstChild == prop->mFrames.FirstChild()
which was failing because prop->mLines had more than one line but the first was empty, so the code took the prop->mFrames.IsEmpty() assertion rather than the other assertion.  This was happening because the assertion was firing *between* when we pulled a child (a block, I think) from the first line and when we deleted the line containing it.  If memory serves, this was in the RemoveOverflowLines call in nsBlockFrame::DoRemoveFrame where the assertion was firing on the first of these lines but the line we cared about wasn't being removed until the second:
        FrameLines* overflowLines = RemoveOverflowLines();
        line = overflowLines->mLines.erase(line);

Your patch here does fix that to avoid the RemoveOverflowLines/SetOverflowLines pair, and thus I think does fix the assertion in the case Scott was looking at.

Comment 6

[David Baron :dbaron:]

Comment on attachment 720465 [details] [diff] [review]
fix+test

Please set |overflowLines| to nullptr right after calling DestroyOverflowLines in both cases (DoRemoveFrame and StealFrame).

r=dbaron with that

Comment 7

[David Baron :dbaron:]

(I suppose in comment 2 I was using "valid" to mean "correctly maintained".  Sorry.)

Comment 8

[Mats Palmgren (inactive)]

No problem, I just used the term "valid" because you claimed it wasn't.
What I mean is that I find the current invariant useful because it catches
more mistakes, without making writing correct code harder (I hope).

Comment 9

[Mats Palmgren (inactive)]

With the suggested nullptr assignments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbd4cef7fde

Comment 10

[David Baron :dbaron:]

Yeah, I think the logic in my head (which is certainly one of many ways of using terminology) was that:

 * invariants are useful or not depending on whether they help us overall (balancing ease of writing code, security, correctness, etc.)

 * assertions are valid or not depending on whether the invariant they assert is actually true

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3cbd4cef7fde

Comment 12

[Scott Johnson (:jwir3)]

Comment on attachment 720465 [details] [diff] [review]
fix+test

We need this on aurora for bug 810726, since otherwise we will have oranges for the test in bug 810726.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We'll have assertions in test cases on aurora in the test for bug 810726.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Minimal, I believe. 
String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Note, this cset should be backed out when landing if approval is granted.
https://hg.mozilla.org/releases/mozilla-aurora/rev/133095cc43fa

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 720465 [details] [diff] [review]
fix+test

[Approval Request Comment]
Same as Aurora, we're hitting these on Beta too.

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 720465 [details] [diff] [review]
fix+test

Helps fix intermittent orange due to backout of patch bug 810726 in beta

Comment 16

[Scott Johnson (:jwir3)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/41a072aa81e5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a932e85257c3