IME leaks nsPrivateTextRangeList objects. A patch, which also cleans up nsPrivateTextRangeList, coming.
Created attachment 250345 [details] [diff] [review] possible patch I'm not quite sure who should review this.
I'll review it. However, I'd prefer that you didn't inherit from nsTArray. I think you should just make it a member instead.
(In reply to comment #2) > However, I'd prefer that you didn't inherit from nsTArray. I > think you should just make it a member instead. > Any reason for that? I like the simplicity when inheriting nsTArray.
Just a healthy prejudice against multiple inheritance. Also, it means that your class exposes all nsTArray methods, which is really a violation of encapsulation.
I think it also increases code size, especially if nsTArray has virtual methods, because upcasting to the second base class now requires the "this" pointer to be adjusted, and (depending on the compiler) adjuster thunks for the second base class's virtual methods have to be created.
Essentially we're paying that cost so that we can pass an nsPrivateTextRangeList as an nsTArray, which we don't really need or want.
nsTArray doesn't have any virtual methods. That is why I decided inheriting it is ok. And I don't see why all the nsTArray methods shouldn't be exposed, nsPrivateTextRangeList is after all "list". But I'll make a new patch where nsTArray is a member.
Created attachment 250437 [details] [diff] [review] v2 Doesn't look quite as nice, mainly because of handling of OOM. DOMEvents really shouldn't allocate memory in their constructor, but that is a different bug.
I think you should make tempPrivateTextRange an nsRefPtr and not delete it explicitly. I think that in the current patch it will actually be double-deleted because we'll make an nsRefPtr temporary when we try to add it to the array. Anyway we don't want to be relying on whether such a temporary is created or not. I think the same OOM issues were in the previous patch.
Created attachment 250444 [details] [diff] [review] v3 Yes, using nsRefPtr makes sense.