Asking dbaron for review for this since he reviewed the original patch for Bug 1285874

Try run seems to be ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99a67f23a67b1de32eb50c7011d6f482fb9eacd6&selectedJob=96039241

Comment on attachment 8863859 [details] Bug 1361229 - Add explanatory comments and assertion to nsTableFrame to ensure that the value of both smallerIter and aDeletedRowStoredIndex remains valid. https://reviewboard.mozilla.org/r/135594/#review139372 r=dbaron with the following changes ::: layout/tables/nsTableFrame.cpp:997 (Diff revision 3) > + // smallerIter can never be out of bounds because of this check and the fact > + // that our range map can never be empty as we check for size above. So understanding this comment requires understanding that upper_bound() might return end(), which is out-of-bounds. So maybe restate this as: // While greaterIter might be out-of-bounds (by being equal to end()), smallerIter now cannot be, since we returned early above for a 0-size map. and move it to right after the smallerIter-- so the "now" makes sense. ::: layout/tables/nsTableFrame.cpp:1003 (Diff revision 3) > + // Checks to ensure that aDeletedRowIndexRanges does not already contain > + // aDeletedRowStoredIndex. That would mean we are trying to delete an already > + // deleted row. Remove this comment; it says the same thing the assertion text does. (Or, if you want, move the parts that aren't in the assertion text already into it.) ::: layout/tables/nsTableFrame.cpp:1006 (Diff revision 3) > + if (greaterIter == mDeletedRowIndexRanges.end() || smallerIter != greaterIter) { > + // Note: smallerIter can only be equal to greaterIter when both > + // of them point to the beginning of the map and in that case smallerIter > + // does not "exist" but we clip smallerIter to point to beginning of map > + // so that it doesn't point to something unknown or outside the map boundry. > + > + // Note: When greaterIter is not the end (i.e. it "exists") upper_bound() > + // ensures aDeletedRowStoredIndex < greaterIter->first so no need to > + // assert that. > + MOZ_ASSERT(aDeletedRowStoredIndex > smallerIter->second, "Trying to delete " > + "an already deleted row?"); > + } A few comments here: I don't see why the "greaterIter == mDeletedRowIndexRanges.end() ||" is needed. If greaterIter is end(), then smallerIter != greaterIter, so you should be able to check only the latter condition. Second, it's better to put the conditions inside the assertion so they don't run in non-DEBUG builds. That is, instead of having a MOZ_ASSERT inside an if, just: MOZ_ASSERT(smallerIter == greaterIter || aDeletedRowStoredIndex > smallerIter->second, "...");

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a208e1be3910 Add explanatory comments and assertion to nsTableFrame to ensure that the value of both smallerIter and aDeletedRowStoredIndex remains valid. r=dbaron