687777 - Context menu items added by extension use 900KB each; not returned on destroy()

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

From http://forum.agilebits.com/index.php?/topic/8962-firefox-right-click/page__hl__firefox__fromsearch__1

[quote]
We recently pushed a new update to the Firefox beta extension which should resolve some performance issues.

We managed to narrow the problem down to the context menus. As far as we can tell, we did everything properly but Mozilla has a horrible memory leak. We properly call destroy(), which states "Permanently removes the item from its parent menu and frees its resources.", but apparently this is not happening. We even tried nulling the menu item afterward: no effect.

We calculated that each Context Menu item in Firefox requires 900K. Nearly a meg for a simple menu item with just a name and action! We have been forced to remove Context Menus from Firefox until this problem is resolved. :( 
[end quote]

They may be doing something wrong, of course, though they believe they aren't.  The two memshrink issues here are 1) why does it use 900KB for a menu item, and 2) why doesn't it come back.

Note I don't have a mac so I can't test it.

Comment 1

[(no longer active)]

I just sent a message to khad in the forum asking for some more information about this.

Comment 2

[khad]

"Good news, everyone." —  Hubert J. Farnsworth

https://wiki.mozilla.org/Labs/Jetpack/Weekly_Meeting/2011-09-20#Minutes

It looks like "Jeff is helping author of 1password isolate memory problem in context-menu; gabor, alexp may help investigate".

Comment 3

[(no longer active)]

Attachment: Sample add-on to reproduce the bug

Comment 4

[(no longer active)]

I can totally reproduce this.  This bug is caused by the context-menu module in the add-on sdk.  See attachment 564658 [details] for an add-on which reproduces this bug (here's the source code: https://builder.addons.mozilla.org/addon/1018952/latest/).

Myk, do you know who can look into this?  It seems like the add-on SDK runtime is holding to some js objects...

Comment 5

[Myk Melez [:myk] [@mykmelez]]

cc:ing Gabor and Alex, who are looking into the issue.  Also cc:ing Drew, who authored the context-menu module and may have some insight.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Gabor, you said you might be able to look at this?

Comment 8

[Gabor Krizsanits (INACTIVE)]

I'm a bit flooded with tasks right now, will do some prioritization today and in worst case I'll pass it to Eddy.

Comment 9

[Gabor Krizsanits (INACTIVE)]

Ok, so I did some measurements on this. My test was to open up a blank page with the addon, then open another blank tab, then shutting down the whole browser. Compared two cases, in the first adding 40 in the second run adding 160 context menu items. And picked only the type of object, where the count of created instances showed a big difference.

Identity                     214  694
ObjectEntry                  534  935
PtrAndPrincipalHashKey       746  2187
XPCNativeInterface           350  395
XPCNativeMember            22184  31362
XPCNativeScriptableInfo     1851  2372
XPCNativeScriptableShared   1926  2452
XPCNativeSet                 905  1427
XPCVariant                    69  73
XPCWrappedNative           13293  19161
XPCWrappedNativeProto       1270  1776
XPCWrappedNativeScope        286  767
nsDOMEvent                   388  500
nsDOMMouseEvent               88  184
nsDOMNotifyPaintEvent         18  34 
nsDOMTokenList               165  645
nsDOMUIEvent                  92  188
nsDeque                      312  2707
nsDisplayBackground         1797  6054  
nsDisplayBorder              190  427
nsDisplayBoxShadowInner      102  199
nsDisplayBoxShadowOuter      102  199
nsDisplayCaret                 9  18
nsDisplayClip                561  1746
nsDisplayListBuilder (816)   143  465
nsDisplayOpacity              34  70
nsDisplayOwnLayer             56  273
nsDisplaySVGEffects           17  32
nsDisplayText                 19  34
nsDisplayXULImage            273  538
nsDisplayXULTextBox           40  82
nsEvent                      741  1155
nsJSID                      3270  5676
nsNSElementTearoff           513  1953
nsNodeSupportsWeakRefTearoff 102  241
nsNodeWeakReference           87  156
nsRect                    274989  625445
nsRenderingContext            75  90
nsRuleWalker                5001  9130
nsRunnable                  1348  1894
nsStyleContext              3027  7121
nsStyleDisplay              2140  6079
nsStyleUserInterface        2877  6943
nsTArray_base              74878  113281
nsTimerEvent                  92  147
nsTimerImpl                  230  632
nsWeakReference              414  898
nsXPCComponents              357  839
nsXPCWrappedJS              2217  4142
nsXULElement                1834  2314
nsXULElementTearoff          138  480
txAExprResult                160  317
xpc::CompartmentPrivate      220  700
xpcJSWeakReference           167  647
xptiInterfaceInfo           2424  2931 

I see multiple things here, one that I started with was that there seem to be a LOT of sandbox created. So I checked the context-menu.js and logged how many times do we create a new worker in _makeWorker (each worker created a sandbox!) and the numbers were shocking. So for example in the 160 context menu item case when I open up the browser (with cfx run), for the first blank page there is no context menu added (for some reason) but when I open the second tab 641 worker (so sandbox and other things) created for the third tab it goes up to 1611... I don't know how many of these sandboxes are destroyed but it just seems really bad to me. For the single menu item case the numbers are (1 tab:)0 (2nd tab:)9 (3rd tab:)23
2nd: url:about:blank 4 + url:fillLogin 5 
3rd: fillLogin 7 + fillLogin 7 

Something is really fishy here on the javascript side, can someone take a look at this?

Comment 10

[Drew Willcoxon :adw]

Attachment: patch

I think there are several issues.  This patch addresses the first three.

1) Workers are only created for top-level menu items.  When a top-level item is later added to a Menu, its workers are destroyed.  Well, all items start out as top-level because there is, unfortunately, no contextMenu.add().  So for items that are immediately added to Menus, their workers are needlessly created and then immediately destroyed.

2) When a Menu is destroy()ed, context-menu deletes its reference to the Menu's DOM element but not to the DOM elements of its child items.  That's a bug.

3) When you open a tab, a worker is needlessly created and destroyed per menu item.  content-document-global-created is fired twice.  The first time doc.readyState is "uninitialized", the second time it's "loading".  Each time the inner window ID is different.  So _onDocGlobalCreated creates two workers.  The first worker is immediately destroyed because inner-window-destroyed is fired for the first inner window ID.  (I don't recall this content-document-global-created behavior when I wrote context-menu.)

4) context-menu uses lots of workers.  It's a worker per content window per top-level menu item.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Drew Willcoxon :adw from comment #10)
 
> 4) context-menu uses lots of workers.  It's a worker per content window per
> top-level menu item.

It's not entirely clear for me what are those workers do, but would it be possible to share some of those between menu items? Per addon, per principal or some other way.

Comment 12

[Drew Willcoxon :adw]

They let each menu item talk to each page, just like page mods talk to each page.  context-menu uses no more workers than page-mod does: a worker per menu item per content window, a worker per page mod per content window (unless, in both cases, the user specifies URL match patterns .  Does page-mod share workers?

Comment 13

[Gabor Krizsanits (INACTIVE)]

Ok. So I misunderstood it. I thought one worker per menu item per content window, that's why I wanted to share workers, but it's per top-level menu item. Meaning in this example regardless of how many menu item I add, it will create only 1 worker per content window. This patch seem to solve the problem, only thing I don't get why, do we need to destroy all the workers and recreate them every time I open a new tab. Can't we just keep the old workers and create a worker for the new content window only? Does it not leak because of this behavior (just _much_ less than it did earlier)?

Comment 14

[Drew Willcoxon :adw]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> This patch seem to solve the problem, only thing I don't get
> why, do we need to destroy all the workers and recreate them
> every time I open a new tab. Can't we just keep the old workers
> and create a worker for the new content window only? Does it
> not leak because of this behavior (just _much_ less than it did
> earlier)?

In the context of the add-on in comment 4, right?  That add-on calls destroy() on its menu item each time you open or select a tab.  With the patch, that causes all menu items to be destroyed -- the top-level fillLoginContextMenuItem and all its child items -- but the only workers that are destroyed are the workers for fillLoginContextMenuItem, since it's the only menu item that has workers.  Unless there are more bugs, that shouldn't leak, because the objects created are destroyed.

I don't know what the original code was trying to do, but recreating menu items each time a tab is opened or selected is pretty inefficient.  It causes DOM elements to be recreated, and for top-level items it also causes workers to be recreated.  If the add-on needs to change its submenu, it could at least assign a new items array to fillLoginContextMenuItem rather than recreating it.  That would still destroy all the old submenu items, but it wouldn't destroy the top-level item and its workers.  But a better choice would be to create a different top-level item per context, each with a submenu tailored to that context.  Then you don't have to recreate anything or listen for tab events.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Thanks. I'm not sure either, but the author of the addon who reported this issue, and provided this example as well, wanted to know if they are doing something wrong or not. So I can at least send him your answer, or a link to this bug.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 567023 [details] [diff] [review]
patch

>+    require("timer").setTimeout(function AIT__finishActiveItemInit() {

Nit: since there are two references to the `timer` module, it's worth factoring them out to a `timer` const defined at the top of the script.

Otherwise looks good, r=myk.

Comment 17

[Drew Willcoxon :adw]

https://github.com/mozilla/addon-sdk/commit/5d2d9043babe08bffcded115fb5ad623b42e24f8