1068635 - [Loop] Not remembering + allow access to the contact list, 2nd time Loop is started -> Crash!

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Javier de Prado]

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.

Comment 2

[Cristian Rodriguez (:crdlc)]

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

Comment 3

[Javier de Prado]

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

Comment 4

[Cristian Rodriguez (:crdlc)]

Attachment: app.zip

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

Comment 5

[Cristian Rodriguez (:crdlc)]

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

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

(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 ...

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

[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.

Comment 9

[[:fabrice] Fabrice Desré]

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

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(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.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 12

[Benjamin Smedberg]

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.

Comment 13

[Cervantes Yu [:cyu] [:cervantes]]

(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.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

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.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

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 17

[Andrea Marchesini [:baku]]

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

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

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 19

[Andrea Marchesini [:baku]]

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) {
  ...
}

Comment 21

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 23

[Cervantes Yu [:cyu] [:cervantes]]

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?

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 25

[Benjamin Smedberg]

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.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Same thing without #ifdefs

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Thanks Andrea. The new patch breaks the tests, I'm looking into it.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9c530921918

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

This fixes the tests

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

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8506105 [details] [diff] [review]
Tests

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

is it fully green on try?

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 34

[bhavana bajaj [:bajaj]]

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

Comment 35

[mbarone]

Tested on trunk and works fine. No crash

Comment 36

[Maria Angeles Oteo (:oteo)]

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!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Javier de Prado]

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).

Comment 41

[Javier de Prado]

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

Comment 42

[Lance（leave from Mozilla）]

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

Comment 43

[Lance（leave from Mozilla）]

Attachment: 1107.MP4

Comment 44

[Lance（leave from Mozilla）]

The Loop version:bd8f1c2