1348123 - Add non-debug mode bounds checking to nsTArray::InsertElementAt and nsTArray::ReplaceElementAt

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

Similar to bug 1159244 we should check bounds when inserting and replacing at a specific location. We probably don't have to worry about a perf hit here as inserting anywhere but the end isn't expected to be terribly fast.

Mainly I'm thinking of:
  - |RelpaceElementsAt| [1]
  - |InsertElementsAt| [2]
  - |InsertElementAt| [3]

[1] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#1962
[2] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#2020
[3] http://searchfox.org/mozilla-central/rev/571c1fd0ba0617f83175ccc06ed6f3eb0a1a8b92/xpcom/ds/nsTArray.h#2042

Comment 1

[Eric Rahm [:erahm]]

Attachment: Add release bounds checking when inserting and replacing

This adds release bounds checking to ReplaceElementsAt, InsertElementAt, and
InsertElementsAt to make sure the insertion point is within the current array
bounds.

MozReview-Commit-ID: 1pFr8LuOROI

Comment 2

[Eric Rahm [:erahm]]

Attachment: Add release bounds checking when inserting and replacing

Sorry for the churn, missed one.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8848276 [details] [diff] [review]
Add release bounds checking when inserting and replacing

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

Will be fascinating to see if anything turns up from these.

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9674c6cf3fb

Comment 5

[Eric Rahm [:erahm]]

Attachment: Add release bounds checking when inserting and replacing

This adds release bounds checking to ReplaceElementsAt, InsertElementAt, and
InsertElementsAt to make sure the insertion point is within the current array
bounds.

MozReview-Commit-ID: 1pFr8LuOROI

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

Updated the |>=| checks for InsertAt to be |>|; inserting at the end of the array is legit. Carrying forward r+ from IRC.

Comment 7

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28f0b4e6f37

Comment 8

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6b4d237ef6732d19240dde3b4fa9ac1d1d61aa
Bug 1348123 - Add release bounds checking when inserting and replacing. r=froydnj

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0e6b4d237ef6

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

> Approval Request Comment
> [Feature/Bug causing the regression]:

This is a diagnostic improvement, it enforces bounds checking nsTArray insertion functions.

> [User impact if declined]:

Hard to track down crashes.

> [Is this code covered by automated tests?]:

These code paths are thoroughly tested.

> [Has the fix been verified in Nightly?]:

Baked on nightly for a few days.

> [Needs manual test from QE? If yes, steps to reproduce]: 

No.

> [List of other uplifts needed for the feature/fix]:

N/A

> [Is the change risky?]:

Low risk.

> [Why is the change risky/not risky?]:

This just flags erroneous behavior earlier and should give us better crash reports.

> [String changes made/needed]:

N/A

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8848606 [details] [diff] [review]
Add release bounds checking when inserting and replacing

This is a diagnostic improvement. We can take it. Aurora54+ & Beta53+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/473d3b13cc78

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8b1e9e5362d

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

Setting qe-verify- based on comment 10.