IME leaks nsPrivateTextRangeList objects

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({inputmethod, mlk})

Trunk
x86
All
inputmethod, mlk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: smontagu → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #250345 - Flags: review?(masayuki)
Attachment #250345 - Flags: superreview?(roc)
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.
Attachment #250345 - Attachment is obsolete: true
Attachment #250437 - Flags: superreview?(roc)
Attachment #250437 - Flags: review?(roc)
Attachment #250345 - Flags: superreview?(roc)
Attachment #250345 - Flags: review?(masayuki)
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.
Attachment #250437 - Attachment is obsolete: true
Attachment #250444 - Flags: superreview?(roc)
Attachment #250444 - Flags: review?(roc)
Attachment #250437 - Flags: superreview?(roc)
Attachment #250437 - Flags: review?(roc)
Attachment #250444 - Flags: superreview?(roc)
Attachment #250444 - Flags: superreview+
Attachment #250444 - Flags: review?(roc)
Attachment #250444 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Keywords: mlk
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.