1050423 - Killing an app that has an open permissions dialog leaks the ContentParent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

[Blocking Requested - why for this release]:

STR:

1. Launch the Camera app.
2. When you get the permission dialog asking to share your location with Camera, kill the Camera app via adb (e.g. adb shell b2g-info, pick out the Camera app, adb shell kill -9 PID)
3. Repeat steps 1 and 2 five times.
4. Take an about memory report with minimization (./tools/get_about_memory.py --minimize)

Actual results:

If you scroll down to "queued-ipc-messages" in the main process you'll see a number of entries for Camera with pid=-1, closed channel, etc

Expected results:

No camera entries.

Comment 1

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

qawanted to test on 1.4 and master.  I suspect this is a regression.

Comment 2

[Jason Smith [:jsmith]]

Sure - I'll have someone look at this.

Comment 3

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

The path in question:

via mCallback :
0xa76dc4a0 [Function contentEvent]
    --[fun_callscope]--> 0xa690c3d0 [Call <no private>]
    --[callback]--> 0xa76dc480 [Function ContentPermissionPrompt.prototype.deleg
at]
    --[fun_callscope]--> 0xa69dbba0 [Call <no private>]
    --[callback]--> 0xa76dc460 [Function onCallback]
    --[fun_callscope]--> 0xaa25e510 [Call <no private>]
    --[frame]--> 0xa761d5e0 [Proxy <no private>]
    --[private]--> 0xa8e5c760 [Proxy <no private>]
    --[private]--> 0xaaa5b460 [HTMLIFrameElement <no private>]

mCallback is a mozContentEvent listener on the system app's window.  It was added from https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/annotate/tip/b2g/components/ContentPermissionPrompt.js#l385.  That callback is chained to https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/annotate/tip/b2g/components/ContentPermissionPrompt.js#l332 which holds alive the iframe.  This needs to get torn down somehow when the app dies but it's not.

Comment 4

[bhavana bajaj [:bajaj]]

blocking as this is a spin-off from https://bugzilla.mozilla.org/show_bug.cgi?id=1047639 needed by CAF

Comment 5

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue does occur on Flame 2.0, Flame 2.1, Flame 1.4, and Buri 2.0.  This indicates that it isn't a regression so I'm removing the tag.

Environmental Variables:
Device: Flame 2.0
BuildID: 20140807094215
Gaia: bb3d2cf36c10f7186899431d4a21a2489864d9c6
Gecko: 4ad32f9626ad
Version: 32.0 (2.0) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Environmental Variables:
Device: Flame Master
BuildID: 20140807084328
Gaia: 54c3c19d439f7dbafda5c6cc3b4850b545a068ba
Gecko: 2f198e81ed98
Version: 34.0a1 (Master) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Environmental Variables:
Device: Flame 1.4
BuildID: 20140807110116
Gaia: e9dce1f60f729e228810f751417681b5ff937b6b
Gecko: 509ebd2e22f7
Version: 30.0 (1.4) 
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Environmental Variables:
Device: Buri 2.0
BuildID: 20140806140415
Gaia: 47fa0ba8197e71cc7034943ff037642e7f35cdfe
Gecko: 641b3758537b
Version: 32.0 (2.0) 
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: permission-prompt.patch

Kyle, this patch let us remove the event listeners, but still doesn't fix the queued-ipc-message count.

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

We had issues both on the gecko side and on the gaia side.
In gecko, we were sending requests with an id that was expected to be increasing for each request, but unfortunately many call sites do a do_CreateInstance(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID) so we start again at 0 every time. To make everything worse, gaia just ignored the id it received and maintained its own id to send in responses.

That's why we were failing in the second prompt: in the first one we sent 0 and received 0, but in the next ones we were still sending 0 while gaia answered 1, 2, etc. Awesome.

This patch fixes the gecko side by always sending a new uuid since I'm not sure at all we can consider this composant as a service (the 'historical' users in nsDocument and nsGeoloc are doing do_CreateInstance(NS_CONTENT_PERMISSION_PROMPT_CONTRACTID). Using a uuid here we immune to being a service or a new instance.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: gaia patch

Gaia part: make the content permission manager actually return ids that match requests, instead of doing its own thing.
Alive, feel free to redirect the review.

Overall, I'm very surprised all this prompt system worked so far...

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8469836 [details] [diff] [review]
gaia patch

Review of attachment 8469836 [details] [diff] [review]:
-----------------------------------------------------------------

I guess you had to fix some test in permission_manager_test.js

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> Comment on attachment 8469836 [details] [diff] [review]
> gaia patch
> 
> Review of attachment 8469836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess you had to fix some test in permission_manager_test.js

Not yet, but I will.

Comment 11

[[:fabrice] Fabrice Desré]

gaia part: https://github.com/mozilla-b2g/gaia/commit/3e5f4c0d3a2ab6acf9a23232ecc6eaf6969086a2

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/18f47da1e332

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18f47da1e332

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/de28796a8956a48bb98ca67df6a33e0622d642d1

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b6e9aaed56d0