936106 - leak when loading panels from jetpacks

Status: RESOLVED INVALID

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

Description

[desiradaniel2007]

I have written a Jetpack to provide quick access to the Playground on typescriptlang.org and noticed that after 3 hours not launching the panel, memory for the TypeScript compiler is not released. (through about:memory)

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi desireadaniel2007, Can you provide a test case or attach more information? Thanks!

Comment 2

[desiradaniel2007]

https://builder.addons.mozilla.org/package/187507/latest/

Let me know if you need more feedback

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

OK. I'm not sure how to test this. Jason, does this look like the right component for this bug? Thanks!

Comment 4

[desiradaniel2007]

(In reply to Liz Henry :lizzard from comment #3)
> OK. I'm not sure how to test this. Jason, does this look like the right
> component for this bug? Thanks!

If you ask me, it looks more like a bug in Jetpack not de-allocating memory when a panel is no longer shown. However, I believe I could do something like Panel.destroy() on my side. Should I let you know how that does?

Comment 5

[Jason Orendorff [:jorendorff]]

Hi desireadaniel2007, and thanks for the bug report. Does adding this work around the problem?

    tsPanel.on("hide", function () { tsPanel.destroy(); });

If so, then this is almost certainly an Addon SDK bug, and there's a fairly straightforward to fix it:

    1.  Find out exactly which step in tsPanel.destroy()
        causes the document to be freed up
        (probably unhooking an event listener from somewhere).

    2.  Do that step whenever the popup is hidden, and then put it back
        whenever the popup is shown.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

So, the panel is not destroyed when is hidden on purpose, because it needs to keep the state. To be more precise, the document inside the panel is kept alive, swapped when the XUL panel is not displayed, in order to keep the state.
You can destroy the panel when you do not need it anymore, using the `destroy` method. Notice that this will free the memory but also destroy the panel, so sub sequential calls like `panel.show` won't work.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)
> So, the panel is not destroyed when is hidden on purpose, because it needs  
> to keep the state.

I don't understand XUL too well, but surely no state needs to be kept after the panel is hidden and there are no remaining references to it.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Jason Orendorff [:jorendorff] from comment #7)

> > So, the panel is not destroyed when is hidden on purpose, because it needs  
> > to keep the state.

> I don't understand XUL too well, but surely no state needs to be kept after
> the panel is hidden and there are no remaining references to it.

Well, that's the point: XUL panel doesn't do that, but SDK panel yes: it was a requirement during the API design.
So the XUL panel is properly destroyed and released, but the HTML document loaded in the frame contained by the panel is kept alive in order to be reused when the panel is displayed again.
In this way the state is kept across window and when is displayed on the same window after it was hidden, and the changes made to the document are not lost.

In short, is not a memory leak neither a bug. You can release the HTML document calling explicitly `panel.destroy`, however in that case you need to instantiate again before showing.

If you don't need to keep the document alive, and the document originally loaded in the panel consume too much memory, you can change the `contentURL` property of the panel dynamically with something different, like `about:blank`; and then set it again to the previous URL when you need it.

Comment 9

[desiradaniel2007]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Hi desireadaniel2007, and thanks for the bug report. Does adding this work
> around the problem?
> 
>     tsPanel.on("hide", function () { tsPanel.destroy(); });
> 
> If so, then this is almost certainly an Addon SDK bug, and there's a fairly
> straightforward to fix it:
> 
>     1.  Find out exactly which step in tsPanel.destroy()
>         causes the document to be freed up
>         (probably unhooking an event listener from somewhere).
> 
>     2.  Do that step whenever the popup is hidden, and then put it back
>         whenever the popup is shown.

Yes, I thought that would be available and it worked.

I believe it is the extension developer's responsibility to clean up when going heavy on resources, but couldn't the SDK do some heuristics? Maybe we should look at how other browsers would deal with this :-)

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> Well, that's the point: XUL panel doesn't do that, but SDK panel yes: it was
> a requirement during the API design.

It makes sense to retain the document if it could be displayed again. I get that. As long as the Panel object is alive, it should keep the document alive. Fine.

But here I don't see who has a reference to the Panel, the XUL popup, or the document.

The point of GC is to be able to reclaim such unreachable object graphs, no matter how much they may refer to each other internally. Our GC can do this; what reference, exactly, is keeping the document alive?

> So the XUL panel is properly destroyed and released, but the HTML document
> loaded in the frame contained by the panel is kept alive in order to be
> reused when the panel is displayed again.

*Can* it ever be displayed again? How? Nobody should have a reference to it. Even the event listeners attached to it for show/hide events are all weak. The Panel keeps them alive, not the other way round.

> In short, is not a memory leak neither a bug.

I remain convinced of the exact opposite! Pistols at dawn!  :)

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)

> > Well, that's the point: XUL panel doesn't do that, but SDK panel yes: it was
> > a requirement during the API design.
> 
> It makes sense to retain the document if it could be displayed again. I get
> that. As long as the Panel object is alive, it should keep the document
> alive. Fine.

Once a SDK panel is created and the document loaded, until the add-on is disabled / removed, or the SDK panel explicitly destroyed via `panel.destroy`, the panel object is alive. And therefore the document is alive too.

> The point of GC is to be able to reclaim such unreachable object graphs, no
> matter how much they may refer to each other internally. Our GC can do this;
> what reference, exactly, is keeping the document alive?

I believe is the background frame that consumes memory: so basically we have two frame per panel, and we "swap" them when we want to display the panel, in order to keep the state. When the panel is not shown, the document is "hosted" by an iframe in the `hiddenWindow`.

> *Can* it ever be displayed again? How? Nobody should have a reference to it.

In that specific case could be displayed again only via accessing directly to the DOM, I believe.
The nodes are still in the DOM, nothing is removed by GC I guess, therefore both panel and document are still alive; what will be destroyed by GC will be only the add-on sdk js layer to access to them (the "model", let's say. The "view" are not removed).

> > In short, is not a memory leak neither a bug.

> I remain convinced of the exact opposite! Pistols at dawn!  :)

I'm open to consider that, maybe I misunderstood something. :)

Comment 12

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

It sounds like things are working as intended.