896755 - cost-control-widget does not ever clean up after itself

Status: RESOLVED FIXED

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

Description

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

The cost control widget code does not handle the app crashing.  It simply creates a new iframe the next time it wants to load cost control without removing the old one, potentially leaking an unbounded number of <iframe mozbrowser>s

Comment 1

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

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11101

Pointer to Github pull-request

Comment 2

[Justin Lebar (not reading bugmail)]

Blocks a blocker.

Comment 3

[Salvador de la Puente González [:salva]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #0)
> The cost control widget code does not handle the app crashing.  It simply
> creates a new iframe the next time it wants to load cost control without
> removing the old one, potentially leaking an unbounded number of <iframe
> mozbrowser>s

I think no. The condition in line 29:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/cost_control.js#L29

prevents the code to create another new iframe. In case of error, the `killed` attribute is set so a reconfiguration is forced but not a new creation.

What do you think?

Comment 4

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

Yes, I believe you are correct.  There is only ever one iframe.  But we can still keep that iframe around (which causes things like leaks of ContentParent objects (e.g. bug 893012)).

Sorry I confused this with some of the other gaia bugs I was investigating yesterday :-)

Comment 5

[Salvador de la Puente González [:salva]]

AFAIK, the `mozbrowser` should detach the iframe from the main process and we should not have ContentParent leaks as it is impossible to communicate from one to another via JavaScript, isn't it?

I'm not an expert here so asking for further clarifications. Please, explain me what kind of leak is a ContentParent leak and why we could have one? 

Thank you.

Comment 6

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

If you leave a crashed iframe mozbrowser in the DOM, it will hold onto the ContentParent and associated IPC machinery in the parent process.

Comment 7

[Salvador de la Puente González [:salva]]

I see. And when you reset the attributes, are not these mechanisms wiped out?

Comment 8

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

They are, but the attributes are not reset until _ensureWidget is called again.  That could be some time from when the process crashes.

Comment 9

[Justin Lebar (not reading bugmail)]

Hi, Salva.

This is a critical bug that we need to fix as soon as possible, so Leo can continue testing devices for stability.

Please let me know if you can't get to this review in the next day or two; I'm happy to find another reviewer.

Comment 10

[Salvador de la Puente González [:salva]]

Per comment 1, this is not even happening, this is only a potential risk and per comment 3 the risk is even lower. But I'm testing just right now to ensure nothing breaks and I grant the r+ as soon as possible.

Comment 11

[Salvador de la Puente González [:salva]]

Comment on attachment 779470 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11101

Nice and easy.
Your last comment convinced me.

Thank you!

Comment 12

[[:fabrice] Fabrice Desré]

Pushed to master : https://github.com/mozilla-b2g/gaia/commit/8b49be437ee81aa76ac8e267afeb7d25520068fc

Comment 13

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

[v1-train 055c122] Bug 896755: Handle app crashes in the cost control widget. r=salva