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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Uplifted d85dbb240e000903c9cb0a7a1f8aff45b3592e28 to: v1-train: 3724c21c68f88169a443e2a5636b65b4a4a58f59
status-b2g18: --- → fixed
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.