Closed Bug 1344628 Opened 3 years ago Closed 3 years ago

Assertion failure: GetTableRowGroupFrame()-> GetTableFrame()->IsDeletedRowIndexRangesEmpty() (mDeletedRowIndexRanges should be empty here!), at nsTableRowFrame.h:355

Categories

(Core :: Layout: Tables, defect)

54 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: cbook, Assigned: neerja)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, regression)

Attachments

(2 files, 8 obsolete files)

142.90 KB, text/plain
Details
5.07 KB, patch
Details | Diff | Splinter Review
Attached file bughunter stack
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
mats,Jonathan: could you take a look at this ?
Flags: needinfo?(mats)
Flags: needinfo?(jfkthame)
: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?
Blocks: 1285874
Flags: needinfo?(jfkthame) → needinfo?(npancholi)
http://www.c-rewards.com is another page where this assertion on load can be reproduced.
(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.
Assignee: nobody → npancholi
Flags: needinfo?(npancholi)
Attachment #8844241 - Flags: review?(dholbert)
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!
Attachment #8844241 - Attachment is obsolete: true
Flags: needinfo?(mats) → needinfo?(dbaron)
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?)
Flags: needinfo?(dbaron) → needinfo?(npancholi)
Version: unspecified → 54 Branch
Flags: needinfo?(npancholi) → needinfo?(dbaron)
Attachment #8844248 - Attachment is obsolete: true
(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
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!)
Flags: needinfo?(dbaron) → needinfo?(npancholi)
Attachment #8844698 - Attachment is obsolete: true
Flags: needinfo?(npancholi) → needinfo?(dbaron)
(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!
Duplicate of this bug: 1344808
Duplicate of this bug: 1344429
Duplicate of this bug: 1344288
Duplicate of this bug: 1344312
Duplicate of this bug: 1344542
(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
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
Flags: needinfo?(dbaron)
Attachment #8844725 - Flags: review+
(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!
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a97e0294c73e
Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron
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)
https://hg.mozilla.org/mozilla-central/rev/a97e0294c73e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8845172 - Flags: approval-mozilla-aurora?
Comment on attachment 8845172 [details] [diff] [review]
Bug 1344628 - Remove RecalculateRowIndex() and combine its functionality with ResetRowIndices(). r=dbaron

Fix an assertion failure. Aurora54+.
Attachment #8845172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1344583
You need to log in before you can comment on or make changes to this bug.