Last Comment Bug 460637 - [BC] ASSERTION: invalid BC damage area: 'PR_FALSE'
: [BC] ASSERTION: invalid BC damage area: 'PR_FALSE'
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: unspecified
: x86 Linux
: -- normal with 2 votes (vote)
: mozilla11
Assigned To: Bernd
:
Mentors:
: 605807 (view as bug list)
Depends on: 706779 711359 712849
Blocks: 315549 697398 704132
  Show dependency treegraph
 
Reported: 2008-10-19 01:38 PDT by Mats Palmgren (vacation)
Modified: 2012-02-01 13:57 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (1019 bytes, patch)
2008-10-19 02:00 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 1 (29.68 KB, patch)
2008-10-20 15:47 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 2 (30.22 KB, patch)
2008-10-25 14:59 PDT, Mats Palmgren (vacation)
bernd_mozilla: review-
Details | Diff | Splinter Review
crashtest.diff (4.96 KB, patch)
2008-10-25 17:24 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Fix invalid BC damage area assertion (un-bitrot patch) (34.51 KB, patch)
2011-10-25 12:17 PDT, Randell Jesup [:jesup]
mats: feedback+
Details | Diff | Splinter Review
Fix invalid BC damage area assertion (un-bitrot patch) (34.50 KB, patch)
2011-10-27 07:33 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
this are the places where things go wrong (2.76 KB, patch)
2011-11-07 13:11 PST, Bernd
no flags Details | Diff | Splinter Review
setdamageArea Call Structure (26.50 KB, image/svg+xml)
2011-11-11 00:15 PST, Bernd
no flags Details
revised patch (45.44 KB, patch)
2011-11-15 00:45 PST, Bernd
no flags Details | Diff | Splinter Review
patch + test case (49.13 KB, patch)
2011-11-15 11:41 PST, Bernd
mats: review+
bernd_mozilla: review+
Details | Diff | Splinter Review

Description Mats Palmgren (vacation) 2008-10-19 01:38:26 PDT
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
Comment 1 Mats Palmgren (vacation) 2008-10-19 01:44:55 PDT
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
Comment 2 Mats Palmgren (vacation) 2008-10-19 02:00:10 PDT
Created attachment 343788 [details] [diff] [review]
wip

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)
Comment 3 Mats Palmgren (vacation) 2008-10-20 15:47:48 PDT
Created attachment 343980 [details] [diff] [review]
Patch rev. 1

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
Comment 4 Bernd 2008-10-21 21:53:07 PDT
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.
Comment 5 Bernd 2008-10-25 02:37:34 PDT
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.
Comment 6 Bernd 2008-10-25 02:38:18 PDT
That said, I fully agree with your analysis, this is certainly broken.
Comment 7 Mats Palmgren (vacation) 2008-10-25 14:59:27 PDT
Created attachment 344775 [details] [diff] [review]
Patch rev. 2

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.)
Comment 8 Mats Palmgren (vacation) 2008-10-25 17:24:56 PDT
Created attachment 344784 [details] [diff] [review]
crashtest.diff
Comment 9 Bernd 2008-11-09 10:55:05 PST
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.
Comment 10 Bernd 2008-11-09 22:34:56 PST
+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.
Comment 11 Mats Palmgren (vacation) 2008-11-10 00:42:03 PST
> 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.
Comment 12 Bernd 2008-11-17 09:02:50 PST
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.
Comment 13 Jonathan Kew (:jfkthame) 2009-06-05 09:12:52 PDT
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.
Comment 14 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2010-10-20 16:46:18 PDT
*** Bug 605807 has been marked as a duplicate of this bug. ***
Comment 15 Randell Jesup [:jesup] 2011-05-31 15:40:08 PDT
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.
Comment 16 Randell Jesup [:jesup] 2011-10-25 12:17:39 PDT
Created attachment 569458 [details] [diff] [review]
Fix invalid BC damage area assertion (un-bitrot patch)

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.
Comment 17 Randell Jesup [:jesup] 2011-10-26 21:56:58 PDT
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=?
Comment 18 Randell Jesup [:jesup] 2011-10-26 22:14:53 PDT
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).
Comment 19 Mats Palmgren (vacation) 2011-10-27 00:03:44 PDT
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?
Comment 20 Randell Jesup [:jesup] 2011-10-27 06:55:20 PDT
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 Randell Jesup [:jesup] 2011-10-27 07:25:20 PDT
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 Randell Jesup [:jesup] 2011-10-27 07:33:36 PDT
Created attachment 569967 [details] [diff] [review]
Fix invalid BC damage area assertion (un-bitrot patch)

fixed PR_MAX/PR_TRUE issues
Comment 23 Bernd 2011-10-30 09:14:45 PDT

 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.
Comment 24 Mats Palmgren (vacation) 2011-11-06 06:57:48 PST
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.
Comment 25 Bernd 2011-11-07 13:11:22 PST
Created attachment 572586 [details] [diff] [review]
this are the places where things go wrong
Comment 26 Bernd 2011-11-11 00:15:24 PST
Created attachment 573759 [details]
setdamageArea Call Structure

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.
Comment 27 Bernd 2011-11-15 00:45:14 PST
Created attachment 574551 [details] [diff] [review]
revised patch

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
Comment 29 Bernd 2011-11-15 00:49:33 PST
>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.
Comment 30 Bernd 2011-11-15 11:41:17 PST
Created attachment 574634 [details] [diff] [review]
patch + test case

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.
Comment 31 Bernd 2011-11-15 11:43:08 PST
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.
Comment 32 Mats Palmgren (vacation) 2011-11-15 13:49:52 PST
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
Comment 33 Bernd 2011-11-16 22:10:35 PST
>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.
Comment 34 Mats Palmgren (vacation) 2011-11-17 13:40:41 PST
> ... 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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-25 07:11:56 PST
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.
Comment 37 :Ms2ger 2011-11-27 23:52:37 PST
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?
Comment 38 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 04:10:49 PST
We don't even use a separate libcairo, do we?
Comment 39 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-28 05:21:32 PST
https://hg.mozilla.org/mozilla-central/rev/0b39e1a64c2b

please file a separate bug for the reftest crash if related (unless this needs a backout)
Comment 40 Ed Morley [:emorley] 2011-12-04 07:19:02 PST
https://hg.mozilla.org/mozilla-central/rev/2a15404c1d71

Note You need to log in before you can comment on or make changes to this bug.