894135 - Don't leak homescreenFrame from within Gaia's apploaded closure

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Justin Lebar (not reading bugmail)]

In trying to fix bug 893012, we (mccr8, khuey, mrbkap, dbaron) discovered that the |apploaded| function in window_manager.js leaks homescreenFrame, even though it doesn't touch that variable.

Changing the closure so it doesn't touch |document| fixes the problem.

Patch in a moment.

Comment 1

[Justin Lebar (not reading bugmail)]

This is the PR, which also includes bug 894081.

https://github.com/mozilla-b2g/gaia/pull/10991

We just need a gaia person to sign off that we're not breaking anything here.  Note that I also changed the event listener from living on |frame| to living on |iframe|; this isn't strictly necessary, but given bug 894081, I think it's a good example to set.

Comment 2

[Justin Lebar (not reading bugmail)]

Alive, do you mind signing off on this as well, when you review the PR?  I have the sign-off of four Gecko people on the change, but that's not quite the same.  :)

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Let's be honest here ;-)

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Just give you the r+ on another bug about this issue.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Justin, I want to know more about this to avoid the same thing happens again.
Is this a general leaking pattern if we touch 'document' in any event handler?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Alive Kuo [:alive] from comment #5)
> Justin, I want to know more about this to avoid the same thing happens again.
> Is this a general leaking pattern if we touch 'document' in any event
> handler?

We currently think this is a bug in the JS engine.  You may want to follow along in Bug 894149.

Comment 7

[Justin Lebar (not reading bugmail)]

(In reply to Alive Kuo [:alive] from comment #5)
> Justin, I want to know more about this to avoid the same thing happens again.
> Is this a general leaking pattern if we touch 'document' in any event
> handler?

But until we fix that bug, I think it's probably wise to avoid touching |document| from within event handlers.

In fixing this bug we've now found three leaks caused by event handlers (one in Gecko, two in Gaia), so I definitely owe the mailing lists an e-mail about this.  None of the bugs was obvious, I think; we just have to be careful.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> so I definitely owe the mailing lists an e-mail
> about this.  None of the bugs was obvious, I think; we just have to be
> careful.

Agree, it's tricky...thanks.

Comment 9

[Justin Lebar (not reading bugmail)]

https://github.com/mozilla-b2g/gaia/commit/d85dbb240e000903c9cb0a7a1f8aff45b3592e28

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted d85dbb240e000903c9cb0a7a1f8aff45b3592e28 to:
v1-train: 3724c21c68f88169a443e2a5636b65b4a4a58f59

Comment 11

[Justin Lebar (not reading bugmail)]

fwiw we now believe that pulling |document| out of the closure and getting it off the event target has no effect, but I'm pretty sure that registering the listener on |iframe| instead of |frame| is necessary.  But I don't want to churn this code unnecessarily right now, so if it's OK with you guys, I'm going to leave it alone until things calm down.