460637 - [BC] ASSERTION: invalid BC damage area: 'PR_FALSE'

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Mats Palmgren (inactive)]

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 (inactive)]

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 (inactive)]

Attachment: 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 (inactive)]

Attachment: 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]

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]

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]

That said, I fully agree with your analysis, this is certainly broken.

Comment 7

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

Attachment: crashtest.diff

Comment 9

[Bernd]

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]

+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 (inactive)]

> 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]

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]]

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 15

[Randell Jesup [:jesup] (needinfo me)]

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] (needinfo me)]

Attachment: 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] (needinfo me)]

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] (needinfo me)]

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 (inactive)]

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] (needinfo me)]

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] (needinfo me)]

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] (needinfo me)]

Attachment: Fix invalid BC damage area assertion (un-bitrot patch)

fixed PR_MAX/PR_TRUE issues

Comment 23

[Bernd]

 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 (inactive)]

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]

Attachment: this are the places where things go wrong

Comment 26

[Bernd]

Attachment: 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]

Attachment: 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 28

[Bernd]

http://hg.mozilla.org/try/pushloghtml?changeset=6745f515ad1f

Comment 29

[Bernd]

>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]

Attachment: 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]

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 (inactive)]

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]

>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 (inactive)]

> ... 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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 36

[Bernd]

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

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 (Exited; not receiving bugmail, old account, do not use)]

We don't even use a separate libcairo, do we?

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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]]

https://hg.mozilla.org/mozilla-central/rev/2a15404c1d71