669845 - The quick find bar causes a zombie compartment that lives until the next search (only on pages with onclick handlers)

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Geoff Lankow (:darktrojan)]

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

Looks like you need to actually type something in the bar to cause this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Gavin, is this something you could look into, or find someone else for?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

How about just using weak references?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Oh, the obvious things are weak references. Maybe it's entraining stuff via the references to nsIDOMRanges.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Oh, no, they aren't (mCurrentWindow, mFoundLink, mFoundEditable)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ranges are what I was thinking of in comment 2, for what it's worth...

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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 [:mccr8]]

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 [:mccr8]]

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: [e-mail]]]

Attachment: 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 [:mccr8]]

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 [inactive]]

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]

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 17

[Nicholas Nethercote [inactive]]

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 [:bzbarsky]]

> 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 [inactive]]

Aha!  This sounds related to bug 717549.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Did adding one of those to your trivial page cause the zombie compartment to happen, then?

Comment 21

[dindog]

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 (Exited; not receiving bugmail, old account, do not use)]

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 [inactive]]

> 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 [inactive]]

Attachment: 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 [inactive]]

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 [inactive]]

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 (Exited; not receiving bugmail, old account, do not use)]

(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 [inactive]]

> Two different top level windows?

I don't know.  about:robots was the compartment that wouldn't disappear.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 [:bzbarsky]]

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]

I'm seeing it both with find (ctrl-f) and quick find (/) - shouldn't we have to change the title ?

Comment 33

[Marco Castelluccio [:marco]]

Bug 718273 is about https://developer.mozilla.org/, that hasn't any onclick handler.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 36

[Dão Gottwald [::dao]]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 38

[:Gavin Sharp [email: [e-mail]]]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

How about this instead?

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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][[e-mail]]]

I believe this can cause significantly slower CC times when otherwise dead page is kept alive
by find.

Comment 43

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

Just got 15x increase to CC times because of find :/
I think this is actually [Snappy:P1]

Comment 44

[:Gavin Sharp [email: [e-mail]]]

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 (Exited; not receiving bugmail, old account, do not use)]

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: [e-mail]]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #46)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Narrower patch.  This avoids hitting the component manager on every setDocshell call (which happens on every tab switch).

Comment 49

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This is a better patch.

Comment 50

[:Gavin Sharp [email: [e-mail]]]

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 (Exited; not receiving bugmail, old account, do not use)]

(In reply to Gavin Sharp (use [e-mail] 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 (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/bdb34145c37a

Comment 53

[Nicholas Nethercote [inactive]]

Cool!  Thanks, Kyle and Gavin.