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

VERIFIED FIXED in Firefox 36, Firefox OS v2.0

Status

()

Core
DOM: Device Interfaces
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Javier de Prado, Assigned: ferjm)

Tracking

(Blocks: 1 bug, {crash, reproducible})

unspecified
mozilla36
ARM
Gonk (Firefox OS)
crash, reproducible
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [blocking][tef-triage][Test-Run1])

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
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.
Blocks: 1036490
Severity: normal → major
Whiteboard: [mobile app]
Severity: major → critical
Hardware: x86 → ARM
Whiteboard: [mobile app] → [mobile app][blocking][tef-triage]
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
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
Flags: needinfo?(borja.bugzilla)
(Reporter)

Comment 3

3 years ago
Cristian, about STR, you can skip step five (5. Make a call and hang up), to reproduce de same actual result.
Created attachment 8500370 [details]
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
Component: Gaia::Loop → DOM: Device Interfaces
Product: Firefox OS → Core
Hi Fabrice, do you know which is the correct module reading comment 4? thanks a lot
Assignee: crdlc → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(borja.bugzilla) → needinfo?(fabrice)
(Assignee)

Updated

3 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Comment 6

3 years ago
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
Flags: needinfo?(amarchesini)
(Assignee)

Comment 7

3 years ago
(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 ...
(Assignee)

Comment 8

3 years ago
[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.
blocking-b2g: --- → 2.0?
I'm not sure why we choose to use EXPIRE_SESSION and not EXPIRE_NEVER here.
Flags: needinfo?(fabrice)
Whiteboard: [mobile app][blocking][tef-triage] → [blocking][tef-triage]

Updated

3 years ago
blocking-b2g: 2.0? → 2.0+
Keywords: crash, reproducible
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
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
Flags: needinfo?(benjamin)

Comment 12

3 years ago
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.
Flags: needinfo?(benjamin)
(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.
(Assignee)

Comment 14

3 years ago
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
Flags: needinfo?(cyu)
(Assignee)

Comment 15

3 years ago
Created attachment 8503144 [details] [diff] [review]
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.
Attachment #8503144 - Flags: feedback?(jonas)
Attachment #8503144 - Flags: feedback?(amarchesini)
Flags: needinfo?(cyu)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 16

3 years ago
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
Attachment #8503144 - Flags: feedback?(amarchesini)
(Assignee)

Comment 18

3 years ago
Created attachment 8503198 [details] [diff] [review]
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.
Attachment #8503144 - Attachment is obsolete: true
Attachment #8503144 - Flags: feedback?(jonas)
Attachment #8503198 - Flags: review?(jonas)
Attachment #8503198 - Flags: review?(amarchesini)
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) {
  ...
}
Attachment #8503198 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1079538
Blocks: 1062883
(Assignee)

Updated

3 years ago
Blocks: 1045598
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?
(Assignee)

Comment 24

3 years ago
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.
Flags: needinfo?(benjamin)

Comment 25

3 years ago
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.
Flags: needinfo?(benjamin)
(Assignee)

Comment 26

3 years ago
Created attachment 8505405 [details] [diff] [review]
v2

Same thing without #ifdefs
Attachment #8503198 - Attachment is obsolete: true
Attachment #8503198 - Flags: review?(jonas)
Attachment #8505405 - Flags: review?(amarchesini)
Attachment #8505405 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 27

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

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

Comment 28

3 years ago
Created attachment 8506105 [details] [diff] [review]
Tests

This fixes the tests

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=738151d9a351
Attachment #8506105 - Flags: review?(amarchesini)
Comment on attachment 8506105 [details] [diff] [review]
Tests

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

is it fully green on try?
Attachment #8506105 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 30

3 years ago
Looking good so far 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83b758b1cfa1
(Reporter)

Updated

3 years ago
Whiteboard: [blocking][tef-triage] → [blocking][tef-triage][Test-Run1]
(Assignee)

Comment 31

3 years ago
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Comment 33

3 years ago
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.
Attachment #8505405 - Flags: approval-mozilla-b2g34?
Attachment #8505405 - Flags: approval-mozilla-b2g32?
(Assignee)

Updated

3 years ago
Attachment #8506105 - Flags: approval-mozilla-b2g34?
Attachment #8506105 - Flags: approval-mozilla-b2g32?
javier can you please help verify this on trunk before we uplift this on branches?
Flags: needinfo?(javier.deprado)

Comment 35

3 years ago
Tested on trunk and works fine. No crash
Flags: needinfo?(javier.deprado)
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!
Flags: needinfo?(bbajaj)
Attachment #8505405 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8506105 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+

Updated

3 years ago
Flags: needinfo?(bbajaj)
Attachment #8505405 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+

Updated

3 years ago
Attachment #8506105 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/00e822c5c751
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2df4dd0670a9
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox34: --- → wontfix
status-firefox35: --- → wontfix
status-firefox36: --- → fixed
Flags: in-testsuite+
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b1cb8df2ed2f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/bb5689113c1d
status-b2g-v2.0: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b1cb8df2ed2f
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bb5689113c1d
status-b2g-v2.0M: --- → fixed
(Reporter)

Comment 40

3 years ago
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).
Status: RESOLVED → VERIFIED
(Reporter)

Updated

3 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 3 years ago3 years ago
(Reporter)

Comment 41

3 years ago
Verified on FireE, firee-kk-v2.0-SW2E5-4,
loop version, 1.1: cc87bd0
Status: RESOLVED → VERIFIED
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.0M: fixed → verified
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Created attachment 8534792 [details]
1107.MP4
The Loop version:bd8f1c2
You need to log in before you can comment on or make changes to this bug.