758894 - 'Highlight all' feature causes zombie compartments

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Josef Vitu]

Steps to reproduce:

1. Visit http://www.mozilla.org/en-US/
2. Press CTRL+F to raise the search bar.
3. Type 'a' to the search field.
4. Click on 'Highlight all'.
5. Close the tab.

Even after refreshing about:compartments several times and waiting approx. 10 minutes, a compartment labeled 'http://www.mozilla.org/en-US/' was still present.

about:memory says:

│   ├─────1,774,144 B (01.23%) -- compartment(http://www.mozilla.org/en-US/)
│   │   ├──1,085,440 B (00.75%) -- gc-heap
│   │   │  ├────328,864 B (00.23%) -- objects
│   │   │  │    ├──245,728 B (00.17%) ── function
│   │   │  │    └───83,136 B (00.06%) ── non-function
│   │   │  ├────321,968 B (00.22%) -- arena
│   │   │  │    ├──304,760 B (00.21%) ── unused
│   │   │  │    ├────8,728 B (00.01%) ── padding
│   │   │  │    └────8,480 B (00.01%) ── headers
│   │   │  ├────305,232 B (00.21%) -- shapes
│   │   │  │    ├──199,200 B (00.14%) ── tree
│   │   │  │    ├───53,872 B (00.04%) ── base
│   │   │  │    └───52,160 B (00.04%) ── dict
│   │   │  ├────121,440 B (00.08%) ── scripts
│   │   │  └──────7,936 B (00.01%) ── sundries
│   │   ├────320,384 B (00.22%) ── script-data
│   │   ├────295,840 B (00.20%) -- shapes-extra
│   │   │    ├──205,408 B (00.14%) ── tree-tables
│   │   │    ├───47,104 B (00.03%) ── compartment-tables
│   │   │    ├───22,112 B (00.02%) ── dict-tables
│   │   │    └───21,216 B (00.01%) ── tree-shape-kids
│   │   ├─────60,288 B (00.04%) ── objects/slots
│   │   ├──────8,672 B (00.01%) ── type-inference/script-main
│   │   └──────3,520 B (00.00%) ── other-sundries

These results are from Safe Mode. When NoScript is used, two compartments instead of just one get leaked, according to about:compartments.

It seems zombie compartments aren't created while performing steps 2-5 above on the 'about:' page, but they are created on 'about:support'.

If the Highlight all feature is used in another tab, the old tab's zombie compartment disappears.

Comment 1

[alex_mayorga]

I do see the zombie in about:compartments after following the steps outlined.

Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120526084732

Comment 2

[Nicholas Nethercote [inactive]]

Aryeh, we suspect selection code might be involved here, can you take a look?

Comment 3

[Kris Maglione [:kmag]]

The highlighter code stores nsIEditors for each of the editable elements that it highlights for use in removing the highlighting. Chances are, these are holding the document alive from binary code since they don't use proxies.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I'm probably not the best person to look at this -- I honestly don't have more than the foggiest understanding of how our garbage collection works.  If someone wants to step me through how to deal with this sort of thing, maybe by pointing to some similar bugs whose fixes I can look it, I could try taking a look when I have the time.

Comment 5

[Justin Lebar (not reading bugmail)]

> I honestly don't have more than the foggiest understanding of how our garbage collection works

Thankfully you don't need to have much of an idea.  :)

This bug is happening because somehow, someone is holding a strong reference (e.g. nsCOMPtr, nsRefPtr) to some object which transitively holds a strong reference to a window object.  It could be a DOM node, an event, or any number of things.

See for example Roc's latest post about fixing the nytimes zombie [1].

It could also be because some structure doesn't have the right CC macros; it's missing a field, for example.  I don't have a good bug as an example of this, but maybe mccr8 does...

[1] http://robert.ocallahan.org/2012/05/firefox-vs-new-york-times.html

Comment 6

[Josef Vitu]

I have just found something that may be related to this problem.

Follow these steps:
1. Visit http://www.mozilla.org/en-US/
2. Press CTRL+F to raise the search bar.
3. Type 'a' to the search field.
4. Click on 'Highlight all'.
5. Press CTRL+T  to open a new tab.
6. Close the new tab or switch back to the previous tab.
7. Press CTRL+F to focus the search field.
8. Type 'b' to the search field (only 'b' should be in the search field right now).
9. Click on 'Highlight all'.

All occurrences of 'a' and 'b' are highlighted, instead of just occurrences of 'b'. The 'Highlight all' feature works as expected (meaning occurrences of 'a' cease to be highlighted) if steps 5, 6 and 9 are skipped.

Should I fill this as a new bug?

Comment 7

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

Yes, please file a new bug. This bug is just about zombie compartments.

Comment 8

[Josef Vitu]

(In reply to Olli Pettay [:smaug] from comment #7)
> Yes, please file a new bug. This bug is just about zombie compartments.

Filed as bug 760780.

Comment 9

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

Mano, could you look at this? Based on CVS blame you should know this code ;)

If I read the code correctly _highlightDoc ends up creating ranges etc which are kept alive,
but not released until something else is searched.
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#919

Comment 10

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

Attachment: patch?

Mano, is this all wrong?
I don't see where else _searchRange, _startPt and _endPt are used, so why
do they need to be stored in 'this'?

Comment 11

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

(And yes, the patch does fix the leak :) )

Comment 12

[Asaf Romano (gone)]

Comment on attachment 629467 [details] [diff] [review]
patch?

The code using this members used to live in few functions, but it doesn't anymore, so this change is fine.

Thanks.

Comment 13

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

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