869225 - StrictMode policy violation in GeckoApp.getSessionRestoreState(GeckoApp.java:1726)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Reading SharedPreferences from the main thread.

D/StrictMode(16578): StrictMode policy violation; ~duration=178 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=31 violation=2
D/StrictMode(16578): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1198)
D/StrictMode(16578): 	at android.app.SharedPreferencesImpl.awaitLoadedLocked(SharedPreferencesImpl.java:184)
D/StrictMode(16578): 	at android.app.SharedPreferencesImpl.getBoolean(SharedPreferencesImpl.java:241)
D/StrictMode(16578): 	at org.mozilla.gecko.GeckoApp.getSessionRestoreState(GeckoApp.java:1726)
D/StrictMode(16578): 	at org.mozilla.gecko.GeckoApp.onCreate(GeckoApp.java:1411)
D/StrictMode(16578): 	at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:343)
D/StrictMode(16578): 	at android.app.Activity.performCreate(Activity.java:5066)
D/StrictMode(16578): 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1102)
D/StrictMode(16578): 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2288)
D/StrictMode(16578): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2368)
D/StrictMode(16578): 	at android.app.ActivityThread.access$600(ActivityThread.java:151)
D/StrictMode(16578): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1330)
D/StrictMode(16578): 	at android.os.Handler.dispatchMessage(Handler.java:99)
D/StrictMode(16578): 	at android.os.Looper.loop(Looper.java:155)
D/StrictMode(16578): 	at android.app.ActivityThread.main(ActivityThread.java:5536)

Comment 1

[Richard Newman [:rnewman]]

On my local builds, this seems to totally kill about:home, and other background tasks that were queued up. Very annoying.

Comment 2

[Richard Newman [:rnewman]]

Actually, that might be a lie. Hard to tell with strict mode.

Comment 3

[Jan Henning [:JanH]]

Hmm, I'm not seeing those particular violations any more (even though we're still reading shared prefs to determine whether we should restore or not), but instead there are a number of those caused by checking whether our last session crashed or not. I think it should be safe to move those bits into the the background thread runnable that does the actual session file parsing.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 869225 - Move decision whether to restore session or not onto background thread.

This involves accessing the file system to check whether our last session crashed or not (as well as reading some shared preferences), so moving this onto the background thread as well avoids some strict mode warnings.

Review commit: https://reviewboard.mozilla.org/r/138842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138842/

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8867334 [details]
Bug 869225 - Move decision whether to restore session or not onto background thread.

https://reviewboard.mozilla.org/r/138842/#review145516

mShouldRestore seems to be used later on the UI thread -> Are you sure it's safe to set it on a background thread (that might run a bit later)? Also doesn't mShouldRestore need to be defined as volatile in this case? I didn't investigate too much here, so I r+ in case you think it's safe.

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8867334 [details]
Bug 869225 - Move decision whether to restore session or not onto background thread.

https://reviewboard.mozilla.org/r/138842/#review145516

I'm updating the commit message to update my reasoning, but yes, it should be a) safe and b) no worse than the mixed access we're already having today.

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8867334 [details]
Bug 869225 - Move decision whether to restore session or not onto background thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138842/diff/1-2/

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8867334 [details]
Bug 869225 - Move decision whether to restore session or not onto background thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138842/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89319dc2a1d0
Move decision whether to restore session or not onto background thread. r=sebastian

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89319dc2a1d0