1361229 - Add comments and assertions to clarify correctness of iterator value (Coverity complains about smallerItr possibly being out of bounds but this is not true)

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Comment 1

[Neerja Pancholi[:neerja]]

Attachment: Bug 1361229 - Add explanatory comments and assertion to nsTableFrame to ensure that the value of both smallerIter and aDeletedRowStoredIndex remains valid.

Review commit: https://reviewboard.mozilla.org/r/135594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135594/

Comment 2

[Neerja Pancholi[:neerja]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135594/diff/1-2/

Comment 3

[Neerja Pancholi[:neerja]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135594/diff/2-3/

Comment 4

[Neerja Pancholi[:neerja]]

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

Comment 5

[Neerja Pancholi[:neerja]]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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,
           "...");

Comment 7

[Neerja Pancholi[:neerja]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135594/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Carsten Book [:Tomcat]]

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