'Highlight all' feature causes zombie compartments

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Find Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Josef Vitu, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 668871
Whiteboard: [MemShrink]

Comment 1

5 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 15 Branch → Trunk
Aryeh, we suspect selection code might be involved here, can you take a look?
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
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.
> 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
(Reporter)

Comment 6

5 years ago
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?
(Assignee)

Comment 7

5 years ago
Yes, please file a new bug. This bug is just about zombie compartments.
(Reporter)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 728407
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Updated

5 years ago
Component: General → Find Toolbar
Product: Firefox → Toolkit
QA Contact: general → fast.find
(Assignee)

Comment 10

5 years ago
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'?
Attachment #629467 - Flags: review?(mano)
(Assignee)

Comment 11

5 years ago
(And yes, the patch does fix the leak :) )
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.
Attachment #629467 - Flags: review?(mano) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d2bf31e62b72
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Assignee: nobody → bugs
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.