Bugzilla

"Quick" update.

Changes in the above diagnostic patch will help validate:
* Correct item is removed from mRecs on release builds
* Items are removed from mRecs during resizing are indeed null first on release builds
* Duplicate appends to mRecs will not occur on release builds 

Some other considerations:
* Could lock is drop for potentially blocking IO operations be a factor here? I took a quick look, and didn’t see anything during the lock drop that touches `mRecs`. So this seems unlikely.
* `mRecs.IndexOf` seems to return an unexpected `NoIndex`. Maybe it could return an incorrect item that gets deleted too early or twice? The [assumptions of E](https://searchfox.org/mozilla-central/source/xpcom/ds/nsTArray.h#136-142 ) in `nsTArray<E>` appear to hold: RefPtr has copy-constructor, operator< and operator== are defined in FrecencyComparator and (I’m pretty sure(?)) RefPtr is [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable).
* Could an item be destroyed and re-added? It looks like there are situations where we remove and then re-add an item to `mRecs`. But the item should be kept alive by RefPtr from what I can tell.
* `sLock.AssertCurrentThreadOwns` will only verify on debug builds, which isn’t super helpful as this crash seems rare enough to only happen on release builds in the wild. I considered adding MOZ_RELEASE_ASSERT here, but we already have proof_of_lock parameters, so this didn’t seem worth the time at this point.

It may also be worth noting the following:
* we seem to also have allocation problems, [for example](https://crash-stats.mozilla.org/report/index/8b2598b3-adbd-4c1a-8c14-b7bb60221014). May allow us to be more confident that items are being correctly added otherwise we would see these errors first.
* [This crash](https://crash-stats.mozilla.org/report/index/e6d79139-9361-40bc-9685-e8bf80220923#allthreads) was also interesting. It appears to be a result of trying to directly index `mRecs` with a `NoIndex`. Though the line reference is off and I’m not sure how we got past the ASSERT. I took a look around but it looks like all direct indexing (including via the iterators) of mRecs happens under lock. So it doesn’t seem like we can label this as another thread messing with the contents of mRecs.

Any further ideas or critiques of my assumptions would be greatly appreciated.

"Quick" update.

Changes in the above diagnostic patch will help validate:
* Correct item is removed from mRecs on release builds
* Items are removed from mRecs during resizing are indeed null first on release builds
* Duplicate appends to mRecs will not occur on release builds 

Some other considerations:
* Could lock is drop for potentially blocking IO operations be a factor here? I took a quick look, and didn’t see anything during the lock drop that touches `mRecs`. So this seems unlikely.
* `mRecs.IndexOf` seems to return an unexpected `NoIndex`. Maybe it could return an incorrect item that gets deleted too early or twice? The [assumptions of E](https://searchfox.org/mozilla-central/source/xpcom/ds/nsTArray.h#136-142 ) in `nsTArray<E>` appear to hold: RefPtr has copy-constructor, operator< and operator== are defined in FrecencyComparator and (I’m pretty sure(?)) RefPtr is [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable). If we assume all modifications to mRecs (justification below) then is there any other way `IndexOf` could return an incorrect value? This commit may help narrow down if this is a problem to begin with. 
* Could an item be destroyed and re-added? It looks like there are situations where we remove and then re-add an item to `mRecs`. But the item should be kept alive by RefPtr from what I can tell.
* `sLock.AssertCurrentThreadOwns` will only verify on debug builds, which isn’t super helpful as this crash seems rare enough to only happen on release builds in the wild. I considered adding MOZ_RELEASE_ASSERT here, but we already have proof_of_lock parameters, so this didn’t seem worth the time at this point.

It may also be worth noting the following:
* we seem to also have allocation problems, [for example](https://crash-stats.mozilla.org/report/index/8b2598b3-adbd-4c1a-8c14-b7bb60221014). May allow us to be more confident that items are being correctly added otherwise we would see these errors first.
* [This crash](https://crash-stats.mozilla.org/report/index/e6d79139-9361-40bc-9685-e8bf80220923#allthreads) was also interesting. It appears to be a result of trying to directly index `mRecs` with a `NoIndex`. Though the line reference is off and I’m not sure how we got past the ASSERT. I took a look around but it looks like all direct indexing (including via the iterators) of mRecs happens under lock. So it doesn’t seem like we can label this as another thread messing with the contents of mRecs.

Any further ideas or critiques of my assumptions would be greatly appreciated.