Last Comment Bug 894135 - Don't leak homescreenFrame from within Gaia's apploaded closure
: Don't leak homescreenFrame from within Gaia's apploaded closure
Status: RESOLVED FIXED
[MemShrink:P1] QARegressExclude [LeoVB+]
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::System (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 894149
Blocks: 893012
  Show dependency treegraph
 
Reported: 2013-07-15 17:16 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-08-05 16:42 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
leo+
fixed


Attachments

Description Justin Lebar (not reading bugmail) 2013-07-15 17:16:42 PDT
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) 2013-07-15 17:29:08 PDT
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) 2013-07-15 17:29:53 PDT
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 [:khuey] (khuey@mozilla.com) 2013-07-15 17:36:59 PDT
Let's be honest here ;-)
Comment 4 (inactive after 6/18) Alive Kuo [:alive] 2013-07-15 20:21:52 PDT
Just give you the r+ on another bug about this issue.
Comment 5 (inactive after 6/18) Alive Kuo [:alive] 2013-07-15 22:35:25 PDT
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 [:khuey] (khuey@mozilla.com) 2013-07-15 23:02:26 PDT
(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) 2013-07-15 23:24:06 PDT
(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] 2013-07-15 23:30:26 PDT
(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 10 John Ford [:jhford] 2013-07-17 13:00:00 PDT
Uplifted d85dbb240e000903c9cb0a7a1f8aff45b3592e28 to:
v1-train: 3724c21c68f88169a443e2a5636b65b4a4a58f59
Comment 11 Justin Lebar (not reading bugmail) 2013-07-18 11:25:26 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.