Carsten Book [:Tomcat] Reporter
	Description • 7 years ago

Assertion failure: GetTableRowGroupFrame()-> GetTableFrame()->IsDeletedRowIndexRangesEmpty() (mDeletedRowIndexRanges should be empty here!), at c:\builds\moz2_slave\m-cen-w32-d-000000000000000000\build\src\layout\tables\nsTableRowFrame.h:355

seems to be a recent trunk regression found by bughunter and reproduced on latest m-c debug windows tinderbox build. Regressio maybe from Bug 1285874  ?

Steps to reproduce:
-> Load https://en.wikipedia.org/wiki/Philippines
--> Assertion failure after a few seconds

	Carsten Book [:Tomcat] Reporter
	Comment 1 • 7 years ago

mats,Jonathan: could you take a look at this ?

	Jonathan Kew [:jfkthame]
	Comment 2 • 7 years ago

:neerja, could you take a look, please; this seems to be a regression from bug 1285874. At a glance, ISTM that nsTableFrame::InsertRowGroups needs to call RecalculateRowIndices() somewhere early on, but I'm not sure what more (if anything) it would need to do. Or maybe ResetRowIndices() should do the Recalculate if necessary?

	Carsten Book [:Tomcat] Reporter
	Comment 3 • 7 years ago

http://www.c-rewards.com is another page where this assertion on load can be reproduced.

	Neerja Pancholi[:neerja] Assignee
	Comment 4 • 7 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> :neerja, could you take a look, please; this seems to be a regression from
> bug 1285874. At a glance, ISTM that nsTableFrame::InsertRowGroups needs to
> call RecalculateRowIndices() somewhere early on, but I'm not sure what more
> (if anything) it would need to do. Or maybe ResetRowIndices() should do the
> Recalculate if necessary?

Thanks Jonathan. I'll take a look at this.

	Neerja Pancholi[:neerja] Assignee
	Comment 6 • 7 years ago

Hi dbaron,

This assertion failure was due to the fix for Bug 1285874 so I'm sending the review request to you since you reviewed the original bug fix. 

I believe this also fixes these related failures (but will confirm)-
Bug 1344312
Bug 1344808
Bug 1344429
Bug 1344288
Bug 1344312

Thanks!

	David Baron :dbaron:
	Comment 7 • 7 years ago

What if there are deletions that affect aRowGroupsToExclude?  (It might well be that that can't happen, given the callers, but we should make sure it's safe and future-proof...)

(Also, I wasn't aware of ResetRowIndices, but it seems awfully similar to RecalculateRowIndices, which you just added.  Perhaps RecalculateRowIndices should call ResetRowIndices with an empty exclusion set?)

	Neerja Pancholi[:neerja] Assignee
	Comment 11 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #7)
> What if there are deletions that affect aRowGroupsToExclude?  (It might well
> be that that can't happen, given the callers, but we should make sure it's
> safe and future-proof...)
> 
> (Also, I wasn't aware of ResetRowIndices, but it seems awfully similar to
> RecalculateRowIndices, which you just added.  Perhaps RecalculateRowIndices
> should call ResetRowIndices with an empty exclusion set?)

I just updated a patch which combines the functionality of RecalculateRowIndices into
ResetRowIndices. Try run looks good for this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ffdb07d993c2fcece4ce7ddcdf012f12bbc2592

I think this fix should be safe and future-proof because the documentation for
ResetRowIndices explicitly states that aRowGroupsToExclude is only meant to
exclude row groups for which the rows haven't been added yet (Even though the rows
are part of mFrames for the row group, the function InsertRows has not been called yet)
See https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.h#795-800
So calling ResetRowIndices via the row insertion path is not a problem. 

In the case of when ResetRowIndices is called from nsTableFrame::DoRemoveFrame, it uses an 
empty slice, so we're good again -
https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#2722

	David Baron :dbaron:
	Comment 12 • 7 years ago

So could you add an assertion (probably in an #ifdef DEBUG block) to assert that the row indices for all the excluded rows are 0?  (Though it might be better if we had a different value for "uninitialized row index" so that we could distinguish from the first row!)

	Neerja Pancholi[:neerja] Assignee
	Comment 14 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #12)
> So could you add an assertion (probably in an #ifdef DEBUG block) to assert
> that the row indices for all the excluded rows are 0?  (Though it might be
> better if we had a different value for "uninitialized row index" so that we
> could distinguish from the first row!)

Sounds good. :) I added the assertion you mentioned.
I'm not sure off hand how easy it will be to change the initial value for the row index to be other than '0'. It seems to me like parts of the code rely on it to be '0'. I will have to look into this more if I have to prove/disprove it.
Let me know if you think I should look into it and I'll create a follow up bug.
Thanks!

	Neerja Pancholi[:neerja] Assignee
	Comment 20 • 7 years ago

(In reply to Neerja Pancholi[:neerja] from comment #14)
> (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #12)
> > So could you add an assertion (probably in an #ifdef DEBUG block) to assert
> > that the row indices for all the excluded rows are 0?  (Though it might be
> > better if we had a different value for "uninitialized row index" so that we
> > could distinguish from the first row!)
> 
> Sounds good. :) I added the assertion you mentioned.
> I'm not sure off hand how easy it will be to change the initial value for
> the row index to be other than '0'. It seems to me like parts of the code
> rely on it to be '0'. I will have to look into this more if I have to
> prove/disprove it.
> Let me know if you think I should look into it and I'll create a follow up
> bug.
> Thanks!

+
Try run with this patch looks good to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6642dafdd0dd0df16fe6de1f5faa9f81171bdf38&selectedJob=82327659

	David Baron :dbaron:
	Comment 21 • 7 years ago

Comment on attachment 8844725 [details] [diff] [review]
Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r?dbaron

>   while (!excludeRowGroupsEnumerator.AtEnd()) {
>     excludeRowGroups.PutEntry(static_cast<nsTableRowGroupFrame*>(excludeRowGroupsEnumerator.get()));
>+    #ifdef DEBUG

Don't put any spaces before the #ifdef (i.e., don't indent it).  Same for the #endif

However, immediately inside the #ifdef and #endif, please put a { and a } at the 4-space indent, and then indent the contents an extra two spaces, so the variables inside of it don't leak outside the #ifdef.

>+    // Check to make sure that the row indices of all excluded row groups
>+    // are '0' (i.e. the initial value since they haven't been added yet)

"all excluded" -> "all rows in excluded"

>+    const nsFrameList& rowFrames =
>+      excludeRowGroupsEnumerator.get()->PrincipalChildList();
>+    for (nsFrameList::Enumerator rEnum(rowFrames); !rEnum.AtEnd(); rEnum.Next()) {
>+      nsTableRowFrame *row = static_cast<nsTableRowFrame*>(rEnum.get());
>+      MOZ_ASSERT(row->GetRowIndex() == 0);

Assertion text here would be useful, e.g., "exclusions cannot be used for rows that were already added, because we'd need to process mDeletedRowIndexRanges".

>-      for (nsFrameList::Enumerator rows(rowFrames); !rows.AtEnd(); rows.Next()) {
>+      for (nsFrameList::Enumerator rEnum(rowFrames); !rEnum.AtEnd(); rEnum.Next()) {

I'd suggest keeping the "rows" variable name here rather than "rEnum".

>-    if (!didRecalculate) {
>-      if (aRowIndex < origNumRows) {
>-        AdjustRowIndices(aRowIndex, numNewRows);
>+    // Perform row index adjustment only if row indices were not
>+    // reset above
>+    if (!shouldRecalculateIndex) {
>+      if(aRowIndex < origNumRows) {
>+      AdjustRowIndices(aRowIndex, numNewRows);

Please leave the space between "if" and "(", and leave the indentation of AdjustRowIndices.


r=dbaron with that

	Neerja Pancholi[:neerja] Assignee
	Comment 22 • 7 years ago

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #21)
> Comment on attachment 8844725 [details] [diff] [review]
> Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality
> with ResetRowIndices(). r?dbaron
> 
> >   while (!excludeRowGroupsEnumerator.AtEnd()) {
> >     excludeRowGroups.PutEntry(static_cast<nsTableRowGroupFrame*>(excludeRowGroupsEnumerator.get()));
> >+    #ifdef DEBUG
> 
> Don't put any spaces before the #ifdef (i.e., don't indent it).  Same for
> the #endif
RESOLVED

> However, immediately inside the #ifdef and #endif, please put a { and a } at
> the 4-space indent, and then indent the contents an extra two spaces, so the
> variables inside of it don't leak outside the #ifdef.
RESOLVED

> >+    // Check to make sure that the row indices of all excluded row groups
> >+    // are '0' (i.e. the initial value since they haven't been added yet)
> 
> "all excluded" -> "all rows in excluded"
RESOLVED

> >+    const nsFrameList& rowFrames =
> >+      excludeRowGroupsEnumerator.get()->PrincipalChildList();
> >+    for (nsFrameList::Enumerator rEnum(rowFrames); !rEnum.AtEnd(); rEnum.Next()) {
> >+      nsTableRowFrame *row = static_cast<nsTableRowFrame*>(rEnum.get());
> >+      MOZ_ASSERT(row->GetRowIndex() == 0);
> 
> Assertion text here would be useful, e.g., "exclusions cannot be used for
> rows that were already added, because we'd need to process
> mDeletedRowIndexRanges".
RESOLVED

> >-      for (nsFrameList::Enumerator rows(rowFrames); !rows.AtEnd(); rows.Next()) {
> >+      for (nsFrameList::Enumerator rEnum(rowFrames); !rEnum.AtEnd(); rEnum.Next()) {
> 
> I'd suggest keeping the "rows" variable name here rather than "rEnum".
RESOLVED

> >-    if (!didRecalculate) {
> >-      if (aRowIndex < origNumRows) {
> >-        AdjustRowIndices(aRowIndex, numNewRows);
> >+    // Perform row index adjustment only if row indices were not
> >+    // reset above
> >+    if (!shouldRecalculateIndex) {
> >+      if(aRowIndex < origNumRows) {
> >+      AdjustRowIndices(aRowIndex, numNewRows);
> 
> Please leave the space between "if" and "(", and leave the indentation of
> AdjustRowIndices.
RESOLVED

> 
> r=dbaron with that

Thanks dbaron!

	Pulsebot
	Comment 27 • 7 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97e0294c73e
Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron

	Daniel Holbert [:dholbert]
	Comment 28 • 7 years ago

Comment on attachment 8845172 [details] [diff] [review]
Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron

Review of attachment 8845172 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableFrame.cpp
@@ +534,5 @@
> +      for (nsFrameList::Enumerator rows(rowFrames); !rows.AtEnd(); rows.Next()) {
> +        nsTableRowFrame *row = static_cast<nsTableRowFrame*>(rows.get());
> +        MOZ_ASSERT(row->GetRowIndex() == 0,
> +                   "exclusions cannot be used for rows that were already added,"
> +                     "because we'd need to process mDeletedRowIndexRanges");

(Note: I de-indented this line 2 spaces before landing, so it lines up with the line before)

@@ +549,5 @@
>        const nsFrameList& rowFrames = rgFrame->PrincipalChildList();
>        for (nsFrameList::Enumerator rows(rowFrames); !rows.AtEnd(); rows.Next()) {
> +        if (mozilla::StyleDisplay::TableRow ==
> +            rows.get()->StyleDisplay()->mDisplay) {
> +          nsTableRowFrame *row = static_cast<nsTableRowFrame*>(rows.get());

(And I tweaked this and one other line to use "nsTableRowFrame* row" (with the star hugging the typename rather than the variable-name)

	Carsten Book [:Tomcat] Reporter
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a97e0294c73e

	Neerja Pancholi[:neerja] Assignee
	Comment 30 • 7 years ago

Comment on attachment 8845172 [details] [diff] [review]
Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron

Approval Request Comment
[Feature/regressing bug #]: Bug 1285874
[User impact if declined]: possible crash if webpages using tables are loaded
[Describe test coverage new/current, TreeHerder]: reftests generally cover any mistakes that would be made in changing this code
[Risks and why]: most of the changes here are refactoring and only minor changes that affect behavior by addressing a previously missed edge case
[String/UUID change made/needed]: none

	Gerry Chang [:gchang]
	Comment 31 • 7 years ago

Comment on attachment 8845172 [details] [diff] [review]
Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron

Fix an assertion failure. Aurora54+.

	Carsten Book [:Tomcat] Reporter
	Comment 32 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d8e1baa9f98