Leak when caretPositionFromPoint eyes a scrollbar

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: jruderman, Assigned: tnikkel)

Tracking

(Blocks 2 bugs, {memory-leak, testcase})

Trunk
mozilla23
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Posted file testcase
Leak nsDocument, nsGlobalWindow, nsDOMCaretPosition
(Assignee)

Comment 1

6 years ago
I guess we need cycle collection for the mAnonymousContentNode member. Should have probably got a content person to review the original patch.
Sounds right.
NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsDOMCaretPosition, mOffsetNode) ->
NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(nsDOMCaretPosition, mOffsetNode, mAnonymousContentNode)
should probably work.

want to write a patch. I can review
(Assignee)

Comment 3

6 years ago
Posted patch patchSplinter Review
Haven't tested that it fixes the leak or compiled yet, but will do that before pushing.
Assignee: nobody → tnikkel
Attachment #740423 - Flags: review?(bugs)
(In reply to Timothy Nikkel (:tn) from comment #1)
> I guess we need cycle collection for the mAnonymousContentNode member.
> Should have probably got a content person to review the original patch.

I feel like this was brought up somewhere, but either I forgot to include it, or it was decided that we didn't want it...
Attachment #740423 - Flags: review?(bugs) → review+
Whiteboard: [MemShrink]
(Assignee)

Comment 5

6 years ago
Tested and it works
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88be5270662

Forgot to add testcase as a crashtest. I will do that.
(Assignee)

Updated

6 years ago
Blocks: 803719
(Assignee)

Comment 7

6 years ago
(In reply to Scott Johnson (:jwir3) from comment #4)
> (In reply to Timothy Nikkel (:tn) from comment #1)
> > I guess we need cycle collection for the mAnonymousContentNode member.
> > Should have probably got a content person to review the original patch.
> 
> I feel like this was brought up somewhere, but either I forgot to include
> it, or it was decided that we didn't want it...

I'm guessing you must have forgot about it since it fixes the leak. No mention about not wanting it in the original bug.
(In reply to Timothy Nikkel (:tn) from comment #7)
> (In reply to Scott Johnson (:jwir3) from comment #4)
> > (In reply to Timothy Nikkel (:tn) from comment #1)
> > > I guess we need cycle collection for the mAnonymousContentNode member.
> > > Should have probably got a content person to review the original patch.
> > 
> > I feel like this was brought up somewhere, but either I forgot to include
> > it, or it was decided that we didn't want it...
> 
> I'm guessing you must have forgot about it since it fixes the leak. No
> mention about not wanting it in the original bug.

Probably, although I can't seem to find out where it was referred to in the bug. At any rate, thanks for cleaning up my mess.
https://hg.mozilla.org/mozilla-central/rev/f88be5270662
https://hg.mozilla.org/mozilla-central/rev/89b6ecc3afe1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.