Closed
Bug 460637
Opened 16 years ago
Closed 13 years ago
[BC] ASSERTION: invalid BC damage area: 'PR_FALSE'
Categories
(Core :: Layout: Tables, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: MatsPalmgren_bugz, Assigned: bernd_mozilla)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 8 obsolete files)
Followup from bug 284852 (also bug 315549 is probably related): ------- Comment #3 From Mats Palmgren 2005-03-05 15:41:25 PDT Testcase #1 (attachment 176407 [details]) Here are three different examples that leads to an assertion. A bit of debug output on the first example at the top: Clicking 5 times on the button... Document file:///home/mats/work/bug284852/284852.html loaded successfully ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2 aRowSpanIsZero aMap.GetColCount()=1 endColIndex=0 SetDamageArea x=0 y=3 width=1 height=-2 ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2 aRowSpanIsZero aMap.GetColCount()=2 endColIndex=0 SetDamageArea x=0 y=3 width=1 height=-1 ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2 aRowSpanIsZero aMap.GetColCount()=3 endColIndex=0 SetDamageArea x=0 y=3 width=1 height=0 ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2 aRowSpanIsZero aMap.GetColCount()=4 endColIndex=0 SetDamageArea x=0 y=3 width=1 height=1 ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2 aRowSpanIsZero aMap.GetColCount()=5 endColIndex=0 SetDamageArea x=0 y=3 width=1 height=2 ###!!! ASSERTION: invalid BC damage area: 'PR_FALSE', file nsTableFrame.cpp, line 4569 Break: at file nsTableFrame.cpp, line 4569 The damage area setup is wrong... the root of the problem IMO is the 'aRowSpan' value (2) which is the result of: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=3.91&root=/cvsroot&mark=1752,1759,1760#1750 The problem also occurs for aRowSpanIsZero is false though - see the second example. The third is also a zero rowspan case, although it triggers the assertion at a different place, nsTableFrame line 982: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.610&root=/cvsroot&mark=967,982#966
Reporter | ||
Comment 1•16 years ago
|
||
Note that the Bonsai links above refers to old CVS revisions - the code might have changed since then. I have landed the testcase from bug 284852 as a crash test. It triggers: ###!!! ASSERTION: invalid BC damage area: 'PR_FALSE', file /usr/moz/hg3/layout/tables/nsTableFrame.cpp, line 4078
Reporter | ||
Comment 2•16 years ago
|
||
FWIW, this is what I suggested in bug 284852. It fixes the first and third table in the testcase, but the 2nd still asserts. (it used to fix all three according to bug 284852 comment 5)
Reporter | ||
Comment 3•16 years ago
|
||
I've found a major problem with how the BC damage area is setup. The nsTableCellMap Insert/Remove/Append methods calls into the specific nsCellMap method and expect a damage rect that is in nsCellMap coordinates, it then adds the start row index for the row group to that. The problem is that nsCellMap sometimes decides to "rebuild everything" and then the returned damage is in global table coordinates [1] so adding to that creates a bogus damage rect. (I fixed it by using a 'mDidRebuild' flag and skipping the row group adjustment when it's true.) I've also fixed a few other minor problems and added numerous assertions for the damage rect setup. This patch passes all regression tests (without triggering any of the added assertions). Note that the new assertions are stricter then the old one and that I've also removed the old "error correction" code (CheckFixDamageArea). [1] http://hg.mozilla.org/mozilla-central/file/0e117ce1b610/layout/tables/nsCellMap.cpp?mark=763,790#l740
Attachment #343788 -
Attachment is obsolete: true
Attachment #343980 -
Flags: review?(bernd_mozilla)
Reporter | ||
Updated•16 years ago
|
Are you ready for the ultimate test for this? There is one bug that kept me busy for two years.... bug 339128. Does your patch silence the 'ASSERTION: invalid BC damage area' there? The reason to ask for this is, that reftests are blind to this. However this is not a requirement just curiosity.
Mats you write in this patch: "XXX fix nsCellMap::InsertRows() instead" Why aren't you doing just this? Isn't the patch wallpaper? Having a cellmap:: settting the damage area without asking for startrowindex but setting a 0 seems obviously wrong.
That said, I fully agree with your analysis, this is certainly broken.
Reporter | ||
Comment 7•16 years ago
|
||
After running stress tests for a few days I think this is a better patch for now. First, there was a problem in AddBCDamageArea() when multiple mutations occurred before the table got a chance to reflow - the stored damage area can be larger than the current table size and that would trigger an assertion. I've added code that clamps that area and not assert about it since it's not really an error. Secondly, I'm leaving GetRowSpanForNewCell() as is for now. I still think the 2 should be 1 there, but the complexities involved in making the code not depend on this error goes beyond my knowledge of the table code. I can post details in a separate bug if you're interested. As before, this patch passes all regression tests without triggering the added assertions. It's also robust against bug 339128, but I did find one case that triggers the "vertical damage extends outside table" assertion. I'll file that as a followup bug with a minimal testcase. (the testcase currently triggers "Invalid BC damage area"). To sum up, the patch fixes most of the cases that triggers the assertion but not all, and the assertions are now stricter than before. (I have a few testcases for edge cases that I've found along the way and I'll attach them shortly so we can add them as crash tests.) (In reply to comment #4) > There is one bug that kept me busy for two years.... bug 339128. Glad to hear it's working as intended ;-) (In reply to comment #5) > Mats you write in this patch: "XXX fix nsCellMap::InsertRows() instead" > Why aren't you doing just this? Isn't the patch wallpaper? I'm just pointing out what I think are existing wallpapers - nsCellMap should be responsible for setting the damage extent there. However, I think they can be investigated/fixed separately. (I can remove the comments if you don't want them.)
Attachment #343980 -
Attachment is obsolete: true
Attachment #344775 -
Flags: review?(bernd_mozilla)
Attachment #343980 -
Flags: review?(bernd_mozilla)
Reporter | ||
Comment 8•16 years ago
|
||
Mats, sorry for being so slow but I had the dynamic BC stuff to go into 1.9.1 and also some real live. Further this is not one liner that is obviously correct or not. I hope I can review this within the next two weeks. The main concern is that I would want those ("I think they can be investigated/fixed separately.") fixed first and my stomach guess is that this would make parts of the rebuild obsolete.
Assignee | ||
Comment 10•16 years ago
|
||
+static void +SetDamageArea(PRInt32 aXOrigin, + PRInt32 aYOrigin, + PRInt32 aWidth, + PRInt32 aHeight, + nsRect& aDamageArea) +{ + NS_ASSERTION(aXOrigin >= 0, "negative col index"); + NS_ASSERTION(aYOrigin >= 0, "negative row index"); + NS_ASSERTION(aWidth >= 0, "negative horizontal damage"); + NS_ASSERTION(aHeight >= 0, "negative vertical damage"); + aDamageArea.x = aXOrigin; + aDamageArea.y = aYOrigin; + aDamageArea.width = aWidth; + aDamageArea.height = aHeight; +} the first 4 line duplicate +#ifdef DEBUG +#define VerifyNonNegativeDamageRect(r) \ + NS_ASSERTION((r).x >= 0, "negative col index"); \ + NS_ASSERTION((r).y >= 0, "negative row index"); \ + NS_ASSERTION((r).width >= 0, "negative horizontal damage"); \ + NS_ASSERTION((r).height >= 0, "negative vertical damage"); Why do you encapsulate assertions with #ifdef DEBUG? I don't see the point of doing this.
Reporter | ||
Comment 11•16 years ago
|
||
> the first 4 line duplicate Right, I guess we could move the macro to a header file so it can be reused. > Why do you encapsulate assertions with #ifdef DEBUG? I wrapped the call sites to make it clear to the reader. An alternative is to name them NS_ASSERT_NON_NEGATIVE, NS_ASSERT_WITHIN_TABLE_BOUNDS or some such and skip the #ifdef DEBUG.
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 344775 [details] [diff] [review] Patch rev. 2 I would like to see a complete patch that compiles. This one certainly does not as it removes http://mxr.mozilla.org/mozilla-central/search?string=SetBCDamageArea but does not change the other callers of SetBCDamageArea in nsTableCellFrame.cpp, nsTableRowFrame.cpp etc. (I pulled this into my tree to compile it....) Second at all places where mDidRebuild = PR_TRUE; I would like to get the FirstRowIndex of the cellmap set as the second argument to SetDamageArea instead of 0. And then comes the master question once this is done, why we need the mDidRebuild variable. Please notice that the while loop at nsTableCellMap::RebuildConsideringCells is completely bogus one could far more efficient break once the match is found. If there would be two matches then the sibling structure would be certainly damaged at cellMap = cellMap->GetNextSibling(); that rowcount += stuff is just hiding what is really going on.
Attachment #344775 -
Flags: review?(bernd_mozilla) → review-
Comment 13•15 years ago
|
||
I see lots of occurrences of this assertion simply by loading TinderboxPushLog <http://tests.themasta.com/tinderboxpushlog/> using a current trunk build of Minefield on OS X.
Summary: ASSERTION: invalid BC damage area: 'PR_FALSE' → [BC] ASSERTION: invalid BC damage area: 'PR_FALSE'
Comment 15•13 years ago
|
||
Another way to invoke it - the new graph server: http://graphs-new.mozilla.org/#displayrange=7&branch=firefox&platform=[%22windows7%22%2C%22windowsxp%22%2C%22macosx%22%2C%22linux%22]&test=[%22ts%22%2C%22tp%22%2C%22ss%22] I'm going to try to revive this fix (if it makes sense once reviewing it); or otherwise look into the root cause if this patch and analysis is no longer relevant. Taking bug, at least for now.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Comment 16•13 years ago
|
||
Un-did bit-rot; had to expose AddBCDamageArea() to replace outside calls to SetBCDamageArea() which I believe is the correct action. Builds; no testing done.
Updated•13 years ago
|
Attachment #344775 -
Attachment is obsolete: true
Comment 17•13 years ago
|
||
Comment on attachment 569458 [details] [diff] [review] Fix invalid BC damage area assertion (un-bitrot patch) Tested primary testcases marked here; no assertions or other problems noted. Mats, care to give feedback before I ask for r=?
Attachment #569458 -
Flags: feedback?(matspal)
Comment 18•13 years ago
|
||
Comment on attachment 569458 [details] [diff] [review] Fix invalid BC damage area assertion (un-bitrot patch) Adding bernd as well for feedback (your bug 339128's first testcase doesn't generate warnings now).
Attachment #569458 -
Flags: feedback?(bernd_mozilla)
Reporter | ||
Comment 19•13 years ago
|
||
Comment on attachment 569458 [details] [diff] [review] Fix invalid BC damage area assertion (un-bitrot patch) You should use NS_MAX now, not PR_MAX. You missed a couple of PR_TRUE/FALSE. (other than that, I haven't really reviewed the code) Did you address Bernd's comments in comment 12?
Attachment #569458 -
Flags: feedback?(matspal) → feedback+
Comment 20•13 years ago
|
||
Thus far I merely un-bitrotted the patch and updated callers to SetBCDamageArea(). I'll fix the PR_blahs I missed, and look to address bernd's comments. If you want to take this back, I'm fine with that - I poked my head into it because I happened to run into the bug and saw there was an almost-done patch, then un-bitrotted it when the devtools people mentioned there were warping other areas of the code to avoid this assertion.
Comment 21•13 years ago
|
||
Mats, bernd: so far as addressing bernd's comment 12 above, I believe I understand the issue, but to properly address it *and* be sure, I'd need a better understanding of the CellMap code and RebuildConsideringCells/Rows. I'll update the patch for the other issues, and if one of you could look at mDidRebuild and RebuildConsideringCells(), I think the patch will be in good shape for consideration. I think the DevTools guys would appreciate this getting fixed.
Comment 22•13 years ago
|
||
fixed PR_MAX/PR_TRUE issues
Updated•13 years ago
|
Attachment #569458 -
Attachment is obsolete: true
Attachment #569458 -
Flags: feedback?(bernd_mozilla)
Updated•13 years ago
|
Attachment #569967 -
Flags: feedback?(bernd_mozilla)
Assignee | ||
Comment 23•13 years ago
|
||
I would prefer to replace nsRect damageArea(0, 0, GetColCount(), GetRowCount()); AddBCDamageArea(damageArea); with the SetFullBCDamageArea(); that is introduced in this patch. In general this patch lacks comments for the cellmap stuff. I would prefer instead of the global variable passing that is happening now a argument to be passed to the nsCellMap routines if the damageArea is relative to the cellmap or the entire table is covered by the damageArea. This would go along with comment 5 and remove the XXX comments. We now know the problem, why should we leave comments and that will not be fixable without digging deep into this bug.
Updated•13 years ago
|
Assignee: rjesup → matspal
Reporter | ||
Comment 24•13 years ago
|
||
Bernd's comments in comment 12 and comment 23 indicates he wants major changes to this patch. It's unlikely I'll do that in near future so disowning for now.
Assignee: matspal → nobody
Assignee | ||
Comment 25•13 years ago
|
||
Updated•13 years ago
|
Target Milestone: --- → mozilla11
Assignee | ||
Comment 26•13 years ago
|
||
red lines are wrongly setting the damage Area as flagged before green lines set it correctly blue lines are useless calls DamageAreaShift are Mats proposed repairs of the red lines Please notice that the most frequently called AppendCells has only one meaningfull call path that later needs to get shifted.
Assignee | ||
Comment 27•13 years ago
|
||
This is what I wanted from Mats, and I still believe that Mats was at 90% and gave prematurely up. 1. Move the fixup of the damageAreas to the places where it otherwise goes wrong 2. remove the member variable to track if the setting was done correctly by just setting it correctly
Attachment #569967 -
Attachment is obsolete: true
Attachment #572586 -
Attachment is obsolete: true
Attachment #569967 -
Flags: feedback?(bernd_mozilla)
Assignee | ||
Comment 28•13 years ago
|
||
http://hg.mozilla.org/try/pushloghtml?changeset=6745f515ad1f
Assignee | ||
Comment 29•13 years ago
|
||
>Please notice that the most frequently called AppendCells has only one meaningfull >call path that later needs to get shifted.
I have big difficulties to create a testcase that exhibits this path string from nsTableFrame::AppendCell which is called from nsTableRowFrame::AppendFrames.
Assignee | ||
Comment 30•13 years ago
|
||
Mats can you review my changes to nsCellMap I removed 460637-4.xthml from the testcases as it hangs the reftest suite It seems to me that <html xmlns="http://www.w3.org/1999/xhtml" class="reftest-wait"> <head><script type="text/javascript"><![CDATA[ .... document.removeChild(document.documentElement) .... document.documentElement.removeAttribute("class"); does not work as advertized and makes it difficult for the reftest suite to watch for <html xmlns="http://www.w3.org/1999/xhtml" class="reftest-wait"> how it looses its class.
Attachment #344784 -
Attachment is obsolete: true
Attachment #574551 -
Attachment is obsolete: true
Attachment #574634 -
Flags: review?(matspal)
Attachment #574634 -
Flags: review?(bernd_mozilla)
Assignee | ||
Comment 31•13 years ago
|
||
Comment on attachment 574634 [details] [diff] [review] patch + test case r+ on the testcases, the changes to nsTableFrame.cpp. my Cellmap changes should be reviewed by Mats.
Attachment #574634 -
Flags: review?(bernd_mozilla) → review+
Reporter | ||
Comment 32•13 years ago
|
||
Comment on attachment 574634 [details] [diff] [review] patch + test case > layout/tables/nsCellMap.h > + bool mDidRebuild; You forgot to remove this addition. > + PRInt32 aRgFirstRowIndex, Why use a signed type? I'd prefer if all of these were PRUint32. > layout/tables/nsCellMap.cpp > + SetDamageArea(startColIndex,aRgFirstRowIndex + aRowIndex, nit: add a space after the comma (this is in nsCellMap::AppendCell) Thanks for picking this up and finishing it! r=mats
Attachment #574634 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 33•13 years ago
|
||
>Why use a signed type? I'd prefer if all of these were PRUint32.
I would rather make the
PRUint32 rgStartRowIndex = 0;
a signed type now, as all other variables involved in setting the damageArea are signed and use differences and assert on being not negative (see setDamageArea). I would like to avoid a mixup of signed and unsigned types. We are talking about more than 10 of variables plus the consumers of the cellmap, which need to change.
Reporter | ||
Comment 34•13 years ago
|
||
> ... all other variables involved in setting the damageArea are signed
They should all be unsigned IMHO, but feel free to use a signed rgStartRowIndex
for now. Please file a follow-up bug to change them all to PRUint32.
Comment 35•13 years ago
|
||
This is now blocking bug 704132 ... here is a reproducible test case: 1. Apply the patch from bug 704132 to a debug version of Firefox 2. Click Firefox -> Web Developer -> Inspect 3. Click on any page element 4. Click the Style button (bottom right of the screen) 5. Click the Properties button (top of the screen above the style panel's search box) 6. Uncheck "Only user styles" 7. Check "Only user styles" The assertion is raised numerous times. In tests they are logged with a stack trace and because of the large number of assertions the tests time out.
Assignee | ||
Comment 36•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b39e1a64c2b there is a not reviewed change to reftest.list that made try angry https://tbpl.mozilla.org/?tree=Try&rev=0f6545ed7d60 reftest debug ok https://tbpl.mozilla.org/?tree=Try&rev=3cc2704681a0 android broken why? https://tbpl.mozilla.org/?tree=Try&rev=f8389724350b android ok https://tbpl.mozilla.org/?tree=Try&rev=54973a03d9de
Updated•13 years ago
|
Assignee: nobody → bernd_mozilla
Comment 37•13 years ago
|
||
This push hit a jsreftest shutdown crash with firefox-bin: cairo-hash.c:199: _cairo_hash_table_destroy: Assertion `hash_table->live_entries == 0' failed. https://tbpl.mozilla.org/php/getParsedLog.php?id=7595161&tree=Mozilla-Inbound which I hadn't seen before, and haven't seen since. Philor, any chance this rings a bell?
We don't even use a separate libcairo, do we?
Comment 39•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b39e1a64c2b please file a separate bug for the reftest crash if related (unless this needs a backout)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•