Closed Bug 1268898 Opened 8 years ago Closed 8 years ago

Leak in sdk/event/dom

Categories

(Add-on SDK Graveyard :: General, defect)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed, firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: bugzilla.mozilla.org, Assigned: bugzilla.mozilla.org)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P2])

Attachments

(1 file, 2 obsolete files)

Bug-1268898-Fix-window-leak.patch
1017 bytes, patch
gkrizsanits
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
I'm seeing places.xul being leaked in about:memory.
I have narrowed it down to a SDK addon.
Analyzing the the gc/cc logs points at a Map held in a variable called "listeners" as a property of a sandbox global.
Searching the sourcecode with the browser toolbox leaves sdk/event/dom as the only obvious match.

Note that this is a chrome->chrome edge, so bug 695480 does not magically sever it when the window gets closed.



----

000001F1DC959C00 [rc=4] nsGlobalWindow # 152 inner chrome://browser/content/places/places.xul
000001F1D720D000 [rc=4] nsGlobalWindow # 168 inner chrome://browser/content/places/places.xul
000001F1D5FED000 [rc=4] nsGlobalWindow # 209 inner chrome://browser/content/places/places.xul
000001F1D5330C00 [rc=4] nsGlobalWindow # 68 inner chrome://browser/content/places/places.xul
000001F1DBD1EC00 [rc=4] nsGlobalWindow # 146 inner chrome://browser/content/places/places.xul
000001F1DBF84800 [rc=4] nsGlobalWindow # 164 inner chrome://browser/content/places/places.xul
000001F1CC359000 [rc=4] nsGlobalWindow # 64 inner chrome://browser/content/places/places.xul
000001F1D6969800 [rc=4] nsGlobalWindow # 78 inner chrome://browser/content/places/places.xul


$ python2 ./find_roots.py cc-edges.15748.1461940833.log 000001F1DBD1EC00 
Parsing cc-edges.15748.1461940833.log. Done loading graph. 

000001F1E5B17060 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 000001F1DBD1EC00 [nsGlobalWindow # 146 inner chrome://browser/content/places/places.xul]

    Root 000001F1E5B17060 is a marked GC object.


$ python2 ./find_roots.py gc-edges.15748.1461940833.log 000001F1E5B17060 -bro
Parsing gc-edges.15748.1461940833.log. Done loading graph.

via mIncumbentJSGlobal :
000001F1DD2B94C0 [Sandbox 1f1dce50278]
    --[listeners]--> 000001F1DC217040 [Map 1f1dcfe6cd0]
    --[key]--> 000001F1D792A5C0 [Proxy <no private>]
    --[private]--> 000001F1E5B28020 [Proxy <no private>]
    --[private]--> 000001F1E5B17060 [Window <no private>]
note the detached window

536,146,865 B (100.0%) -- explicit
├──275,881,192 B (51.46%) -- window-objects
│  ├──172,879,832 B (32.24%) -- top(none)/detached
│  │  ├──166,436,720 B (31.04%) -- window(chrome://browser/content/places/places.xul)
│  │  │  ├──154,253,440 B (28.77%) -- js-compartment([System Principal], about:blank)
│  │  │  │  ├──116,672,528 B (21.76%) -- classes
│  │  │  │  │  ├───54,162,992 B (10.10%) -- shapes
│  │  │  │  │  │   ├──49,439,216 B (09.22%) -- gc-heap
│  │  │  │  │  │   │  ├──37,917,464 B (07.07%) ── tree [66]
│  │  │  │  │  │   │  ├───7,286,872 B (01.36%) ── dict [66]
│  │  │  │  │  │   │  └───4,234,880 B (00.79%) ── base [66]
│  │  │  │  │  │   └───4,723,776 B (00.88%) -- malloc-heap
│  │  │  │  │  │       ├──2,312,576 B (00.43%) ── dict-tables [66]
│  │  │  │  │  │       ├──1,243,040 B (00.23%) ── tree-tables [66]
│  │  │  │  │  │       └──1,168,160 B (00.22%) ── tree-kids [66]
│  │  │  │  │  ├───47,071,040 B (08.78%) -- class(Function)/objects
│  │  │  │  │  │   ├──43,362,432 B (08.09%) ── gc-heap [66]
│  │  │  │  │  │   └───3,708,608 B (00.69%) ── malloc-heap/slots [66]
│  │  │  │  │  ├────9,923,136 B (01.85%) -- class(<non-notable classes>)/objects
│  │  │  │  │  │    ├──5,151,936 B (00.96%) ── gc-heap [66]
│  │  │  │  │  │    └──4,771,200 B (00.89%) -- malloc-heap
│  │  │  │  │  │       ├──4,713,856 B (00.88%) ── slots [66]
│  │  │  │  │  │       ├─────40,448 B (00.01%) ── elements/normal [66]
│  │  │  │  │  │       └─────16,896 B (00.00%) ── misc [66]
│  │  │  │  │  ├────2,515,712 B (00.47%) -- class(Block)/objects
│  │  │  │  │  │    ├──1,292,672 B (00.24%) ── malloc-heap/slots [66]
│  │  │  │  │  │    └──1,223,040 B (00.23%) ── gc-heap [66]
│  │  │  │  │  ├────1,534,496 B (00.29%) -- class(Object)/objects
│  │  │  │  │  │    ├────938,912 B (00.18%) ── gc-heap [66]
│  │  │  │  │  │    └────595,584 B (00.11%) ── malloc-heap/slots [66]
│  │  │  │  │  ├────1,432,256 B (00.27%) ── class(With)/objects/gc-heap [66]
│  │  │  │  │  ├───────16,512 B (00.00%) ── class(WithTemplate)/objects/gc-heap
│  │  │  │  │  └───────16,384 B (00.00%) ── class(Proxy)/objects/gc-heap
│  │  │  │  ├───17,580,288 B (03.28%) ── compartment-tables [66]
│  │  │  │  ├───16,117,712 B (03.01%) -- scripts
│  │  │  │  │   ├──12,866,048 B (02.40%) ── gc-heap [66]
│  │  │  │  │   └───3,251,664 B (00.61%) ── malloc-heap/data [66]
│  │  │  │  ├────2,703,360 B (00.50%) ── cross-compartment-wrapper-table [66]
│  │  │  │  ├──────749,760 B (00.14%) ── type-inference/object-type-tables [66]
│  │  │  │  └──────429,792 B (00.08%) ── sundries/malloc-heap [66]
│  │  │  ├───11,847,392 B (02.21%) -- dom
│  │  │  │   ├───8,510,432 B (01.59%) ── element-nodes [66]
│  │  │  │   ├───3,323,232 B (00.62%) ── other [66]
│  │  │  │   └──────13,728 B (00.00%) ── text-nodes [66]
│  │  │  ├──────222,224 B (00.04%) ── style-sheets [66]
│  │  │  └──────113,664 B (00.02%) ── property-tables [66]
The sandbox's vars from the gc log. This is matches up quite nicely with https://github.com/mozilla/addon-sdk/blob/f5fab7b242121dccfa4e55ac80489899bb9f2a41/lib/sdk/event/dom.js 

000001F1DD2B94C0 B Sandbox 1f1dce50278
> 000001F1DD58A850 B group
> 000001F1DD5BF6C8 B shape
> 000001F1DD581E00 B CLASS_OBJECT(Date)
> 000001F1DD581B80 B CLASS_OBJECT(Math)
> 000001F1DD5A0DC0 B CLASS_OBJECT(Number)
> 000001F1DD5A0840 B CLASS_OBJECT(Int16Array)
> 000001F1DD5A0A80 B CLASS_OBJECT(Intl)
> 000001F1DD59D200 B **UNKNOWN SLOT 55**
> 000001F1DD581B00 B **UNKNOWN SLOT 56**
> 000001F1DD2BA640 B **UNKNOWN SLOT 57**
> 000001F1DD5637F0 B **UNKNOWN SLOT 74**
> 000001F1DD6A52E0 B **UNKNOWN SLOT 88**
> 000001F1DD581E00 B Object
> 000001F1DD581B80 B Function
> 000001F1DD5A0DC0 B Array
> 000001F1DD5A0A80 B Map
> 000001F1DD5A0500 B **UNKNOWN SLOT 152**
> 000001F1DD581B40 B **UNKNOWN SLOT 154**
> 000001F1DD5637C0 B **UNKNOWN SLOT 164**
> 000001F1DB733D60 B **UNKNOWN SLOT 165**
> 000001F1CC06E920 B **UNKNOWN SLOT 166**
> 000001F1E3950E20 B **UNKNOWN SLOT 183**
> 000001F1DD59D220 B **UNKNOWN SLOT 184**
> 000001F1DD6A53A0 B **UNKNOWN SLOT 189**
> 000001F1DD5A0500 B eval
> 000001F1DD5A0540 B XPCNativeWrapper
> 000001F1DD5A05C0 B dump
> 000001F1DD59D6C0 B emit
> 000001F1DD59D8C0 B unload
> 000001F1DD6A5320 B Promise
> 000001F1DC217040 B listeners
> 000001F1D9392C40 B getWindowFrom
> 000001F1DD5A0940 B removeFromListeners
> 000001F1DD5A0980 B open
> 000001F1DD59DAA0 B ShimWaiver
> 000001F1DD59D260 B protoAndIfaceCache[i]
> 000001F1DD6A5320 B protoAndIfaceCache[i]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: mlk
Whiteboard: [MemShrink]
Irakli, do you know who can take a look at this?
Blocks: sdk-memory-leaks
Flags: needinfo?(rFobic)
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch bug-1268898.patch (obsolete) — — Splinter Review 
Since this is the last remaining GC edge making it weak should do the job.
Attachment #8768837 - Flags: review?
You need to ask a specific person for review. Looking at the hg blame for the file you are modifying might give you some hints about who to ask.
Attachment #8768837 - Flags: review? → review?(gkrizsanits)
I'm also about to post some patches in bug 1267693 that might help here.
Attachment #8768837 - Flags: review?(gkrizsanits) → review+
Assignee: nobody → bugzilla.mozilla.org
Attached patch bug1268898.patch (obsolete) — — Splinter Review 
Fixed the patch to be based off m-c and added a commit message.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13f14feac424
Attachment #8768837 - Attachment is obsolete: true
Attachment #8769489 - Flags: review+
New patch.

Bug 1221292 moved the weak map key enumeration to ThreadSafeChromeUtils but MDN didn't mention that.
Attachment #8769499 - Flags: review?(gkrizsanits)
Attachment #8769489 - Attachment is obsolete: true
Attachment #8769499 - Flags: review?(gkrizsanits) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f4a3b8f1d56d
Fix leak in sdk/event/dom. r=gabor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4a3b8f1d56d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: needinfo?(rFobic)
If you'd like to uplift this to Aurora, go to "details" for the patch, set approval‑mozilla‑aurora to ?, and fill out the form. Thanks for fixing this!
Comment on attachment 8769499 [details] [diff] [review]
Bug-1268898-Fix-window-leak.patch

Approval Request Comment
[Feature/regressing bug #]: Addon-sdk which is used both internally to firefox and in addons.
[User impact if declined]: Memory leaks degrading performance over time.
[Describe test coverage new/current, TreeHerder]: existing functionality is tested in addon-sdk/test/test-event-dom.js
[Risks and why]: Should be minimal. Public APIs are not affected and the altered code only runs on addon unload. 
[String/UUID change made/needed]: No

This should go along with the uplift in bug 1267693. Based on some gc/cc logs I've looked at some leaks require both fixes.
Attachment #8769499 - Flags: approval-mozilla-aurora?
Comment on attachment 8769499 [details] [diff] [review]
Bug-1268898-Fix-window-leak.patch

Fix leak, taking it.
Attachment #8769499 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: