The default bug view has changed. See this FAQ.

Leak when caretPositionFromPoint eyes a scrollbar

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: tnikkel)

Tracking

(Blocks: 2 bugs, {mlk, testcase})

Trunk
mozilla23
x86_64
Mac OS X
mlk, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 740414 [details]
testcase

Leak nsDocument, nsGlobalWindow, nsDOMCaretPosition
(Assignee)

Comment 1

4 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

4 years ago
Created attachment 740423 [details] [diff] [review]
patch

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

4 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)

Comment 6

4 years ago
crashtest
https://hg.mozilla.org/integration/mozilla-inbound/rev/89b6ecc3afe1
(Assignee)

Updated

4 years ago
Blocks: 803719
(Assignee)

Comment 7

4 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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.