Closed Bug 1171213 Opened 9 years ago Closed 9 years ago

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

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: amoghbl1, Assigned: sebastian)

References

Details

Attachments

(3 files)

Attached file build-23.log (deleted) —
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
Blocks: android-m
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."
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Web Apps
OS: Unspecified → Android
Hardware: Unspecified → All
Summary: MODE_MULTI_PROCESS in Context has been deprecated with Android M SDK (MNC) → Don't use Context.MODE_MULTI_PROCESS in Allocator (deprecated in Android M)
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?
Flags: needinfo?(gkeeley)
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.
Flags: needinfo?(gkeeley)
Attached patch bug1171213.diff (deleted) — Splinter Review
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.
Attachment #8646972 - Flags: review?(s.kaspari)
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. :)
Attachment #8646972 - Flags: review?(s.kaspari) → review+
leave-open: Allocator will probably be fixed in a different patch some other day.
Keywords: leave-open
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..
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
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?
Flags: needinfo?(nalexander) → needinfo?(s.kaspari)
(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..
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(mark.finkle)
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
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.
Attached patch 1171213-Allocator.patch (deleted) — Splinter Review
Attachment #8669832 - Flags: review?(mark.finkle)
Attachment #8669832 - Flags: review?(mark.finkle) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: