Closed Bug 847130 Opened 7 years ago Closed 7 years ago
"ASSERTION: value should always be stored and non-empty when state set" with nested -moz-column, <td>
###!!! 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)
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
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
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
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] fix+test 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: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbd4cef7fde
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
Attachment #720465 - Flags: approval-mozilla-aurora?
Note, this cset should be backed out when landing if approval is granted. https://hg.mozilla.org/releases/mozilla-aurora/rev/133095cc43fa
Attachment #720465 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 720465 [details] [diff] [review] fix+test [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] fix+test Helps fix intermittent orange due to backout of patch bug 810726 in beta
Attachment #720465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/41a072aa81e5
You need to log in before you can comment on or make changes to this bug.