Closed Bug 1050423 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
macOS
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v1.4 affected, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v1.4 --- affected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: khuey, Assigned: fabrice)

References

Details

Attachments

(2 files, 1 obsolete file)

[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.
qawanted to test on 1.4 and master.  I suspect this is a regression.
Flags: needinfo?(jsmith)
Keywords: qawanted, regression
Sure - I'll have someone look at this.
Flags: needinfo?(jsmith)
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.
blocking as this is a spin-off from https://bugzilla.mozilla.org/show_bug.cgi?id=1047639 needed by CAF
blocking-b2g: 2.0? → 2.0+
Keywords: qaurgent
Assignee: nobody → fabrice
QA Contact: jmercado
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Attached patch permission-prompt.patch (obsolete) — Splinter Review
Kyle, this patch let us remove the event listeners, but still doesn't fix the queued-ipc-message count.
See Also: → 1047639
Attached patch gecko patchSplinter Review
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.
Attachment #8469681 - Attachment is obsolete: true
Attached patch gaia patchSplinter Review
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...
Attachment #8469836 - Flags: review?(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
Attachment #8469836 - Flags: review?(alive) → review+
(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.
Attachment #8469834 - Flags: review?(21)
https://hg.mozilla.org/mozilla-central/rev/18f47da1e332
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.