Closed Bug 864448 Opened 7 years ago Closed 7 years ago

Leak when caretPositionFromPoint eyes a scrollbar

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jruderman, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase, Whiteboard: [MemShrink])

Attachments

(2 files)

Attached file testcase
Leak nsDocument, nsGlobalWindow, nsDOMCaretPosition
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
Attached 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]
Tested and it works
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88be5270662

Forgot to add testcase as a crashtest. I will do that.
Blocks: 803719
(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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.