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

RESOLVED FIXED

Status

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

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

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

Attachments

(1 attachment)

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
Created attachment 779470 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11101

Pointer to Github pull-request
Attachment #779470 - Flags: review?(salva)
Whiteboard: [MemShrink]
Blocks: 893012
blocking-b2g: --- → leo?
Blocks a blocker.
blocking-b2g: leo? → leo+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?
Flags: needinfo?(khuey)
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 :-)
Flags: needinfo?(khuey)
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Flags: needinfo?(justin.lebar+bug)
If you leave a crashed iframe mozbrowser in the DOM, it will hold onto the ContentParent and associated IPC machinery in the parent process.
I see. And when you reset the attributes, are not these mechanisms wiped out?
They are, but the attributes are not reset until _ensureWidget is called again.  That could be some time from when the process crashes.
Flags: needinfo?(justin.lebar+bug)
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.
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 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!
Attachment #779470 - Flags: review?(salva) → review+
Pushed to master : https://github.com/mozilla-b2g/gaia/commit/8b49be437ee81aa76ac8e267afeb7d25520068fc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
[v1-train 055c122] Bug 896755: Handle app crashes in the cost control widget. r=salva
status-b2g18: --- → fixed

Updated

5 years ago
Whiteboard: [MemShrink:P2] → [MemShrink:P2][LeoVB+]

Updated

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