Closed Bug 894135 Opened 10 years ago Closed 10 years ago
Don't leak homescreen
Frame from within Gaia's apploaded closure
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.
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.
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. :)
Assignee: nobody → justin.lebar+bug
Let's be honest here ;-)
Whiteboard: [MemShrink] → [MemShrink:P1]
Depends on: 894149
Just give you the r+ on another bug about this issue.
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?
(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.
(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.
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Uplifted d85dbb240e000903c9cb0a7a1f8aff45b3592e28 to: v1-train: 3724c21c68f88169a443e2a5636b65b4a4a58f59
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.
Whiteboard: [MemShrink:P1] QARegressExclude → [MemShrink:P1] QARegressExclude [LeoVB+]
You need to log in before you can comment on or make changes to this bug.