365798 - IME leaks nsPrivateTextRangeList objects

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

IME leaks nsPrivateTextRangeList objects.
A patch, which also cleans up nsPrivateTextRangeList, coming.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

I'm not quite sure who should review this.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just a healthy prejudice against multiple inheritance. Also, it means that your class exposes all nsTArray methods, which is really a violation of encapsulation.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Essentially we're paying that cost so that we can pass an nsPrivateTextRangeList as an nsTArray, which we don't really need or want.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Yes, using nsRefPtr makes sense.