Last Comment Bug 669845 - The quick find bar causes a zombie compartment that lives until the next search (only on pages with onclick handlers)
: The quick find bar causes a zombie compartment that lives until the next sear...
Status: RESOLVED FIXED
[MemShrink:P2], [Snappy:P1]
:
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla13
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 688894 705661 718273 (view as bug list)
Depends on: 719101
Blocks: leakychrome ZombieCompartments
  Show dependency treegraph
 
Reported: 2011-07-07 02:33 PDT by Geoff Lankow (:darktrojan)
Modified: 2012-02-17 18:26 PST (History)
33 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sprinkle around some weakrefs (14.65 KB, patch)
2011-07-18 14:17 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
minimal reproducing test case (182 bytes, text/plain)
2012-01-13 01:49 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details
Patch (7.40 KB, patch)
2012-01-16 04:25 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (9.84 KB, patch)
2012-01-18 10:31 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (11.86 KB, patch)
2012-01-18 11:31 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
gavin.sharp: review-
Details | Diff | Review
Just throw them away (7.28 KB, patch)
2012-02-06 11:39 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
gavin.sharp: review-
Details | Diff | Review
Patch (4.88 KB, patch)
2012-02-14 12:25 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (5.16 KB, patch)
2012-02-17 08:46 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
gavin.sharp: review+
Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2011-07-07 02:33:19 PDT
The quick find bar (/ or ', not Ctrl+F) seems to keep a reference to the page it was last used on, causing a zombie compartment. Open it on another page, and the reference is released.
Comment 1 Geoff Lankow (:darktrojan) 2011-07-07 02:36:43 PDT
Looks like you need to actually type something in the bar to cause this.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-07 07:33:34 PDT
Hmm.  I bet the find impl (which is NOT attached to the page; it's owed by chrome) ends up holding a reference to the last bit of content it found an the document (e.g. so it can find again from that point).

There's an existing bug on hiding the find bar on navigation; it seems like resetting it on navigation would also be a good idea....
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-12 13:57:32 PDT
Gavin, is this something you could look into, or find someone else for?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-12 17:52:57 PDT
How about just using weak references?
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-12 17:54:24 PDT
Oh, the obvious things are weak references. Maybe it's entraining stuff via the references to nsIDOMRanges.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-12 18:04:02 PDT
Oh, no, they aren't (mCurrentWindow, mFoundLink, mFoundEditable)
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-12 19:07:19 PDT
Ranges are what I was thinking of in comment 2, for what it's worth...
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-13 10:30:41 PDT
I need to know how to reproduce this, precisely. Compartments don't seem to be reliably tied to tab lifetime (perhaps due to other leaks), so confirming that I've fixed this particular problem seems difficult.

Geoff: how did you confirm this reliably?
Comment 9 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-07-13 10:48:22 PDT
Here's how I can reproduce this reliably on OSX:

1. open a new tab, go to http://en.wikipedia.org/wiki/Strongly_connected_component
2. hit the weird Mac symbol - F to bring up the quick find bar
3. type "directed graph" into the quick find bar
4. close tab
5. go to about memory, click "minimize memory usage" a bunch of times, and the wikipedia compartment will still be there
6. click the x to close the quick find bar, minimize memory usage a few times, compartment is still there
7. open quick find bar again using weird symbol - F, minimize memory usage a few times, compartment goes away

I had a little trouble reproducing this at first because I accidentally searched about:memory using the quick find bar. ;)
Comment 10 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-07-13 11:32:25 PDT
Hmm.  I guess command-F is "Find" and not "Quick Find".  Replace "quick find" with "find" in my comment.

I tried it again with Quick Find and that leaks a compartment with that, too.

The steps are along the lines of:

1. open a new tab, go to http://en.wikipedia.org/wiki/Strongly_connected_component
2. hit / to bring up the quick find bar
3. type "directed graph" into the quick find bar
4. close tab
5. go to about memory, click "minimize memory usage" a bunch of times, and the wikipedia compartment will still be there
6. open quick find bar again using /, minimize memory usage a few times, compartment goes away

(no step to explicitly close the quick find bar, because it goes away on its own)
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-18 14:17:33 PDT
Created attachment 546635 [details] [diff] [review]
sprinkle around some weakrefs

This is what I tried, it doesn't seem to work. I'm still not sure that I have reliable STR, though.
Comment 12 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-07-18 14:25:52 PDT
Do my steps not work reliably for you?  For me, they work consistently with either Find or Quick Find.  I can try it with a clean profile, in case it is one of my addons that is causing it.
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-21 23:56:53 PDT
The steps in comment 10 reproduce reliably for me on Linux64 trunk builds.  The only caveat is that in step 6 I need to type at least one character after hitting '/' so that a search takes place.

Also, AFAICT any combination of "find" and "quick find" on the two pages seems to produce the same effects.
Comment 14 :aceman 2011-09-21 12:04:49 PDT
I can see it in todays Aurora8 Linux with the "search when I start typing" feature. Guess that is "quick find". I noticed it on http://www.joelscoins.com/africa.htm .
Comment 15 Justin Lebar (not reading bugmail) 2011-09-26 06:02:41 PDT
*** Bug 688894 has been marked as a duplicate of this bug. ***
Comment 16 John P Baker 2011-11-29 04:01:22 PST
*** Bug 705661 has been marked as a duplicate of this bug. ***
Comment 17 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-12 22:14:51 PST
Something worth pointing out:  when looking for the zombie compartment in about:memory, don't use either ctrl-f or '/' to find it!  You have to search for it manually or the zombie will go away.  I just spent a few minutes puzzled why I couldn't reproduce this.


Other findings:  this reproduces for me on:  Wikipedia pages, a local copy of a Wikipedia page, bbc.co.uk, techcrunch.com (where it keeps alive two compartments, the techcrunch.com one and a platform.twitter.com one), nytimes.com, www.w3.org, www.mozilla.org/en-US/firefox/fx/.

But it *doesn't* reproduce for me on:  valgrind.org, valgrind.org/njn/, hg.mozilla.org, a trivial local HTML page.  Furthermore, if I have a zombie from a previous search, searching on those pages clears the zombie.

So it appears to be correlated with some characteristic of the page.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 22:29:01 PST
> So it appears to be correlated with some characteristic of the page.

Like having an <input type="text"> or a <textarea> on it, perhaps?  The "doesn't" list is missing those as far as I can tell, the "does" almost certainly has it on all the pages (I spot-checked wikipedia, w3.org, and the firefox page; the others almost certainly have it _somewhere_ in the mess) , and there's special find code for finding things inside text controls that could well be holding on to things too long.
Comment 19 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-12 22:32:40 PST
Aha!  This sounds related to bug 717549.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 22:41:29 PST
Did adding one of those to your trivial page cause the zombie compartment to happen, then?
Comment 21 dindog 2012-01-13 00:04:46 PST
appending two element by scratchpad in valgrind.org changes nothing

var ipt = document.createElement('input');
ipt.setAttribute('type','text');
document.body.appendChild(ipt);

var txtarea = document.createElement('textarea');
document.body.appendChild(txtarea);
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-13 00:38:49 PST
0E2AD270 [nsDocument normal (xhtml) http://dailykos.com/]

    Root 0E2AD270 is a ref counted object with 1 unknown edge(s).
    known edges:
       1559A508 [XPCWrappedNative (HTMLDocument)] --[mIdentity]-> 0E2AD270
       0E50D138 [nsDocumentEncoder] --[mDocument]-> 0E2AD270
       1375DFD0 [nsNodeInfoManager] --[mDocument]-> 0E2AD270

When I find on a different page, the stack for that unknown edge disappearing is:

>	xul.dll!nsDocument::Release()  Line 1722	C++
 	xul.dll!nsHTMLDocument::Release()  Line 274 + 0xd bytes	C++
 	xul.dll!nsCOMPtr<nsINode>::assign_assuming_AddRef(nsINode * newPtr)  Line 507	C++
 	xul.dll!nsCOMPtr<nsINode>::assign_with_AddRef(nsISupports * rawPtr)  Line 1165	C++
 	xul.dll!nsCOMPtr<nsINode>::operator=(nsINode * rhs)  Line 652	C++
 	xul.dll!nsRange::DoSetRange(nsINode * aStartN, int aStartOffset, nsINode * aEndN, int aEndOffset, nsINode * aRoot, bool aNotInsertedYet)  Line 774	C++
 	xul.dll!nsRange::SetEnd(nsINode * aParent, int aOffset)  Line 1020	C++
 	xul.dll!nsRange::SetEnd(nsIDOMNode * aParent, int aOffset)  Line 998 + 0x15 bytes	C++
 	xul.dll!nsTypeAheadFind::GetSearchContainers(nsISupports * aContainer, nsISelectionController * aSelectionController, bool aIsFirstVisiblePreferred, bool aFindPrev, nsIPresShell * * aPresShell, nsPresContext * * aPresContext)  Line 724	C++
 	xul.dll!nsTypeAheadFind::FindItNow(nsIPresShell * aPresShell, bool aIsLinksOnly, bool aIsFirstVisiblePreferred, bool aFindPrev, unsigned short * aResult)  Line 388 + 0x8f bytes	C++
 	xul.dll!nsTypeAheadFind::Find(const nsAString_internal & aSearchString, bool aLinksOnly, unsigned short * aResult)  Line 1020 + 0x1a bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params)  Line 103	C++
Comment 23 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-13 01:15:32 PST
> appending two element by scratchpad in valgrind.org changes nothing

I also couldn't replicate this on my trivial case with <input text="type"> or a <textarea>.  Hmm.  But it must be something like that -- some feature that valgrind.org and those other pages lack.
Comment 24 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-13 01:49:52 PST
Created attachment 588346 [details]
minimal reproducing test case

I just reduced DuckDuckGo's home page to this.  It's the minimal reproducer I can find.  If you remove the empty onclick handler the zombie goes away.
Comment 25 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-13 01:55:00 PST
It appears to be the onclick handler that's the cause.  If you change the <a> link in the test to this:

  <button onclick="">Firefox</button>

the zombie still reproduces.
Comment 26 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-13 01:58:37 PST
One time I managed to get two zombies from searches present at the same time, and searching on valgrind.org only got rid of the second one, I couldn't get rid of the first one.  Not sure how I did that, though.
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-13 02:52:22 PST
(In reply to Nicholas Nethercote [:njn] from comment #26)
> One time I managed to get two zombies from searches present at the same
> time, and searching on valgrind.org only got rid of the second one, I
> couldn't get rid of the first one.  Not sure how I did that, though.

Two different top level windows?
Comment 28 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-13 03:07:19 PST
> Two different top level windows?

I don't know.  about:robots was the compartment that wouldn't disappear.
Comment 29 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-13 03:47:19 PST
(In reply to Boris Zbarsky (:bz) from comment #2)
> Hmm.  I bet the find impl (which is NOT attached to the page; it's owed by
> chrome) ends up holding a reference to the last bit of content it found an
> the document (e.g. so it can find again from that point).
> 
> There's an existing bug on hiding the find bar on navigation; it seems like
> resetting it on navigation would also be a good idea....

This is basically what's going on.  We're entraining stuff through ranges.  I don't know that there's a solution here other than explicitly dropping either the ranges or the nsTypeAheadFind object at the right time.
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-13 07:59:12 PST
So the stack in comment 22 goes through this line:

724   mEndPointRange->SetEnd(rootNode, childCount);

If you look at nsTypeAheadFind::SetDocShell, that resets all sorts of stuff but NOT mEndPointRange.  Does resetting mEndPointRange in there help?
Comment 31 Jo Hermans 2012-01-15 05:26:47 PST
I'm seeing it both with find (ctrl-f) and quick find (/) - shouldn't we have to change the title ?
Comment 32 Marco Castelluccio [:marco] 2012-01-15 05:33:33 PST
*** Bug 718273 has been marked as a duplicate of this bug. ***
Comment 33 Marco Castelluccio [:marco] 2012-01-15 05:36:06 PST
Bug 718273 is about https://developer.mozilla.org/, that hasn't any onclick handler.
Comment 34 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-16 01:17:59 PST
(In reply to Boris Zbarsky (:bz) from comment #30)
> So the stack in comment 22 goes through this line:
> 
> 724   mEndPointRange->SetEnd(rootNode, childCount);
> 
> If you look at nsTypeAheadFind::SetDocShell, that resets all sorts of stuff
> but NOT mEndPointRange.  Does resetting mEndPointRange in there help?

Nope.  The nsTypeAheadFind's mFind entrains a node in mIterNode that holds the document alive too.

We could go through and track down and null out everything, but I think the front end should just stop reusing these objects.
Comment 35 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-16 04:25:31 PST
Created attachment 588842 [details] [diff] [review]
Patch
Comment 36 Dão Gottwald [:dao] 2012-01-16 04:36:41 PST
Comment on attachment 588842 [details] [diff] [review]
Patch

>--- a/toolkit/content/widgets/browser.xml
>+++ b/toolkit/content/widgets/browser.xml

There's more code to remove in swapDocShells. I think you can remove getTabBrowser at this point.
Comment 37 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-18 10:31:53 PST
Created attachment 589565 [details] [diff] [review]
Patch
Comment 38 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-18 10:47:31 PST
Adding/removing the pref observer and re-reading prefs on setDocShell seems wasteful. Can't you just leave init(), and have it do that?
Comment 39 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-18 11:31:37 PST
Created attachment 589584 [details] [diff] [review]
Patch

How about this instead?
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-18 14:33:10 PST
Comment on attachment 589584 [details] [diff] [review]
Patch

discussed this on IRC, I think we should go for a more isolated fix that just adds the necessary changes to setDocShell
Comment 41 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-06 11:39:30 PST
Created attachment 594762 [details] [diff] [review]
Just throw them away

This passed try, and I've been browsing locally with it for a bit and haven't noticed any issues.
Comment 42 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-11 08:41:51 PST
I believe this can cause significantly slower CC times when otherwise dead page is kept alive
by find.
Comment 43 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-02-12 17:30:30 PST
Just got 15x increase to CC times because of find :/
I think this is actually [Snappy:P1]
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 15:40:47 PST
Comment on attachment 594762 [details] [diff] [review]
Just throw them away

Can we get the simple patch we discussed on IRC to fix the leak in the short term, and then revisit more invasive changes in a followup? I don't want to block the leak fix on bigger architectural changes (even though they have merit).
Comment 45 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-13 15:49:41 PST
The narrower patch is still not simple, because we have to throw away the nsFind inside the nsTypeFindAhead to fix all the leaks (or add a way to flush the nsFind's state).

More importantly, given the number of leaks we've tracked down to various parts of chrome holding onto things in content, I think that this kind of explicit flushing of objects that could just as easily be thrown away is an anti-pattern, and I don't want to encourage it.
Comment 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-13 18:33:21 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #45)
> The narrower patch is still not simple, because we have to throw away the
> nsFind inside the nsTypeFindAhead to fix all the leaks

Why is that difficult?

> I don't want to encourage it.

Sounds good, I'm not opposed to making your original change. It just requires more careful review than I'm able to provide in a timely manner, and so I suggested doing it in a followup that isn't blocking fixing this leak.
Comment 47 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-13 18:37:26 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #46)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #45)
> > The narrower patch is still not simple, because we have to throw away the
> > nsFind inside the nsTypeFindAhead to fix all the leaks
> 
> Why is that difficult?

It's not particularly difficult.

> > I don't want to encourage it.
> 
> Sounds good, I'm not opposed to making your original change. It just
> requires more careful review than I'm able to provide in a timely manner,
> and so I suggested doing it in a followup that isn't blocking fixing this
> leak.

Ok, I thought there was a problem with what the patch does.  If it's just the amount of time it takes to review it, that's fine, we can do the narrower fix for the moment.
Comment 48 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-14 12:25:48 PST
Created attachment 597135 [details] [diff] [review]
Patch

Narrower patch.  This avoids hitting the component manager on every setDocshell call (which happens on every tab switch).
Comment 49 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-17 08:46:54 PST
Created attachment 598255 [details] [diff] [review]
Patch

This is a better patch.
Comment 50 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-17 09:17:11 PST
Comment on attachment 598255 [details] [diff] [review]
Patch

>diff --git a/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp

> nsTypeAheadFind::SetCaseSensitive(bool isCaseSensitive)

>+  NS_ENSURE_TRUE(EnsureFind(), NS_ERROR_FAILURE);
>+
>+  mFind->SetCaseSensitive(mCaseSensitive);

You don't actually need to EnsureFind() here, right? You could just set mCaseSensitive if !mFind, and call SetCaseSensitive otherwise. I guess it doesn't really matter either way, since consumers are unlikely to tweak case sensitivity without also triggering a find.

>diff --git a/toolkit/components/typeaheadfind/nsTypeAheadFind.h b/toolkit/components/typeaheadfind/nsTypeAheadFind.h

>   // Cached useful interfaces
>+  bool mCaseSensitive;
>   nsCOMPtr<nsIFind> mFind;

Seems like you should put mCaseSensitive below mFind.
Comment 51 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-17 09:18:23 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #50)
> Comment on attachment 598255 [details] [diff] [review]
> Patch
> 
> >diff --git a/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
> 
> > nsTypeAheadFind::SetCaseSensitive(bool isCaseSensitive)
> 
> >+  NS_ENSURE_TRUE(EnsureFind(), NS_ERROR_FAILURE);
> >+
> >+  mFind->SetCaseSensitive(mCaseSensitive);
> 
> You don't actually need to EnsureFind() here, right? You could just set
> mCaseSensitive if !mFind, and call SetCaseSensitive otherwise. I guess it
> doesn't really matter either way, since consumers are unlikely to tweak case
> sensitivity without also triggering a find.

True.

> >diff --git a/toolkit/components/typeaheadfind/nsTypeAheadFind.h b/toolkit/components/typeaheadfind/nsTypeAheadFind.h
> 
> >   // Cached useful interfaces
> >+  bool mCaseSensitive;
> >   nsCOMPtr<nsIFind> mFind;
> 
> Seems like you should put mCaseSensitive below mFind.

Done.

Thanks for the review.
Comment 52 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-17 15:19:17 PST
https://hg.mozilla.org/mozilla-central/rev/bdb34145c37a
Comment 53 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-02-17 18:26:02 PST
Cool!  Thanks, Kyle and Gavin.

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