1171213 - Don't use Context.MODE_MULTI_PROCESS in Allocator (deprecated in Android M)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[amoghbl1]

Attachment: build-23.log

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150518070114

Steps to reproduce:

Build on the current mozilla-central repo with the new sdk Android SDK MNC


Actual results:

Build failure

Comment 1

[amoghbl1]

Seems to be used at two places, mobile/android/base/webapp/Allocator.java and mobile/android/stumbler/java/org/mozilla/mozstumbler/service/Prefs.java .
As per the new docs:
"This constant is deprecated. MODE_MULTI_PROCESS does not work reliably in some versions of Android, and furthermore does not provide any mechanism for reconciling concurrent modifications across processes. Applications should not attempt to use it. Instead, they should use an explicit cross-process data management approach such as ContentProvider."

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Garvan: Do you have a good idea how to get rid of the deprecated Context.MODE_MULTI_PROCESS flag? I assume it is used to flip the settings in the fennec process but read them from the stumbler process? Google's depcreation comment mentions ContentProviders as an alternative but this seems to be over-engineered for some simple preferences. Maybe keeping the preference in one process and adding IPC to read the values from another process is the way to go?

Comment 3

[:garvan]

Thanks for the ping, IIRC that was there as a remnant of an older process model for stumbler (Mozilla Stumbler the app actually) that is no longer used. Will look now.

Comment 4

[:garvan]

Attachment: bug1171213.diff

An older app design of the stumbler required this, it is no longer needed.

There is a robocop test that toggles the pref to start stumbling, which in turn toggles the internal pref. Not sure which rc* it is but they are all green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ae4f0b6aad6

And of course, I am manually testing it.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8646972 [details] [diff] [review]
bug1171213.diff

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

Awesome! Thank you!

Now only org.mozilla.gecko.webapp.Allocator is left here. But this is not part of Stumbler. :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

leave-open: Allocator will probably be fixed in a different patch some other day.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15a5993ebd8e

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15a5993ebd8e

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

NI: Anyone of you know how we could work around that last Context.MODE_MULTI_PROCESS usage?
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/webapp/Allocator.java?offset=100#50

The docs say:
"This constant was deprecated in API level 23. MODE_MULTI_PROCESS does not work reliably in some versions of Android, and furthermore does not provide any mechanism for reconciling concurrent modifications across processes. Applications should not attempt to use it. Instead, they should use an explicit cross-process data management approach such as ContentProvider."

But I don't think we want to use a ContentProvider here..

Comment 10

[Nick Alexander :nalexander [he/him]]

To radiate information here:
 
Allocator is used in a cross-process manner.  This pattern is actually a good candidate for a ContentProvider, possibly backed by SharedPrefs accessed by the single ContentProvider process.

We could use local broadcasts to a service; or we could expose an AIDL interface to the same service.  At the end of the day, I think the ContentProvider is easiest.

However, it's my belief that this functionality will be killed.  mfinkle, can you comment here?

sebastian: Can we just live with the deprecation warning?

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #10)
> sebastian: Can we just live with the deprecation warning?

Yeah, we can suppress the warning for now. Only concern here is: "MODE_MULTI_PROCESS does not work reliably in some versions of Android". But we have been living with that for quite some time now anyways..

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #10)

> We could use local broadcasts to a service; or we could expose an AIDL
> interface to the same service.  At the end of the day, I think the
> ContentProvider is easiest.
> 
> However, it's my belief that this functionality will be killed.  mfinkle,
> can you comment here?

Yes. This functionality is being considered for removal. Discussions have started, but I wouldn't expect a removal until Fx45 at the earliest.

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> (In reply to Nick Alexander :nalexander from comment #10)
> > sebastian: Can we just live with the deprecation warning?
> 
> Yeah, we can suppress the warning for now. Only concern here is:
> "MODE_MULTI_PROCESS does not work reliably in some versions of Android". But
> we have been living with that for quite some time now anyways..

Agreed. We have been using this for many releases now. If we can suppress the warning, that's the way forward until we reach a timeline for removal.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

I'm confused. I thought this is already fixed. I did a bunch of local and try build without this showing up. Anyways I'll create a patch.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1171213-Allocator.patch

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3e2981a93f03

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e2981a93f03