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

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

(Whiteboard: [MemShrink:P1] QARegressExclude [LeoVB+])

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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.  :)
Flags: needinfo?(alive)
(Assignee)

Updated

4 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink]
Let's be honest here ;-)
Whiteboard: [MemShrink] → [MemShrink:P1]
Depends on: 894149
Flags: needinfo?(alive)
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.
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Updated

4 years ago
blocking-b2g: --- → leo+
(Assignee)

Comment 9

4 years ago
https://github.com/mozilla-b2g/gaia/commit/d85dbb240e000903c9cb0a7a1f8aff45b3592e28
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Uplifted d85dbb240e000903c9cb0a7a1f8aff45b3592e28 to:
v1-train: 3724c21c68f88169a443e2a5636b65b4a4a58f59
status-b2g18: --- → fixed
(Assignee)

Comment 11

4 years ago
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.

Updated

4 years ago
Whiteboard: [MemShrink:P1] → [MemShrink:P1] QARegressExclude

Updated

4 years ago
Whiteboard: [MemShrink:P1] QARegressExclude → [MemShrink:P1] QARegressExclude [LeoVB+]
You need to log in before you can comment on or make changes to this bug.