864448 - Leak when caretPositionFromPoint eyes a scrollbar

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Leak nsDocument, nsGlobalWindow, nsDOMCaretPosition

Comment 1

[Timothy Nikkel (:tnikkel)]

I guess we need cycle collection for the mAnonymousContentNode member. Should have probably got a content person to review the original patch.

Comment 2

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

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

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Haven't tested that it fixes the leak or compiled yet, but will do that before pushing.

Comment 4

[Scott Johnson (:jwir3)]

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

Comment 5

[Timothy Nikkel (:tnikkel)]

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

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

Comment 6

[Timothy Nikkel (:tnikkel)]

crashtest
https://hg.mozilla.org/integration/mozilla-inbound/rev/89b6ecc3afe1

Comment 7

[Timothy Nikkel (:tnikkel)]

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

Comment 8

[Scott Johnson (:jwir3)]

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f88be5270662
https://hg.mozilla.org/mozilla-central/rev/89b6ecc3afe1