ENV: Device: flame LoopClient: 3cd2086 Build: flame-kk.user.v2.0.gecko-6cc9a4d.gaia-31434a3 RAM=512M STR: 1. Install Loop app 2. Open Loop app and follow the wizard until the window that shows the option to allow Loop access to the CONTACT LIST. 3. DON'T REMEMBRER the choice and push ALLOW button. 4. Use phone number or FxAccount. 5. Make a call and hang up 6. Logout from Loop settings. 7. Kill loop app long tapping home button. 8. Start again loop app 9. And select Phone Number or Fx Account. ACTUAL RESULT: Loop app crashes.

According to Maria Angeles I gonna work on it, Borja if you had some patch for this issue or you have some suggestion/idea please let me know. Thanks a lot

Cristian, about STR, you can skip step five (5. Make a call and hang up), to reproduce de same actual result.

This app is privileged and uses the moz Contacts API "permissions": { "contacts":{ "access": "readonly" } } The JS code implements this: navigator.mozContacts.getRevision(); STR: 1) Install and launch the app 2) Once the app permission prompt is displayed, DON'T remember the choice and click on "Allow" button 3) Kill the app from the task switcher 4) Launch the app Expected result: The permission prompt is displayed again Result: The app crashes Note: If you remember the choice, next time the app works fine

Hi Fabrice, do you know which is the correct module reading comment 4? thanks a lot

We are intentionally crashing at [1] because the child process (Loop in this case) has no permissions to use the mozContacts API [2]. Because we have no permissions, we are not supposed to go that far cause we are already checking the permission on the child at [3]. However, there is a mismatch between the permission value obtained on the child (ALLOW_ACTION) and the one obtained on the parent side (PROMPT_ACTION). I've checked the permission database and the one obtained from the parent is the one stored in the DB (PROMPT_ACTION). The same mismatch happens if the user chooses to deny the permission. In this case we don't crash. Instead what happens is that we get a DENY_ACTION value after the app is killed and restarted and we don't show the permission prompt again to allow the user to make a new choice. This is obviously wrong. It seems that what we do when the user chooses to allow or deny the permission is to store the user's choice for that session only [4][5]. But killing the app doesn't restart the session as I would have expected and that's where the permission mismatch between child and parent happens. I haven't really dig into this, but with a first quick look it seems that the permission manager session is restored at [6]. [1] http://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactService.jsm#205 [2] http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#429 [3] http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#483 [4] http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#365 [5] http://mxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#105 [6] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#177

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #6) > It seems that what we do when the user chooses to allow or deny the > permission ... ... without remembering her choice ...

[Blocking Requested - why for this release]: This issue makes Loop and any other privileged app using the mozContacts API crash if the user allows access to her contacts data without remembering her choice.

I'm not sure why we choose to use EXPIRE_SESSION and not EXPIRE_NEVER here.

(In reply to Fabrice Desré [:fabrice] from comment #9) > I'm not sure why we choose to use EXPIRE_SESSION and not EXPIRE_NEVER here. Isn't EXPIRE_NEVER the same as remember the permission by default? If I am not wrong, we want the user choice to be valid only for the current session if she chose not to remember her choice. I understand sessions here as the lifetime of the app process requesting the permission.

Looking at the PermissionManager code it seems that we are notifying other processes about the permission change [1] even if the permission change should only affect to the current process/session (because of the EXPIRE_SESSION thing). Is it possible that Nuwa is one of these processes receiving this permission change notification and because we fork new app processes from Nuwa we are inheriting the in memory cached permission table with the permission change? [1] http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#743

I don't know how B2G handles EXPIRE_SESSION or whether the permission manager is per-app, but in Firefox EXPIRE_SESSION means that the permission expires when Firefox closes. It's not process-specific at all.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #11) > Looking at the PermissionManager code it seems that we are notifying other > processes about the permission change [1] even if the permission change > should only affect to the current process/session (because of the > EXPIRE_SESSION thing). Is it possible that Nuwa is one of these processes > receiving this permission change notification and because we fork new app > processes from Nuwa we are inheriting the in memory cached permission table > with the permission change? > > [1] > http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/ > nsPermissionManager.cpp#743 Here we get the list of ContentParent(s) using ContentParent::GetAll(), which doesn't include the Nuwa because we don't add it to sContentParents: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1850 So I think we need to pursue the root cause in another direction.

Thanks Cervantes! You are right Nuwa is not receiving the permission change notification. But the preallocated process seems to be receiving it. And I still think that it might be the cause of this issue. If I am not wrong we only reset the permissions when we create the preallocated process at [1], but when we launch the app using that process, its permission table still has the session of the previous app. [1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#177

This patch fixes the issue for me. What we do now is not notifying the preallocated process about a session specific permission change and we also ignore these permissions on the initial permission table creation.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6361890c6ce3 I am not sure how can I add tests for this being an issue that specific to B2G processes... :\ Any suggestion is more than welcome.

Comment on attachment 8503144 [details] [diff] [review] v1 Review of attachment 8503144 [details] [diff] [review]: ----------------------------------------------------------------- Tell me what you think about this different approach. ::: extensions/cookie/nsPermissionManager.cpp @@ +433,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + bool ignoreSessionPermissions = false; > + > +#ifdef MOZ_B2G what about: #ifdef MOZ_B2G ignoreSessionPermissions = true; #endif and then in AddInternal: #ifdef MOZ_B2G if (aIgnoreSessionPermissions && aExpireType == nsIPermissionManager::EXPIRE_SESSION) { aPermission = nsIPermissionManager::PROMPT_ACTION; aExpireType = nsIPermissionManager::EXPIRE_NEVER; } #endif

Thanks Andrea. This patch addresses your previous feedback. I'd also like Jonas to take a look at the patch if possible as he reviewed this code on bug 811026.

Comment on attachment 8503198 [details] [diff] [review] v1 Review of attachment 8503198 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +846,5 @@ > + // previously allowed or denied by the user. > + if (aIgnoreSessionPermissions && > + aExpireType == nsIPermissionManager::EXPIRE_SESSION && > + (aPermission == nsIPermissionManager::ALLOW_ACTION || > + aPermission == nsIPermissionManager::DENY_ACTION)) { Probably you don't need this additional check. If you want to ignore the sessions, do it completely: if (aIgnoreSessionPermission && aExpireType == nsIPermissionManager::EXPIRE_SESSION) { ... }

Per the discussion on #b2g channel, it seems the root cause is that nsPermissionManager::Init() is not safe for fork(). Following the comment #14 the root cause could be that the Nuwa process calls nsPermissionManager::Init() and the cached values are inherited to the forked processes. If this is the case, we should move nsPermissionManager::Init() after the content process is forked as we did to mozilla::dom::time::DateCacheCleaner in bug 965912. Later I found the theory to be wrong. The first time nsPermissionManager is used is in dom/ipc/preload.js, which is called after the preallocated process is created. That's why the preallocated process is receiving permission updates. So we are not sure how the mismatch as mentioned in comment #6 came to be. Sorry to jump in after we already have a working patch (given that it's a 2.0+ bug), but I think we should try to not special-case nsPermissionManager with B2G specific code or skipping the updates to the preallocated process if possible. I'll get my hands on the bug and see if we can have a fix without the special cases.

I was looking at this with Fernando and I don't think the problem is that nsPermissionManager::Init is not safe for forking processes as much as that the semantics of SESSION are different between a monoprocess Gecko and a multiprocess one. On a monoprocess, the session lifetime expires with the process. On multiprocess, the session lifetime is linked with the life of the process that requested the permission (not the main process). That's being treated correctly on the parent process, but it is copying the permission to the preloaded process incorrectly. As for adding B2G specific code, I think we can safely remove the #ifdef(s) from Fernando code, and the ignoreSessionPermission boolean. The session permissions should never be copied to the preloaded processes, on any platform. That would leave basically Fernando's code, without any specific B2G restriction because if any other multiprocess platform (e10s?) uses preloaded processes it'll run into the same problems (and because the code that informs other processes of permissions changed isn't ifdef-ed either, nor the IsPreallocated method of ContentParent.

Thanks for the clarification, :amac. Since this bug results from the semantics of SESSION on a multiprocess gecko, that gecko prior to 2.0 also has this problem, right?

Thanks Antonio! I added the #ifdef(s) because of comment 12. Do we use a preloaded process on e10s Firefox? I'll upload a new patch without the #ifdef(s). (In reply to Cervantes Yu from comment #23) > Thanks for the clarification, :amac. Since this bug results from the > semantics of SESSION on a multiprocess gecko, that gecko prior to 2.0 also > has this problem, right? I believe so. If the multiprocess gecko uses a preloaded process it will have the same problem.

I don't think we currently use preloaded processes for desktop Firefox on any platform, but we might in the future. Perhaps obviously, a fork-based preload solution isn't going to work on Windows.

Same thing without #ifdefs

Thanks Andrea. The new patch breaks the tests, I'm looking into it. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9c530921918

This fixes the tests https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=738151d9a351

Comment on attachment 8506105 [details] [diff] [review] Tests Review of attachment 8506105 [details] [diff] [review]: ----------------------------------------------------------------- is it fully green on try?

Looking good so far https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83b758b1cfa1

The tests that were orange on the previous try run were caused by bug 1081842 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2e45a20ec4fd https://hg.mozilla.org/integration/b2g-inbound/rev/0ef8d60815d7 https://hg.mozilla.org/integration/b2g-inbound/rev/0fd07d2752e8

https://hg.mozilla.org/mozilla-central/rev/0ef8d60815d7 https://hg.mozilla.org/mozilla-central/rev/0fd07d2752e8

Comment on attachment 8505405 [details] [diff] [review] v2 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): Permission Manager User impact if declined: Without this patch the user regarding a permission will be remembered even if she explicitly asked not to remember the permission the first time after she made her choice. In some cases, this issue can even make apps crash like it's happening with Loop when we try to access the Contacts API after the user allowed us to do it in a previous run but didn't ask the system to remember her choice. It can also allow apps to obtain access to privileged APIs even if the user has not explicitly allow it for the current execution of the app. Testing completed: Manual tests and unit tests. Risk to taking this patch (and alternatives if risky): This patch affects the Permission Manager which is a core part of the system and just because of that it implies certain risk. I did several tests on my side, but I'd appreciate QA verification before doing any uplift. String or UUID changes made by this patch: None.

javier can you please help verify this on trunk before we uplift this on branches?

Tested on trunk and works fine. No crash

Bhavana, we have verified that this bug and bug 1045598 (Loop) are working fine in master, can you give the approval for this patch? thanks a lot!

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/00e822c5c751 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2df4dd0670a9

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b1cb8df2ed2f https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/bb5689113c1d

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b1cb8df2ed2f https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bb5689113c1d

Verified on FLAME (184based.B-43.Gecko-675d616.Gaia-2183b4f), Loop version 1.1: 5806319 It's still failing on FireE, v2.0-SW2E3-1 (waiting for uplifted).

Verified on FireE, firee-kk-v2.0-SW2E5-4, loop version, 1.1: cc87bd0

This issue has been verified successfully on Flame 2.0, 2.1, 2.2 and woodduck 2.0 See attachment: 1107.MP4 Reproducing rate: 0/5 Step: 1. Install and launch the app. 2. Once the app permission prompt is displayed, DON'T remember the choice and click on "Allow" button. 3. Kill the app from the task switcher. 4. Launch the app, use FX Account to log in. 5. DON'T REMEMBRER the choice and push ALLOW button. 6. Make a call and hang up 7. Logout from Loop settings. 8. Kill loop app long tapping home button. 9. Start again loop app 10. And select Fx Account. Actual result: 4.The permission prompt is displayed again. 10.The Loop app use normally, the crash can't pops up. Woodduck version: Gaia-Rev ead3b72a84512750bc5faff4e9e8faa1715c0d05 Gecko-Rev 8d40d6480ee0e628b0f7655dcd6ff79a2f2fbcfc Build-ID 20141211050313 Version 32.0 Device-Name jrdhz72_w_ff FW-Release 4.4.2 FW-Incremental 1418245573 FW-Date Thu Dec 11 05:06:41 CST 2014 Flame 2.1 version: Gaia-Rev c226db212db4d824c09617cd6dc407b2d4258d9b Gecko-Rev https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/cf8bebfa4703 Build-ID 20141210001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141210.035300 FW-Date Wed Dec 10 03:53:11 EST 2014 Bootloader L1TC00011880 Flame 2.2 version: Gaia-Rev e17c5656dbf517d48fb61ac9bc92119e023fd717 Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/be1f49e80d2d Build-ID 20141210040201 Version 37.0a1 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141210.074809 FW-Date Wed Dec 10 07:48:20 EST 2014 Bootloader L1TC00011880 Flame 2.0 version: Gaia-Rev 856863962362030174bae4e03d59c3ebbc182473 Gecko-Rev https://hg.mozilla.org/releases/mozilla-2g32_v2_0/rev/2d0860bd0225 Build-ID 20141210000202 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141210.034839 FW-Date Wed Dec 10 03:48:50 EST 2014 Bootloader L1TC00011880

The Loop version:bd8f1c2