** CID 1401685: API usage errors (INVALIDATE_ITERATOR) /layout/tables/nsTableFrame.cpp: 1017 in nsTableFrame::AddDeletedRowIndex(int)() ________________________________________________________________________________________________________ *** CID 1401685: API usage errors (INVALIDATE_ITERATOR) /layout/tables/nsTableFrame.cpp: 1017 in nsTableFrame::AddDeletedRowIndex(int)() 1011 // merge current index with smaller and greater range as they are consecutive 1012 smallerIter->second = greaterIter->second; 1013 mDeletedRowIndexRanges.erase(greaterIter); 1014 } 1015 else { 1016 // add aDeletedRowStoredIndex in the smaller range as it is consecutive >>> CID 1401685: API usage errors (INVALIDATE_ITERATOR) >>> Dereferencing iterator "smallerIter" though it is already past the end of its container. 1017 smallerIter->second = aDeletedRowStoredIndex; 1018 } 1019 } else if (greaterIter != mDeletedRowIndexRanges.end() && 1020 greaterIter->first == aDeletedRowStoredIndex + 1) { 1021 // add aDeletedRowStoredIndex in the greater range as it is consecutive 1022 mDeletedRowIndexRanges.insert(std::pair<int32_t, int32_t>

Looks like this code was added in bug 1285874.

(In reply to Mats Palmgren (:mats) from comment #1) > Looks like this code was added in bug 1285874. Thanks Mats, I'll take a look. :jesup, can you please tell me how you were able to reproduce this? Thanks!

> :jesup, can you please tell me how you were able to reproduce this? Thanks! This is from static analysis - Coverity looked at the code and says this will (or may) happen.

(In reply to Randell Jesup [:jesup] from comment #3) > > :jesup, can you please tell me how you were able to reproduce this? Thanks! > > This is from static analysis - Coverity looked at the code and says this > will (or may) happen. Cool, thanks :jesup.

[Tracking Requested - why for this release]: misusing APIs can have serious consequences.

Neerja, do you have an estimate of when this will be done?

(In reply to Nathan Froyd [:froydnj] from comment #6) > Neerja, do you have an estimate of when this will be done? Hi Nathan, I'll fix it this week. Thanks!

Tracking 54/55+ for the reason in Comment 5.

(In reply to Neerja Pancholi[:neerja] from comment #7) > (In reply to Nathan Froyd [:froydnj] from comment #6) > > Neerja, do you have an estimate of when this will be done? > > Hi Nathan, I'll fix it this week. > Thanks! Gentle ping on this bug, since we've now moved 54 to Beta.

(In reply to Nathan Froyd [:froydnj] from comment #9) > (In reply to Neerja Pancholi[:neerja] from comment #7) > > (In reply to Nathan Froyd [:froydnj] from comment #6) > > > Neerja, do you have an estimate of when this will be done? > > > > Hi Nathan, I'll fix it this week. > > Thanks! > > Gentle ping on this bug, since we've now moved 54 to Beta. Hi Nathan, Sorry, I got distracted with something else. I'm keeping your needinfo as a reminder and I'll clear it when I'm finished fixing this which will be this week. Thanks!

After looking at this I can't see a way in which "smallerItr" could ever be out of bounds because of this check: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1006 and also that the list can never be empty because of: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#982 (:dholbert helped me cross check this in case I was missing something so thank you!) I would suggest that this be closed as invalid. Or if someone could point me to what might be making Coverity flag this then I might be able to refactor it to remove the error? What do you think :froydnj and :jesup? Some possible suggestions by dholbert as to why Coverity might be complaining - 1. It doesn't know that the list can never be empty because of: https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#982 2. The equality for greaterItr and smallerItr (https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1005) and then a check on greaterItr to not be end of list (https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1011) might make Coverity think that smallerIter here could be end of list?

If you're certain it's invalid, go ahead. (or mark it invalid and add a comment about it in the code -- "smallerItr can't be out of bounds because XXXX" and/or add a debug assertion if it's not a hot path

Yeah, after staring at this, I'm not sure why Coverity thinks it can be beyond the *end* of the container--unless the error message is bad, and they meant before the *start* of the container. If the access being flagged is beyond the end, then surely the access to smallerIter that dominates this use is *also* beyond the end... Debug assertions might encourage Coverity to understand the code better, but flagging it in Coverity's database itself is also a reasonable option.

Thanks for looking at this!

Thanks jesup and froydnj! I'm closing this bug as invalid and I've created another bug to add a comment and an assertion to further clarify why smallerItr cannot be out of bounds here.

Per comment #15, mark 54/55 won't fix.