Closed
Bug 758894
Opened 12 years ago
Closed 12 years ago
'Highlight all' feature causes zombie compartments
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: josef, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file)
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Blocks: ZombieCompartments
Whiteboard: [MemShrink]
Comment 1•12 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
Comment 2•12 years ago
|
||
Aryeh, we suspect selection code might be involved here, can you take a look?
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
> 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•12 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•12 years ago
|
||
Yes, please file a new bug. This bug is just about zombie compartments.
Reporter | ||
Comment 8•12 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•12 years ago
|
Blocks: leakychrome
Assignee | ||
Comment 9•12 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•12 years ago
|
Component: General → Find Toolbar
Product: Firefox → Toolkit
QA Contact: general → fast.find
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
(And yes, the patch does fix the leak :) )
Comment 12•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bugs
Updated•12 years ago
|
Target Milestone: --- → mozilla15
You need to log in
before you can comment on or make changes to this bug.
Description
•