1005392 - Find seems to keep documents alive too long

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

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

It has happened now several times during the last few months that
find seems to keep alive a document which has been already navigated away.
That increases cycle collection time rather dramatically.

I wonder if e10s-fying has caused this?


Perhaps we should wrappercache TypeAHeadFind and make it skippable, and mark
the owned nodes black during CanSkip.
Or release stuff earlier.

Comment 1

[Mike de Boer [:mikedeboer]]

(In reply to Olli Pettay [:smaug] from comment #0)
> Perhaps we should wrappercache TypeAHeadFind and make it skippable, and mark
> the owned nodes black during CanSkip.

I think this would be fine under all circumstances for TypeAheadFind.

Comment 2

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

Attachment: v1

This changes the API behavior a bit, since currentWindow will now return inner window. That feels more correct, since that is where the results live, in the document inside an inner window.

The testcase for this is to open a tab, load a bugzilla bug page to it, press
ctrl+f, search for 'mozilla', focus location bar and load about:blank.
The bug page will show up in the CC graphs.

TypeAheadFind code is odd, but this was the simplest change to fix the obvious
runtime leak.
ReleaseStrongMemberVariables() name isn't perhaps the best, since there is
still mWebBrowserFind, which is strong, but that doesn't need to be
cleared in this case (it isn't cycle collectable thing)

mDidAddObservers is there to prevent adding more observers than needed.

This bug is very bad for anyone using find a lot.

https://tbpl.mozilla.org/?tree=Try&rev=64137503fa57

Comment 3

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

<gavin> smaug: why is mDidAddObservers needed
<smaug> gavin: the API doesn't prevent anyone to re-initialize the thing
<smaug> gavin: and observer service keeps adding observers to the list
<gavin> smaug: this happens in practice?
<smaug> I'm not aware of that
<gavin> did you notice this in testing or something, or is it speculative?
<smaug> this is speculative
<gavin> ok

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/07dfcb557d32

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/07dfcb557d32