Closed Bug 847130 Opened 11 years ago Closed 11 years ago

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


(Core :: Layout, defect)

Not set



Tracking Status
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed


(Reporter: jruderman, Assigned: MatsPalmgren_bugz)



(Keywords: assertion, testcase)


(3 files)

Attached file 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)
Attached file stack
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.)
Depends on: 810726
Blocks: 810726
No longer depends on: 810726
The invariant that is asserted is valid and I don't want to relax it.
We should just fix the code that violates it...
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch fix+testSplinter Review
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.
Attachment #720465 - Flags: review?(bzbarsky)
(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 on attachment 720465 [details] [diff] [review]

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

r=dbaron with that
Attachment #720465 - Flags: review?(bzbarsky) → review+
(I suppose in comment 2 I was using "valid" to mean "correctly maintained".  Sorry.)
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).
With the suggested nullptr assignments:
Flags: in-testsuite+
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment on attachment 720465 [details] [diff] [review]

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
Attachment #720465 - Flags: approval-mozilla-aurora?
Note, this cset should be backed out when landing if approval is granted.
Attachment #720465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 720465 [details] [diff] [review]

[Approval Request Comment]
Same as Aurora, we're hitting these on Beta too.
Attachment #720465 - Flags: approval-mozilla-beta?
Comment on attachment 720465 [details] [diff] [review]

Helps fix intermittent orange due to backout of patch bug 810726 in beta
Attachment #720465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.