Last Comment Bug 764831 - SDK is leaking on require("context-menu").Item({label:"foo"});
: SDK is leaking on require("context-menu").Item({label:"foo"});
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: 1.10
Assigned To: Alexandre Poirot [:ochameau] PTO, back on 1st
:
Mentors:
: 779224 (view as bug list)
Depends on: 781619 784116
Blocks: 728589 780391
  Show dependency treegraph
 
Reported: 2012-06-14 08:23 PDT by Alexandre Poirot [:ochameau] PTO, back on 1st
Modified: 2012-09-05 01:58 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Pull request 475 (165 bytes, text/html)
2012-07-02 10:54 PDT, Alexandre Poirot [:ochameau] PTO, back on 1st
rFobic: review+
Details

Description Alexandre Poirot [:ochameau] PTO, back on 1st 2012-06-14 08:23:02 PDT
Yet another leak has been identified on context-menu on addon disabling.
We are leaking all modules when a module is using `Item` class from context-menu.

  var cm = require("context-menu");
  cm.Item({
    label: "Go to Resolved Link"
  });

I'm checking only on master, the leak may be different with older SDK version.
Comment 1 Justin Lebar (not reading bugmail) 2012-06-14 08:29:09 PDT
So web content never leaks here -- the issue is that, if the add-on uses a context-menu item, the add-on's modules will stick around even after the add-on is disabled?
Comment 2 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-06-14 08:34:43 PDT
(In reply to Justin Lebar [:jlebar] from comment #1)
> So web content never leaks here -- the issue is that, if the add-on uses a
> context-menu item, the add-on's modules will stick around even after the
> add-on is disabled?

Yes, that's exactly it.
Comment 3 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-06-14 09:21:10 PDT
A first improvement could be to avoid leaking all modules, but only context-menu related ones. See bug 764866.
Comment 4 Wes Kocher (:KWierso) 2012-06-14 11:37:53 PDT
Can we test on stabilization and see if it occurs there too?
Comment 5 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-07-02 10:54:23 PDT
Created attachment 638414 [details]
Pull request 475

This leak is due to some leak in content-proxy.js.

We are leaking JS proxies between the time we are destroying the content script and the time of the destruction of the content document.
When the related content document is destroyed, we free everything.
So that if you close all tabs opened when the addon was active (including about:addons!), we do not leak addons compartments.

In most cases, we destroy the content script when the related document is destroyed. But not in case of addon disabling ... So it is worth fixing this.

The fix itself is quite easy. Instead of storing the JS proxy on the xraywrapper itself, we use a WeakMap.
Comment 6 [github robot] 2012-07-07 15:23:12 PDT
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/7482b9b05283e594b6763beda520839a005f816a
Bug 764831: Avoid leaking JS proxies on addon disabling.

https://github.com/mozilla/addon-sdk/commit/983a9efe8ea2decc1f1e594799f4c0eaea5b1efb
Merge pull request #475 from ochameau/bug/764831-avoid-leaking-js-proxies

Bug 764831: Avoid leaking JS proxies on addon disabling. r=@gozala
Comment 7 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-07-07 15:30:15 PDT
It would be cool to get this fix pushed into stabilization. It isn't a severe leak as related objects are freed whent the tab is closed, but it may help investigate on more important leaks if we aren't hitting this one at first place.
Comment 8 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-08-20 11:09:31 PDT
Reverted per bug 781619 comment 11:
  https://github.com/mozilla/addon-sdk/commit/f3d13138dd1264a7efa507e7726ac9f1d9137f5a
Comment 9 Wes Kocher (:KWierso) 2012-08-20 11:41:55 PDT
Cherry-picked to stabilization: 
https://github.com/mozilla/addon-sdk/commit/e2143125df0845b7e72cac5167d300a5ef406cbf
Comment 10 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-08-21 03:16:11 PDT
It appears to be a bit more harmfull than what I was thinking.
In case of page-mod, when we apply a content script on an iframe and access to  `window.parent`, we end up leaking iframe's content script until the parent document is destroyed.
So that, in bug 780391, wallflower addon is leaking all ecmascript test iframes until we close ecmascript tab.

That's because we cache JS proxies on the xraywrapper itself. The proxy and its sandbox are kept alive until the related xray is destroyed. In case of window.parent and any cross compartment usages, it ends up being quite bad behavior!

With Weakmap patch, wallflower isn't leaking. But Weakmap doesn't work on all kind of wrappers, so that the cache may loose some proxies. But from what I know it is the only way to avoid leaking here.
Comment 11 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-08-21 06:34:18 PDT
I tried to use Components.utils.getWeakReference without much success, it still leaks when trying to access window.parent while running ecmascript tests.

I'm not able to come up with any ideal fix. We either leak or miss some proxies instances from cache. Missing proxies from cache means that for the same webpage object, we will generate two distinct objects which will have different set of expendos.
From what I can see, here is the kind of object that can't be used in weakmaps: Storage, Events, Location, XPath objects, ImageData, ...
As Andrew said in bug 761620 comment 0, it works only for node wrappers.

When bug 761620 will land, we will be able to detect cases where weakmap fails to use the key and switch back to safe-leaky way of doing. But in the meantime I don't know exactly how to address this.
Comment 12 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-08-21 07:20:09 PDT
*** Bug 779224 has been marked as a duplicate of this bug. ***
Comment 13 Alexandre Poirot [:ochameau] PTO, back on 1st 2012-09-05 01:58:05 PDT
Actually this bug can be considered as fixed, thanks to bug 786976, and the removal of JS proxies.
And even if the original report mentioned context-menu, this leak was'nt related to context-menu, but worker code and all dependent APIs.

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