Multiple fixes (fixes #871, fixes #1115, fixes #1116)

Handle storage permissions
Fix multiple processes being started.

app/src/main/AndroidManifest.xml

@@ -30,7 +30,8 @@
         android:name=".SyncthingApp">
         <activity
                 android:name=".activities.FirstStartActivity"
-                android:label="@string/app_name">
+                android:label="@string/app_name"
+                android:launchMode="singleInstance">
             <intent-filter>
                 <action android:name="android.intent.action.MAIN" />
                 <category android:name="android.intent.category.LAUNCHER" />

app/src/main/java/com/nutomic/syncthingandroid/activities/FirstStartActivity.java

@@ -107,6 +107,7 @@ public class FirstStartActivity extends Activity implements Button.OnClickListen
                         grantResults[0] != PackageManager.PERMISSION_GRANTED) {
                     Toast.makeText(this, R.string.toast_write_storage_permission_required,
                             Toast.LENGTH_LONG).show();
+                    this.finish();
                 } else {
                     startApp();
                 }

app/src/main/java/com/nutomic/syncthingandroid/activities/MainActivity.java

@@ -14,6 +14,7 @@ import android.content.pm.PackageManager;
 import android.content.res.Configuration;
 import android.graphics.Bitmap;
 import android.graphics.drawable.BitmapDrawable;
+import android.Manifest;
 import android.net.Uri;
 import android.os.Build;
 import android.os.Bundle;
@@ -24,6 +25,7 @@ import android.support.design.widget.TabLayout;
 import android.support.v4.app.Fragment;
 import android.support.v4.app.FragmentManager;
 import android.support.v4.app.FragmentPagerAdapter;
+import android.support.v4.content.ContextCompat;
 import android.support.v4.view.GravityCompat;
 import android.support.v4.view.ViewPager;
 import android.support.v4.widget.DrawerLayout;
@@ -255,6 +257,17 @@ public class MainActivity extends StateDialogActivity
         onNewIntent(getIntent());
     }
 
+    @Override
+    public void onResume() {
+        // Check if storage permission has been revoked at runtime.
+        if ((ContextCompat.checkSelfPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE) !=
+            PackageManager.PERMISSION_GRANTED)) {
+            startActivity(new Intent(this, FirstStartActivity.class));
+            this.finish();
+        }
+        super.onResume();
+    }
+
     @Override
     public void onDestroy() {
         super.onDestroy();

app/src/main/java/com/nutomic/syncthingandroid/service/DeviceStateHolder.java

@@ -65,7 +65,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
     }
 
     public interface OnDeviceStateChangedListener {
-        void onDeviceStateChanged();
+        void onDeviceStateChanged(boolean shouldRun);
     }
 
     private final Context mContext;
@@ -74,16 +74,22 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
     private final OnDeviceStateChangedListener mListener;
     @Inject SharedPreferences mPreferences;
 
-    private @Nullable NetworkReceiver mNetworkReceiver;
+    private @Nullable NetworkReceiver mNetworkReceiver = null;
-    private @Nullable BatteryReceiver mBatteryReceiver;
+    private @Nullable BatteryReceiver mBatteryReceiver = null;
-    private @Nullable BroadcastReceiver mPowerSaveModeChangedReceiver;
+    private @Nullable BroadcastReceiver mPowerSaveModeChangedReceiver = null;
 
     private boolean mIsAllowedNetworkConnection;
     private String mWifiSsid;
     private boolean mIsCharging;
     private boolean mIsPowerSaving;
 
+    /**
+     * Stores the result of the last call to {@link decideShouldRun}.
+     */
+    private boolean lastDeterminedShouldRun = false;
+
     public DeviceStateHolder(Context context, OnDeviceStateChangedListener listener) {
+        Log.v(TAG, "Created new instance");
         ((SyncthingApp) context.getApplicationContext()).component().inject(this);
         mContext = context;
         mBroadcastManager = LocalBroadcastManager.getInstance(mContext);
@@ -94,6 +100,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
     }
 
     public void shutdown() {
+        Log.v(TAG, "Shutting down");
         mBroadcastManager.unregisterReceiver(mReceiver);
         mPreferences.unregisterOnSharedPreferenceChangeListener(this);
 
@@ -166,9 +173,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
             mIsPowerSaving = intent.getBooleanExtra(EXTRA_IS_POWER_SAVING, mIsPowerSaving);
             Log.i(TAG, "State updated, allowed network connection: " + mIsAllowedNetworkConnection +
                     ", charging: " + mIsCharging + ", power saving: " + mIsPowerSaving);
-
+            updateShouldRunDecision();
-            updateWifiSsid();
-            mListener.onDeviceStateChanged();
         }
     }
 
@@ -183,15 +188,20 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
         }
     }
 
-    public void refreshNetworkInfo() {
+    public void updateShouldRunDecision() {
-        updateWifiSsid();
+        // Check if the current conditions changed the result of decideShouldRun()
-        mListener.onDeviceStateChanged();
+        // compared to the last determined result.
+        boolean newShouldRun = decideShouldRun();
+        if (newShouldRun != lastDeterminedShouldRun) {
+            mListener.onDeviceStateChanged(newShouldRun);
+            lastDeterminedShouldRun = newShouldRun;
+        }
     }
 
     /**
      * Determines if Syncthing should currently run.
      */
-    boolean shouldRun() {
+    private boolean decideShouldRun() {
         boolean prefRespectPowerSaving = mPreferences.getBoolean("respect_battery_saving", true);
         if (prefRespectPowerSaving && mIsPowerSaving)
             return false;
@@ -200,6 +210,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh
             boolean prefStopMobileData = mPreferences.getBoolean(Constants.PREF_SYNC_ONLY_WIFI, false);
             boolean prefStopNotCharging = mPreferences.getBoolean(Constants.PREF_SYNC_ONLY_CHARGING, false);
 
+            updateWifiSsid();
             if (prefStopMobileData && !isWhitelistedNetworkConnection())
                 return false;
 

app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java

@@ -26,6 +26,7 @@ public class NotificationHandler {
     private static final int ID_RESTART = 2;
     private static final int ID_STOP_BACKGROUND_WARNING = 3;
     private static final int ID_CRASH = 9;
+    private static final int ID_MISSING_PERM = 10;
     private static final String CHANNEL_PERSISTENT = "01_syncthing_persistent";
     private static final String CHANNEL_INFO = "02_syncthing_notifications";
     private static final String CHANNEL_PERSISTENT_WAITING = "03_syncthing_persistent_waiting";
@@ -171,6 +172,19 @@ public class NotificationHandler {
         mNotificationManager.notify(id, n);
     }
 
+    public void showStoragePermissionRevokedNotification() {
+        Intent intent = new Intent(mContext, FirstStartActivity.class);
+        Notification n = getNotificationBuilder(mInfoChannel)
+                .setContentTitle(mContext.getString(R.string.syncthing_terminated))
+                .setContentText(mContext.getString(R.string.toast_write_storage_permission_required))
+                .setSmallIcon(R.drawable.ic_stat_notify)
+                .setContentIntent(PendingIntent.getActivity(mContext, 0, intent, 0))
+                .setAutoCancel(true)
+                .setOnlyAlertOnce(true)
+                .build();
+        mNotificationManager.notify(ID_MISSING_PERM, n);
+    }
+
     public void showRestartNotification() {
         Intent intent = new Intent(mContext, SyncthingService.class)
                 .setAction(SyncthingService.ACTION_RESTART);

app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java

@@ -2,11 +2,14 @@ package com.nutomic.syncthingandroid.service;
 
 import android.app.Service;
 import android.content.Intent;
+import android.content.pm.PackageManager;
 import android.content.SharedPreferences;
+import android.Manifest;
 import android.os.AsyncTask;
 import android.os.Handler;
 import android.os.IBinder;
 import android.support.annotation.Nullable;
+import android.support.v4.content.ContextCompat;
 import android.util.Log;
 import android.widget.Toast;
 
@@ -91,7 +94,7 @@ public class SyncthingService extends Service {
     private ConfigXml mConfig;
     private RestApi mApi;
     private EventProcessor mEventProcessor;
-    private DeviceStateHolder mDeviceStateHolder;
+    private @Nullable DeviceStateHolder mDeviceStateHolder = null;
     private SyncthingRunnable mSyncthingRunnable;
     private Handler mHandler;
 
@@ -113,6 +116,31 @@ public class SyncthingService extends Service {
      */
     private boolean mStopScheduled = false;
 
+    /**
+     * True if the user granted the storage permission.
+     */
+    private boolean mStoragePermissionGranted = false;
+
+    /**
+     * Starts the native binary.
+     */
+    @Override
+    public void onCreate() {
+        super.onCreate();
+        PRNGFixes.apply();
+        ((SyncthingApp) getApplication()).component().inject(this);
+        mHandler = new Handler();
+
+        /**
+         * If runtime permissions are revoked, android kills and restarts the service.
+         * see issue: https://github.com/syncthing/syncthing-android/issues/871
+         * We need to recheck if we still have the storage permission.
+         */
+        mStoragePermissionGranted = (ContextCompat.checkSelfPermission(this,
+                                            Manifest.permission.WRITE_EXTERNAL_STORAGE) ==
+                                        PackageManager.PERMISSION_GRANTED);
+    }
+
     /**
      * Handles intents, either {@link #ACTION_RESTART}, or intents having
      * {@link DeviceStateHolder#EXTRA_IS_ALLOWED_NETWORK_CONNECTION} or
@@ -120,6 +148,18 @@ public class SyncthingService extends Service {
      */
     @Override
     public int onStartCommand(Intent intent, int flags, int startId) {
+        if (!mStoragePermissionGranted) {
+            Log.e(TAG, "User revoked storage permission. Stopping service.");
+            if (mNotificationHandler != null) {
+                mNotificationHandler.showStoragePermissionRevokedNotification();
+            }
+            stopSelf();
+            return START_NOT_STICKY;
+        }
+
+        mDeviceStateHolder = new DeviceStateHolder(SyncthingService.this, this::onUpdatedShouldRunDecision);
+        mNotificationHandler.updatePersistentNotification(this);
+
         if (intent == null)
             return START_STICKY;
 
@@ -136,35 +176,39 @@ public class SyncthingService extends Service {
                 new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
             });
         } else if (ACTION_REFRESH_NETWORK_INFO.equals(intent.getAction())) {
-            mDeviceStateHolder.refreshNetworkInfo();
+            mDeviceStateHolder.updateShouldRunDecision();
         }
         return START_STICKY;
     }
 
     /**
-     * Checks according to preferences and charging/wifi state, whether syncthing should be enabled
+     * After run conditions monitored by {@link DeviceStateHolder} changed and
-     * or not.
+     * it had an influence on the decision to run/terminate syncthing, this
-     *
+     * function is called to notify this class to run/terminate the syncthing binary.
-     * Depending on the result, syncthing is started or stopped, and {@link #onApiChange} is
+     * {@link #onApiChange} is called while applying the decision change.
-     * called.
      */
-    private void updateState() {
+    private void onUpdatedShouldRunDecision(boolean shouldRun) {
+        if (shouldRun) {
             // Start syncthing.
-        if (mDeviceStateHolder.shouldRun()) {
+            switch (mCurrentState) {
-            if (mCurrentState == State.ACTIVE || mCurrentState == State.STARTING) {
+                case DISABLED:
-                mStopScheduled = false;
+                case INIT:
-                return;
-            }
-
                     // HACK: Make sure there is no syncthing binary left running from an improper
                     // shutdown (eg Play Store update).
                     shutdown(State.INIT, () -> {
-                Log.i(TAG, "Starting syncthing according to current state and preferences");
+                        Log.i(TAG, "Starting syncthing according to current state and preferences after State.INIT");
                         new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
                     });
+                    break;
+                case STARTING:
+                case ACTIVE:
+                    mStopScheduled = false;
+                    break;
+                default:
+                    break;
             }
+        } else {
             // Stop syncthing.
-        else {
             if (mCurrentState == State.DISABLED)
                 return;
 
@@ -173,20 +217,6 @@ public class SyncthingService extends Service {
         }
     }
 
-    /**
-     * Starts the native binary.
-     */
-    @Override
-    public void onCreate() {
-        super.onCreate();
-        PRNGFixes.apply();
-        ((SyncthingApp) getApplication()).component().inject(this);
-        mHandler = new Handler();
-        
-        mDeviceStateHolder = new DeviceStateHolder(SyncthingService.this, this::updateState);
-        mNotificationHandler.updatePersistentNotification(this);
-    }
-
     /**
      * Sets up the initial configuration, and updates the config when coming from an old
      * version.
@@ -248,19 +278,27 @@ public class SyncthingService extends Service {
      */
     @Override
     public void onDestroy() {
+        if (mStoragePermissionGranted) {
             synchronized (mStateLock) {
                 if (mCurrentState == State.INIT || mCurrentState == State.STARTING) {
-                Log.i(TAG, "Delay shutting down service until initialisation of Syncthing finished");
+                    Log.i(TAG, "Delay shutting down synchting binary until initialisation finished");
                     mStopScheduled = true;
-
                 } else {
-                Log.i(TAG, "Shutting down service immediately");
+                    Log.i(TAG, "Shutting down syncthing binary immediately");
                     shutdown(State.DISABLED, () -> {});
                 }
             }
+        } else {
+            // If the storage permission got revoked, we did not start the binary and
+            // are in State.INIT requiring an immediate shutdown of this service class.
+            Log.i(TAG, "Shutting down syncthing binary due to missing storage permission.");
+            shutdown(State.DISABLED, () -> {});
+        }
 
+        if (mDeviceStateHolder != null) {
             mDeviceStateHolder.shutdown();
         }
+    }
 
     /**
      * Stop Syncthing and all helpers like event processor and api handler.
@@ -277,6 +315,7 @@ public class SyncthingService extends Service {
         if (mApi != null)
             mApi.shutdown();
 
+        if (mNotificationHandler != null)
             mNotificationHandler.cancelPersistentNotification(this);
 
         if (mSyncthingRunnable != null) {

app/src/main/res/values/strings.xml

@@ -578,6 +578,8 @@ Please report any problems you encounter via Github.</string>
 
     <string name="syncthing_disabled">Syncthing is disabled</string>
 
+    <string name="syncthing_terminated">Syncthing was terminated</string>
+
     <!-- Toast shown if syncthing failed to create a config -->
     <string name="config_create_failed">Failed to create config file</string>