Last Comment Bug 758894 - 'Highlight all' feature causes zombie compartments
: 'Highlight all' feature causes zombie compartments
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks: leakychrome ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-05-26 09:55 PDT by Josef Vitu
Modified: 2012-06-13 01:59 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch? (2.04 KB, patch)
2012-06-02 06:26 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
asaf: review+
Details | Diff | Splinter Review

Description Josef Vitu 2012-05-26 09:55:45 PDT
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 2012-05-26 10:09:54 PDT
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 [:njn] 2012-05-29 16:21:08 PDT
Aryeh, we suspect selection code might be involved here, can you take a look?
Comment 3 Kris Maglione [:kmag] 2012-05-29 16:23:58 PDT
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 (working until September 2) 2012-05-31 04:54:09 PDT
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) 2012-05-31 06:20:31 PDT
> 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 2012-06-02 02:09:29 PDT
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] (vacation Aug 25-28) 2012-06-02 02:36:02 PDT
Yes, please file a new bug. This bug is just about zombie compartments.
Comment 8 Josef Vitu 2012-06-02 03:42:53 PDT
(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] (vacation Aug 25-28) 2012-06-02 05:41:00 PDT
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] (vacation Aug 25-28) 2012-06-02 06:26:54 PDT
Created attachment 629467 [details] [diff] [review]
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] (vacation Aug 25-28) 2012-06-02 06:27:21 PDT
(And yes, the patch does fix the leak :) )
Comment 12 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 15:10:59 PDT
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] (vacation Aug 25-28) 2012-06-03 06:42:51 PDT
https://hg.mozilla.org/mozilla-central/rev/d2bf31e62b72

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