Closed
Bug 847130
Opened 12 years ago
Closed 12 years ago
"ASSERTION: value should always be stored and non-empty when state set" with nested -moz-column, <td>
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
389 bytes,
application/xhtml+xml
|
Details | |
15.67 KB,
text/plain
|
Details | |
4.05 KB,
patch
|
dbaron
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
###!!! 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)
Reporter | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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.)
Assignee | ||
Comment 8•12 years ago
|
||
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).
Assignee | ||
Comment 9•12 years ago
|
||
With the suggested nullptr assignments:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbd4cef7fde
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
Comment 11•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 12•12 years ago
|
||
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?
Comment 13•12 years ago
|
||
Note, this cset should be backed out when landing if approval is granted.
https://hg.mozilla.org/releases/mozilla-aurora/rev/133095cc43fa
Updated•12 years ago
|
Attachment #720465 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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+
Comment 16•12 years ago
|
||
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/41a072aa81e5
Updated•12 years ago
|
status-firefox19:
--- → affected
status-firefox20:
--- → fixed
Comment 17•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•