1189347 - Strict mode: Reading application restrictions triggers disk read violation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Profile Handling, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Reading application restrictions from the UserManager on the MainThread triggers a strict mode / disk read violation:

> D/StrictMode(24695): StrictMode policy violation; ~duration=14 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=287 violation=2
> D/StrictMode(24695): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1137)
> D/StrictMode(24695): 	at libcore.io.BlockGuardOs.access(BlockGuardOs.java:67)
> D/StrictMode(24695): 	at java.io.File.doAccess(File.java:283)
> D/StrictMode(24695): 	at java.io.File.exists(File.java:363)
> D/StrictMode(24695): 	at android.util.AtomicFile.openRead(AtomicFile.java:195)
> D/StrictMode(24695): 	at com.android.server.pm.UserManagerService.readApplicationRestrictionsLocked(UserManagerService.java:1664)
> D/StrictMode(24695): 	at com.android.server.pm.UserManagerService.getApplicationRestrictionsForUser(UserManagerService.java:1448)
> D/StrictMode(24695): 	at com.android.server.pm.UserManagerService.getApplicationRestrictions(UserManagerService.java:1437)
> D/StrictMode(24695): 	at android.os.IUserManager$Stub.onTransact(IUserManager.java:297)
> D/StrictMode(24695): 	at android.os.Binder.execTransact(Binder.java:446)
> D/StrictMode(24695): # via Binder call with stack:
> D/StrictMode(24695): android.os.StrictMode$LogStackTrace
> D/StrictMode(24695): 	at android.os.StrictMode.readAndHandleBinderCallViolations(StrictMode.java:1717)
> D/StrictMode(24695): 	at android.os.Parcel.readExceptionCode(Parcel.java:1527)
> D/StrictMode(24695): 	at android.os.Parcel.readException(Parcel.java:1496)
> D/StrictMode(24695): 	at android.os.IUserManager$Stub$Proxy.getApplicationRestrictions(IUserManager.java:792)
> D/StrictMode(24695): 	at android.os.UserManager.getApplicationRestrictions(UserManager.java:1174)
> D/StrictMode(24695): 	at org.mozilla.gecko.restrictions.RestrictedProfileConfiguration.getAppRestrictions(RestrictedProfileConfiguration.java:82)
> D/StrictMode(24695): 	at org.mozilla.gecko.restrictions.RestrictedProfileConfiguration.isAllowed(RestrictedProfileConfiguration.java:45)
> D/StrictMode(24695): 	at org.mozilla.gecko.RestrictedProfiles.isAllowed(RestrictedProfiles.java:101)

Let's try to read these restrictions on a background thread. Maybe we can limit reading the restrictions to only "app start" and when changed.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1189347-strict-mode-part1.patch

Step 1: Avoid unnecessary reading from disk when isRestrictedProfile() is called.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1189347-strict-mode-part2.patch

This is part 2. Restrictions are read from a cached Bundle. On read the bundle will be updated on a background thread. Trade-off: The first read is still happening on the main thread but all subsequent reads are handled in the background. Updates are triggered on read so on read we might return an old value but we are eventually consistent. Usually these values never change (only when edited by the device admin) so I think this is acceptable here.

Also asking rnewman for architectural feedback. My goal was to always ever have only one background thread and not queuing up multiple updates.

Comment 3

[Richard Newman [:rnewman]]

Two suggestions:

1.

14ms isn't long; one frame. It probably takes about as much time to create and schedule the executor as it does to do the read on the main thread, during which time you're blocking other background operations, so you're basically muffling the warning by doing non-prohibited work. _Just turn off the warning instead._

The work has to be done, and stale values for restrictions seems like a bad tradeoff!

You should check to see whether these user restrictions are implemented anything like SharedPreferences -- that is, they're backed on disk, they sometimes trigger StrictMode warnings, but they're actually cached in memory, and the reads are always fast.

If not, consider speculative preload. Fire off a background read as early as possible in startup, hoping to pre-warm for when you really need to read. We already do this for SharedPreferences, I think; a cold start there *is* a disk read, so we just get it out of the way.

2.

When you have a feature that depends on isRestrictedProfile, you'll often find that it's triggered from a background thread -- e.g., a Gecko message, a queued runnable, whatever. Rather than getting to the UI thread work, then calling back out to a background thread to do a read or write, just do the read/write on the original background runnable before you get to the UI code. This will involve a little bit of walking up and down the call stack in IntelliJ, but it's the better solution.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #3)
> 14ms isn't long; one frame. It probably takes about as much time to create
> and schedule the executor as it does to do the read on the main thread,
> during which time you're blocking other background operations, so you're
> basically muffling the warning by doing non-prohibited work. _Just turn off
> the warning instead._

This is true for a single call to isAllowed(). But we call it more often in various places to determine whether we want to show or hide parts of the UI. Just on app startup I count 32 calls to isAllowed().


(In reply to Richard Newman [:rnewman] from comment #3)
> The work has to be done, and stale values for restrictions seems like a bad
> tradeoff!

It's not perfect but it seems like the best trade-off there is (except for not caring). As I mentioned above: Normally these values never change at runtime. The only way they can is that someone switches to the admin profile, goes to the settings, flips some bits and then switches back to the restricted profile. In this case only the first call would receive a stale value and from then on we are consistent again.

It would be nice to just read the values in the background and have a callback but as most of these reads come from the UI/main thread this is not possible without blocking again or having UI flicker.


(In reply to Richard Newman [:rnewman] from comment #3)
> You should check to see whether these user restrictions are implemented
> anything like SharedPreferences -- that is, they're backed on disk, they
> sometimes trigger StrictMode warnings, but they're actually cached in
> memory, and the reads are always fast.

This would be very nice but it's triggering a file read and XML parsing every time:
https://github.com/android/platform_frameworks_base/blob/master/services/core/java/com/android/server/pm/UserManagerService.java#L1679

And this only works for SharedPreferences if you are reading and writing them from the same process. In our case they are written from some process of the admin profile and read from another one of the restricted profile.


(In reply to Richard Newman [:rnewman] from comment #3)
> If not, consider speculative preload. Fire off a background read as early as
> possible in startup, hoping to pre-warm for when you really need to read. We
> already do this for SharedPreferences, I think; a cold start there *is* a
> disk read, so we just get it out of the way.

I did play a bit with that. Firing off the background read from the constructor of the class. But this was too late to be effective most of the time. We can move it to an earlier point in time like Application.onCreate(). But this way we just move the initial slow read from somewhere where we do UI stuff to the application startup?


(In reply to Richard Newman [:rnewman] from comment #3)
> When you have a feature that depends on isRestrictedProfile, you'll often
> find that it's triggered from a background thread -- e.g., a Gecko message,
> a queued runnable, whatever. Rather than getting to the UI thread work, then
> calling back out to a background thread to do a read or write, just do the
> read/write on the original background runnable before you get to the UI
> code. This will involve a little bit of walking up and down the call stack
> in IntelliJ, but it's the better solution.


isRestrictedProfile() is not that big of a problem because we need to detect that we are on a restricted profile or not only once in an app's lifetime. But isAllowed() will get called a lot. And right now it seems like it gets called on the main thread a lot. Mostly because we use it to hide UI based on whether things are allowed.

I'm not 100% sure I understand your proposal. But what I get from it is that we can skip the background scheduling and perform the read on the current thread if we are not the UI thread. In addition to that you are proposing to expose the API for performing the reads and then call it from the background thread that is active before we jump to the UI thread? I'd like to avoid having the caller needing to obey some contract and hide that implementation. Especially because the same interface is used for guest profiles too.

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 8648691 [details] [diff] [review]
1189347-strict-mode-part2.patch

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

Overall, your code looks fine, but I am not convinced that this is the best path forward, and it sounds rnewman isn't either. Perhaps the three of us should talk this over in person. 

I defer to rnewman on the trade offs of threading procedures.

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java
@@ +42,5 @@
>  
>      public RestrictedProfileConfiguration(Context context) {
>          this.context = context.getApplicationContext();
> +
> +        // Background executor service to update restrictions in the background to avoid disk read violations (strict mode).

Is there really no other way to avoid the violation? This feels like a rather heavy handed solution.

@@ +66,5 @@
> +
> +    private void readRestrictionsInBackground() {
> +        try {
> +            executorService.submit(updateRestrictionsRunnable);
> +        } catch (RejectedExecutionException e) {

I don't suppose its possible to avoid this throw? I understand that using exceptions for non-exceptional cases is bad form. Or does the api foist this off onto the caller and we're stuck with it?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)

> I'm not 100% sure I understand your proposal. But what I get from it is that
> we can skip the background scheduling and perform the read on the current
> thread if we are not the UI thread.

Kinda. I'm suggesting avoiding background work altogether if we can.

If this were like SharedPreferences, then we'd be trivially able to avoid a lot of complexity: just pretend that the prefs are always in memory.

As it's not that way, I'd suggest doing some work in that direction ourselves: fetch the bundle of restrictions synchronously, on the main thread, muffling StrictMode, and cache the results.

If the first time you need the values is from the main thread, that's when you'll do the fetch -- in that case we can't realistically background the work, so just muffle and we'll suck up the stall.

If the first time is in the background, we're fine to do the IO.

A small extension from there is to notice when the cache perhaps should be invalidated -- e.g., flip a flag in GeckoApp.onResume. If we try to retrieve a restriction from the UI thread, always use the cache, ignoring that flag. If we try to retrieve from a background thread, and that flag is flipped, eat those 14msec again and invalidate the cache there and then.

Beyond that it's just thread safety.

Does that make sense?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #6)
> Does that make sense?

Yes! Now I got it. I was so focused on only invalidating the cache if the restrictions have actually changed (and this is close to impossible) that I didn't look for the simple solutions. Doing this in onResume() is simple and elegant and will only over-invalidate rarely. As we always have to leave the app to change restrictions this will prevent us from returning stale values. Very nice.

I'll update my patch. :)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1189347-strict-mode-part2-v2.patch

Updated patch. Tested it with extensive logging and it's looking good: Just one read on the UI thread max.

Comment 9

[Allison Naaktgeboren :ally]

Comment on attachment 8649903 [details] [diff] [review]
1189347-strict-mode-part2-v2.patch

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

I like simple(r) solutions. :)

This looks good to me on the kidfox side. To the best I can make out, readRestrictions() is fine. Though I defer to rnmewan on that block of code as I have not worked with Android's thread policy objects directly. There may be side effects that I am unaware of.

I look forward to learning something from his review.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8649903 [details] [diff] [review]
1189347-strict-mode-part2-v2.patch

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

LGTM apart from thread safety errors.

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java
@@ +39,4 @@
>  
>      public RestrictedProfileConfiguration(Context context) {
>          this.context = context.getApplicationContext();
> +        this.isCacheInvalid = true;

Just put this in the declaration above.

@@ +43,5 @@
>      }
>  
>      @Override
>      public boolean isAllowed(Restriction restriction) {
> +        if (isCacheInvalid || !ThreadUtils.isOnUiThread()) {

This is checked from a background thread.

isCacheInvalid needs to be volatile or a more sophisticated concurrency construct. E.g., you might use an AtomicBoolean here and do a get-and-set operation rather than checking and assigning to false below.

@@ +44,5 @@
>  
>      @Override
>      public boolean isAllowed(Restriction restriction) {
> +        if (isCacheInvalid || !ThreadUtils.isOnUiThread()) {
> +            cachedRestrictions = readRestrictions();

cachedRestrictions similarly needs to be thread safe.

@@ +50,3 @@
>          }
>  
> +        return  !cachedRestrictions.getBoolean(restriction.name, DEFAULT_RESTRICTIONS.contains(restriction));

s/  / /

@@ +91,5 @@
>      }
>  
> +    @Override
> +    public void update() {
> +        isCacheInvalid = true;

This is called from the UI thread.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1189347-strict-mode-part3.patch

Thanks for pointing this out. I was ruling out race conditions but I didn't think about data visibility.

I decided to synchronize reads and writes to isCacheInvalid and cachedRestrictions using the same lock. This will not only guarantee atomicity but also memory visibility without adding much complexity.


(In reply to Richard Newman [:rnewman] from comment #10)
> @@ +43,5 @@
> >      }
> >  
> >      @Override
> >      public boolean isAllowed(Restriction restriction) {
> > +        if (isCacheInvalid || !ThreadUtils.isOnUiThread()) {
> 
> This is checked from a background thread.
> 
> isCacheInvalid needs to be volatile or a more sophisticated concurrency
> construct. E.g., you might use an AtomicBoolean here and do a get-and-set
> operation rather than checking and assigning to false below.

Trivia: In this very special case this was actually safe. isCacheInvalid is only written on the UI thread so on the UI thread we will never have a stale value. On a background thread the value doesn't really matter because we are always proceeding to read restrictions. But of course this is neither explicit nor guaranteed to stay this way when the class is modified. But yeah, this was just a random observation and this behavior wasn't intended. ;)

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8650408 [details] [diff] [review]
1189347-strict-mode-part3.patch

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

::: mobile/android/base/restrictions/RestrictedProfileConfiguration.java
@@ +42,5 @@
>      }
>  
>      @Override
>      public boolean isAllowed(Restriction restriction) {
> +        synchronized (this) {

IIRC, you can add synchronized to an overridden method, so just make the method synchronized; it's equivalent.

  public synchronized boolean isAllowed…

@@ +92,5 @@
>      }
>  
> +    @Override
> +    public void update() {
> +        synchronized (this) {

Same.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #12)
> IIRC, you can add synchronized to an overridden method, so just make the
> method synchronized; it's equivalent.

Okay. I was trying to encapsulate the actual used meachnism inside the class. But as this class is only used through the interface this is probably not needed.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/f876d7b28d81fe7e60a382d09633e4797a58a6c3
changeset:  f876d7b28d81fe7e60a382d09633e4797a58a6c3
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 21 12:12:12 2015 +0200
description:
Bug 1189347 - RestrictedProfiles: Exit early in isGuest/RestrictedProfile() to avoid unnecessary disk reads. r=ally

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/0fd54ce01b937c51891b2f43a1810e28e8032f2b
changeset:  0fd54ce01b937c51891b2f43a1810e28e8032f2b
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 21 12:14:12 2015 +0200
description:
Bug 1189347 - RestrictedProfileConfiguration: Cache restrictions to avoid unnecessary disk reads. r=ally,rnewman

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/f876d7b28d81
https://hg.mozilla.org/mozilla-central/rev/0fd54ce01b93