Last Comment Bug 864448 - Leak when caretPositionFromPoint eyes a scrollbar
: Leak when caretPositionFromPoint eyes a scrollbar
Status: RESOLVED FIXED
[MemShrink]
: mlk, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Mac OS X
-- normal (vote)
: mozilla23
Assigned To: Timothy Nikkel (:tnikkel)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: randomstyles 326633 803719
  Show dependency treegraph
 
Reported: 2013-04-22 12:20 PDT by Jesse Ruderman
Modified: 2013-05-13 17:09 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (450 bytes, text/html)
2013-04-22 12:20 PDT, Jesse Ruderman
no flags Details
patch (1.06 KB, patch)
2013-04-22 12:49 PDT, Timothy Nikkel (:tnikkel)
bugs: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2013-04-22 12:20:13 PDT
Created attachment 740414 [details]
testcase

Leak nsDocument, nsGlobalWindow, nsDOMCaretPosition
Comment 1 User image Timothy Nikkel (:tnikkel) 2013-04-22 12:25:57 PDT
I guess we need cycle collection for the mAnonymousContentNode member. Should have probably got a content person to review the original patch.
Comment 2 User image Olli Pettay [:smaug] 2013-04-22 12:38:39 PDT
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 User image Timothy Nikkel (:tnikkel) 2013-04-22 12:49:01 PDT
Created attachment 740423 [details] [diff] [review]
patch

Haven't tested that it fixes the leak or compiled yet, but will do that before pushing.
Comment 4 User image Scott Johnson (:jwir3) 2013-04-22 12:51:57 PDT
(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 User image Timothy Nikkel (:tnikkel) 2013-04-22 13:20:14 PDT
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 User image Timothy Nikkel (:tnikkel) 2013-04-22 13:24:53 PDT
crashtest
https://hg.mozilla.org/integration/mozilla-inbound/rev/89b6ecc3afe1
Comment 7 User image Timothy Nikkel (:tnikkel) 2013-04-22 13:26:13 PDT
(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 User image Scott Johnson (:jwir3) 2013-04-22 13:28:41 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.