833783 - Opening and closing browser windows seems to add FragmentOrElement objects to the CC graph when Addon SDK addons are installed which add an addonbar widget

Status: RESOLVED FIXED

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

Description

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

Install memchaser and the addon from bug 726346.

STR:
Start FF
Load about:cc
  - run CC and check whether there are FragmentOrElement objects in the graph
    (usually there are non right after startup). Check also how many objects there are,
    something like 200 is expected.
Open a new window (ctrl+n)
Close the new window (ctrl+w)
Run CC few times until the number of objects in the graph stabilizes
Look for FragmentOrElement objects.
  - I see objects like
    root FragmentOrElement (XUL) commandset id='editMenuCommandSetAll' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) commandset id='placesCommands
    FragmentOrElement (XUL) menupopup id='appmenu-popup' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) toolbar id='toolbar-menubar' class='chromeclass-menubar' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) toolbarbutton id='appmenu-toolbar-button' class='chromeclass-toolbar-additional' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) hbox chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) commandset id='mainCommandSet' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) vbox id='appmenuPrimaryPane' chrome://browser/content/browser.xul
    root FragmentOrElement (XUL) vbox id='appmenuSecondaryPane' chrome://browser/content/browser.xul

I tried to reproduce this without memchaser, and couldn't see new elements.
(randomly just 2 XBL elements related to about:cc)

I don't know whether this is a FF, Addon SDK or memchaser bug.

Comment 1

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

Maybe this is the same as Bug 751557

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I also thought about that. Shall I do a test with both addons installed and the Jenkins webpage? I still see those leaks. Will that work with Aurora too?

Comment 3

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

I saw this with FF18 too, so Aurora should be ok.
But anyhow, the leak is rather bad. Would be good if someone from AddonSDK team could look at this.
(unless it is a memchaser bug.)

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

None of the above entries have been added by myself for Memchaser. If those are leaking they are coming in through the Add-ons SDK.

For now I would mark this bug as blocking bug 751557, because we leak a lot more on the Jenkins website. We should revisit the other bug once the basic steps don't produce any leaks anymore.

Comment 5

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

Attachment: simple addon

This is super simple addon canuckistani	created.
Just installing this is enough to get the leaks.

Comment 6

[Alexandre Poirot [:ochameau]]

Is there any way to filter JS object for a given compartment/sandbox/principal/global?
Current ccdump output are really hard to interpret.
If that's an SDK bug, we are most likely leaking a DOM listener or a reference to some browser window chrome node/object.
It would be easier to figure out what's wrong if you could filter objects rooted to a given SDK sandbox or a given chrome window. SDK loads each module in its own sandbox.

Comment 7

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

CC logs may not be enough for this. GC log is possibly useful too
https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump

One thing to check, is this runtime leak only, or is the leak there still on shutdown
(use debug build and XPCOM_MEM_LEAK_LOG env variable).
Runtime leaks are often easier to analyze. Take the address of leaked object (for example from
about:cc) and then in C++ debugger add break point to that object's Release and check what all ends
up releasing the object during shutdown.

Comment 8

[Alexandre Poirot [:ochameau]]

Mossop, I tried to test this leak against your leak detector branch with such example: http://pastebin.mozilla.org/2092115
But it doesn't detect any leak. I was expecting to see a chrome browser compartment leak. But looking at about:compartments, it seems that we are only leaking one compartment each time we open a new window. An about:blank system principal compartment.

Comment 9

[Alexandre Poirot [:ochameau]]

Hum I'm not used to XPCOM_MEM_LEAK_LOG, it doesn't seem to be working:

$ XPCOM_MEM_LEAK_LOG=leaks.log ./firefox -profile profile-with-leaky-addon

--- open a new window, close it and then File > Quit --

$ cat leaks.log
  == BloatView: ALL (cumulative) LEAK STATISTICS, default process 31672
  nsTraceRefcntImpl::DumpStatistics: 877 entries
EOF!!

My mozconfig is extremely simple:
  ac_add_options --enable-debug
  ac_add_options --enable-trace-malloc

Comment 10

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

I have just export XPCOM_MEM_LEAK_LOG=1 so that I get the leak log to stdout.
It is basically just a list of refcounted objects which stayed alive even after shutdown process.

Anyhow, I don't see any shutdown leaks.

Comment 11

[Alexandre Poirot [:ochameau]]

Here is the stack on shutdown:

#0  mozilla::dom::FragmentOrElement::Release (this=0x34e7520)
    at /mnt/desktop/gecko/content/base/src/FragmentOrElement.cpp:1681
#1  0x00007f662388fbf1 in nsXMLElement::Release (this=0x34e7520)
    at /mnt/desktop/gecko/content/xml/content/src/nsXMLElement.cpp:32
#2  0x00007f6623418a4c in nsCOMPtr<nsIContent>::Assert_NoQueryNeeded (this=0x7fff2f16df38)
    at ../../dist/include/nsCOMPtr.h:504
#3  0x00007f66238959d1 in nsXMLContentSink::HandleStartElement (this=0x2867520, aName=<optimized out>, 
    aAtts=0x2f980c0, aAttsCount=2, aIndex=-1, aLineNumber=539, aInterruptable=true)
    at /mnt/desktop/gecko/content/xml/document/src/nsXMLContentSink.cpp:993
#4  0x00007f6623895d1c in nsXMLContentSink::HandleStartElement (this=<optimized out>, aName=<optimized out>, 
    aAtts=<optimized out>, aAttsCount=<optimized out>, aIndex=<optimized out>, aLineNumber=<optimized out>)
    at /mnt/desktop/gecko/content/xml/document/src/nsXMLContentSink.cpp:948
#5  0x00007f66233c38dc in nsExpatDriver::HandleStartElement (this=0x31b2130, aValue=0x1dfd940, aAtts=0x2f980c0)
    at /mnt/desktop/gecko/parser/htmlparser/src/nsExpatDriver.cpp:385
#6  0x00007f6624581a7c in doContent (parser=0x2c95860, startTagLevel=0, enc=0x7f6625ba8af0, s=0x38bca6a "<", 
    end=0x38bd9e6 "", nextPtr=<optimized out>, haveMore=1 '\001')
    at /mnt/desktop/gecko/parser/expat/lib/xmlparse.c:2413
#7  0x00007f6624582282 in contentProcessor (parser=0x2c95860, start=<optimized out>, end=<optimized out>, 
    endPtr=<optimized out>) at /mnt/desktop/gecko/parser/expat/lib/xmlparse.c:2043
#8  0x00007f662458393a in MOZ_XML_ParseBuffer (parser=0x2c95860, len=<optimized out>, isFinal=0)
    at /mnt/desktop/gecko/parser/expat/lib/xmlparse.c:1622
#9  0x00007f6624583d3c in MOZ_XML_Parse (parser=0x2c95860, s=0x239ee00 "1", len=8192, isFinal=0)
    at /mnt/desktop/gecko/parser/expat/lib/xmlparse.c:1593
#10 0x00007f66233c2dc8 in nsExpatDriver::ParseBuffer (this=0x31b2130, aBuffer=0x239ee00, aLength=4096, 
    aIsFinal=false, aConsumed=0x7fff2f16e334) at /mnt/desktop/gecko/parser/htmlparser/src/nsExpatDriver.cpp:996
#11 0x00007f66233c4919 in nsExpatDriver::ConsumeToken (this=0x31b2130, aScanner=..., 
    aFlushTokens=<optimized out>) at /mnt/desktop/gecko/parser/htmlparser/src/nsExpatDriver.cpp:1094
#12 0x00007f66233d269d in nsParser::Tokenize (this=0x2867400, aIsFinalChunk=false)
    at /mnt/desktop/gecko/parser/htmlparser/src/nsParser.cpp:2044
#13 0x00007f66233d31ef in nsParser::ResumeParse (this=0x2867400, allowIteration=true, aIsFinalChunk=false, 
    aCanInterrupt=<optimized out>) at /mnt/desktop/gecko/parser/htmlparser/src/nsParser.cpp:1534
#14 0x00007f66233d14a1 in nsParser::OnDataAvailable (this=0x2867400, request=0x3c6af50, 
    aContext=<optimized out>, pIStream=<optimized out>, sourceOffset=<optimized out>, aLength=45718)
    at /mnt/desktop/gecko/parser/htmlparser/src/nsParser.cpp:1926
#15 0x00007f662369b1da in nsDOMParser::ParseFromStream (this=<optimized out>, stream=0x2ec4b78, 
    charset=0x7f66238994c4 "UH\211\345AWM\211\307AVAUATI\211\324SH\211\373H\201\354", <incomplete sequence \370>, contentLength=45718, contentType=<optimized out>, aResult=0x7fff2f16e740)
    at /mnt/desktop/gecko/content/base/src/nsDOMParser.cpp:278
#16 0x00007f662369b3a8 in nsDOMParser::ParseFromStream (this=0x2f8ffa0, aStream=0x41eaad8, aCharset=..., 
    aContentLength=<optimized out>, aType=<optimized out>, rv=...)
    at /mnt/desktop/gecko/content/base/src/nsDOMParser.cpp:178
#17 0x00007f6624214d47 in mozilla::dom::DOMParserBinding::parseFromStream (cx=0xec7660, obj=<optimized out>, 
    self=0x2f8ffa0, argc=<optimized out>, vp=0x7f660cd11380)
    at /mnt/desktop/gecko/obj-x86_64-unknown-linux-gnu/dom/bindings/DOMParserBinding.cpp:263
#18 0x00007f6624214123 in mozilla::dom::DOMParserBinding::genericMethod (cx=0xec7660, argc=4, 
    vp=0x7f660cd11380) at /mnt/desktop/gecko/obj-x86_64-unknown-linux-gnu/dom/bindings/DOMParserBinding.cpp:398
#19 0x00007f662488e16a in js::CallJSNative (cx=0xec7660, 
    native=0x7f6624213fbc <mozilla::dom::DOMParserBinding::genericMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /mnt/desktop/gecko/js/src/jscntxtinlines.h:364
#20 0x00007f66248a2e3b in js::InvokeKernel (cx=0xec7660, args=..., construct=js::NO_CONSTRUCT)
    at /mnt/desktop/gecko/js/src/jsinterp.cpp:389
#21 0x00007f66248990cc in js::Interpret (cx=0xec7660, entryFrame=0x7f660cd111a0, 
    interpMode=js::JSINTERP_NORMAL) at /mnt/desktop/gecko/js/src/jsinterp.cpp:2341
#22 0x00007f66248a2ae5 in js::RunScript (cx=0xec7660, script=..., fp=0x7f660cd111a0)
    at /mnt/desktop/gecko/js/src/jsinterp.cpp:346
#23 0x00007f66248a2fb3 in js::InvokeKernel (cx=0xec7660, args=..., construct=js::NO_CONSTRUCT)
    at /mnt/desktop/gecko/js/src/jsinterp.cpp:404
#24 0x00007f66248a3afd in Invoke (args=..., cx=0xec7660, construct=<optimized out>)
    at /mnt/desktop/gecko/js/src/jsinterp.h:112
#25 js::Invoke (cx=0xec7660, thisv=..., fval=..., argc=<optimized out>, argv=0x7fff2f16fc00, 
    rval=0x7fff2f16f898) at /mnt/desktop/gecko/js/src/jsinterp.cpp:437
#26 0x00007f66247c37ec in JS_CallFunctionValue (cx=0xec7660, objArg=0x7f65fe18f200, fval=..., argc=1, 
    argv=0x7fff2f16fc00, rval=0x7fff2f16f898) at /mnt/desktop/gecko/js/src/jsapi.cpp:5786
#27 0x00007f6623d32643 in nsXPCWrappedJSClass::CallMethod (this=0x1862ea0, wrapper=<optimized out>, 
    methodIndex=3, info_=0xc07678, nativeParams=0x7fff2f16fd88)
    at /mnt/desktop/gecko/js/xpconnect/src/XPCWrappedJSClass.cpp:1432
#28 0x00007f6623d2a8db in nsXPCWrappedJS::CallMethod (this=0x10ce3a0, methodIndex=3, info=0xc07678, 
    params=<optimized out>) at /mnt/desktop/gecko/js/xpconnect/src/XPCWrappedJS.cpp:580
#29 0x00007f66242e4550 in PrepareAndDispatch (self=0x1ce8eb0, methodIndex=<optimized out>, 
    args=<optimized out>, gpregs=0x7fff2f16fe60, fpregs=0x7fff2f16fe90)
    at /mnt/desktop/gecko/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
#30 0x00007f66242e3913 in SharedStub () from /mnt/desktop/gecko/obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so
#31 0x00007f66242e3878 in NS_InvokeByIndex_P (that=<optimized out>, methodIndex=<optimized out>, 
    paramCount=<optimized out>, params=<optimized out>)
    at /mnt/desktop/gecko/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:164
#32 0x00007f6623d3a70a in Invoke (this=0x7fff2f170010)


I hope it tells you something, because I don't really know what to think with such stracktrace :o

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 765

So we were having two leaks related to the action to open more than one top-level browser window and closing one of them.
We were keeping a reference to a tab and another one to the top-level window object so that it was preventing from freeing the whole closed browser window and all its related object and data. Hopefully, content tabs weren't leaking thanks to the khuey nuke feature but I'm expecting this leak to be quite important for users using multiple browser windows.

Addons being impacted are those using windows and/or tabs API directly and indirectly. This bug report highlights that some other high level API (here Widget) depend on those modules.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Fixed simple addon

Here is a similar simple addon, but with the fix included.

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/15a436fee6a11436082bf777ed59a7edda2e3264
Bug 833783: Fix leaks in tabs and windows when closing a top-level chrome window.

https://github.com/mozilla/addon-sdk/commit/b06eb5ba5d95fa7c2ccfb916ed7a4bc388579a9a
Merge pull request #765 from ochameau/bug/833783

Fix Bug 833783: Fix leaks in tabs and windows when closing a top-level chrome window. r=@erikvold

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 781

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 712400 [details]
Pull request 781

note this would close bug 839990

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d4556ce2521960a009c5051b91132de9f6ed9b0f
Bug 833783: followup, fix exception when calling destroy multiple times.

https://github.com/mozilla/addon-sdk/commit/435be058081ce064eaa574e3d0d2f1a83e29fca7
Merge pull request #781 from ochameau/destroy-followup@833783

Bug 833783: followup, fix exception when calling destroy multiple times. r=@erikvold

Comment 18

[[github robot]]

Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1c315cb1e8cc3ecd5cb0ac34fceda9fa73b41630
Bug 833783: followup, fix exception when calling destroy multiple times.
(cherry picked from commit d4556ce2521960a009c5051b91132de9f6ed9b0f)

Comment 19

[[github robot]]

Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b439ee918cc5132d2cced8c536a46790f9794b3c
Merge pull request #765 from ochameau/bug/833783

Fix Bug 833783: Fix leaks in tabs and windows when closing a top-level chrome window. r=@erikvold(cherry picked from commit b06eb5ba5d95fa7c2ccfb916ed7a4bc388579a9a)

Comment 20

[[github robot]]

Commits pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f98f3279e77e2eee835f9661cf87c8b286dc4278
Merge pull request #765 from ochameau/bug/833783

Fix Bug 833783: Fix leaks in tabs and windows when closing a top-level chrome window. r=@erikvold(cherry picked from commit b06eb5ba5d95fa7c2ccfb916ed7a4bc388579a9a)
(cherry picked from commit b439ee918cc5132d2cced8c536a46790f9794b3c)

Conflicts:

	lib/sdk/tabs/tab-firefox.js

https://github.com/mozilla/addon-sdk/commit/38fdb1af4073b4e2e0bf36c5af09db38c496b4c8
Bug 833783: followup, fix exception when calling destroy multiple times.
(cherry picked from commit d4556ce2521960a009c5051b91132de9f6ed9b0f)
(cherry picked from commit 1c315cb1e8cc3ecd5cb0ac34fceda9fa73b41630)

Comment 21

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Update, SDK 1.13.2 has been released:

https://blog.mozilla.org/addons/2013/02/12/announcing-add-on-sdk-1-13-2/

For the curious, once the AMO validator is updated I have proposed to the amo editors that we require add-ons be re-packed to 1.13.2 in order to get full review.

Comment 22

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

Tested with the latest memchaser and this leak is now gone. Thanks.

Comment 23

[[github robot]]

Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/15a436fee6a11436082bf777ed59a7edda2e3264
Bug 833783: Fix leaks in tabs and windows when closing a top-level chrome window.

https://github.com/mozilla/addon-sdk/commit/b06eb5ba5d95fa7c2ccfb916ed7a4bc388579a9a
Merge pull request #765 from ochameau/bug/833783

https://github.com/mozilla/addon-sdk/commit/d4556ce2521960a009c5051b91132de9f6ed9b0f
Bug 833783: followup, fix exception when calling destroy multiple times.

https://github.com/mozilla/addon-sdk/commit/435be058081ce064eaa574e3d0d2f1a83e29fca7
Merge pull request #781 from ochameau/destroy-followup@833783

Comment 24

[[github robot]]

Commits pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f98f3279e77e2eee835f9661cf87c8b286dc4278
Merge pull request #765 from ochameau/bug/833783

https://github.com/mozilla/addon-sdk/commit/38fdb1af4073b4e2e0bf36c5af09db38c496b4c8
Bug 833783: followup, fix exception when calling destroy multiple times.