Issue 3224 - Unbreak filter assistant

Issue 3224 - Unbreak filter assistant

[tschuster, 2015-12-01 16:42:40]

https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
lib/child/contentPolicy.js:61: let storedNodes = new Map();
So, this is cleaned up by DeleteNodes, which is called when you close the UI,
right? Shouldn't we clean this up when the content navigates in the child? (I
assume we can't leak because of the DeadObjectProxy, but still something to
think about)

[Thomas Greiner, 2015-12-01 18:07:47]

Looks fine from my end. Just waiting for a response to Tom's comment.

[Wladimir Palant, 2015-12-01 19:15:31]

https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
lib/child/contentPolicy.js:61: let storedNodes = new Map();
On 2015/12/01 16:42:40, tschuster wrote:
> So, this is cleaned up by DeleteNodes, which is called when you close the UI,
> right? Shouldn't we clean this up when the content navigates in the child?

Nodes can be removed without navigating away (yes, we used to have memory leaks
on websites that did this). So cleaning up when the UI is closed isn't optional.
Doing the same when the child navigates away can be done as well - but it would
add complexity and the gain is rather questionable (typically, the page lives
longer than our composer UI).

[tschuster, 2015-12-02 11:46:32]

On 2015/12/01 19:15:31, Wladimir Palant wrote:
>
https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
> File lib/child/contentPolicy.js (right):
> 
>
https://codereview.adblockplus.org/29329884/diff/29330104/lib/child/contentPo...
> lib/child/contentPolicy.js:61: let storedNodes = new Map();
> On 2015/12/01 16:42:40, tschuster wrote:
> > So, this is cleaned up by DeleteNodes, which is called when you close the
UI,
> > right? Shouldn't we clean this up when the content navigates in the child?
> 
> Nodes can be removed without navigating away (yes, we used to have memory
leaks
> on websites that did this). So cleaning up when the UI is closed isn't
optional.
> Doing the same when the child navigates away can be done as well - but it
would
> add complexity and the gain is rather questionable (typically, the page lives
> longer than our composer UI).

Okay. We should maybe open a follow-up bug for this, but otherwise:
LGTM

[Wladimir Palant, 2015-12-02 11:48:59]

On 2015/12/02 11:46:32, tschuster wrote:
> Okay. We should maybe open a follow-up bug for this

Feel free to open that bug because I really don't see what there is to follow
up.