Closed Bug 407074 Opened 12 years ago Closed 12 years ago

Leak PropItem and nsStringBuffer with execCommand("inserthtml", ...)

Categories

(Core :: Editor, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jruderman)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase)

Attachments

(2 files)

Attached file testcase
According to trace-refcnt shutdown information, this testcase makes Firefox leak two nsStringBuffer objects.
The leak happens because nsHTMLEditRules::CreateStyleForInsertText bails while it owned the PropItem |item| through a raw pointer.  In particular,

4378         res = mEditor->DeleteNode(leftNode);
4379         if (NS_FAILED(res)) return res;

gets an NS_ERROR_FAILURE and exits silently.  My patch fixes the leak by using nsAutoPtr<PropItem> for the owning pointer |item|, so we don't have to worry about doing "delete item;" on every exit path.

My patch also makes several debug-only changes: instrument PropItem and StyleCache so we see when they leak, and use NS_ENSURE_SUCCESS in CreateStyleForInsertText so that early bails aren't silent in debug builds.
Summary: Leak nsStringBuffer with <textarea contenteditable="true"> and execCommand("inserthtml", ...) → Leak PropItem and nsStringBuffer with execCommand("inserthtml", ...)
Attached patch patchSplinter Review
Attachment #292190 - Flags: superreview?(peterv)
Attachment #292190 - Flags: review?(peterv)
Assignee: nobody → jruderman
Status: NEW → ASSIGNED
Comment on attachment 292190 [details] [diff] [review]
patch

>+  StyleCache() : PropItem(nsnull, EmptyString(), EmptyString()), mPresent(PR_FALSE) {

Seems like that could just use PropItem().
Attachment #292190 - Flags: superreview?(peterv)
Attachment #292190 - Flags: superreview+
Attachment #292190 - Flags: review?(peterv)
Attachment #292190 - Flags: review+
Attachment #292190 - Flags: approval1.9?
Comment on attachment 292190 [details] [diff] [review]
patch

a=endgame drivers
Attachment #292190 - Flags: approval1.9? → approval1.9+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.