Crash on first installation due to missing storageManager

[tobiasKaminsky]

java.lang.NullPointerException: Attempt to invoke virtual method 'com.owncloud.android.datamodel.OCFile com.owncloud.android.datamodel.FileDataStorageManager.getFileByPath(java.lang.String)' on a null object reference
        at com.owncloud.android.ui.activity.FileDisplayActivity.onStart(FileDisplayActivity.java:2573)
        at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1425)
        at android.app.Activity.performStart(Activity.java:7825)
        at android.app.ActivityThread.handleStartActivity(ActivityThread.java:3294)
        at 

1. uninstall app
2. install app
3. set up user account
   -> boom

@ezaquarii I suspect that this is from the user account changes and that storage manager is not set after adding a new account.

(Normally we do not set milestone, but this must be fixed before next major release).

[tobiasKaminsky]

Strange while debugging this seems to be some sort of race conditions, as i did not had this when debugging (which is obviously slower than a normal start).

[ezaquarii]

@tobiasKaminsky

TL;DR, this is pretty easy to fix, we just need to agree about the UX flow.

I checked the source code and line FileDisplayActivity.java:2573 does not contain a call to FileDataStorageManager:

android/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

Lines 2572 to 2574 in 8daa9a2

	final String lastDisplayedAccountName = mLastDisplayedAccount != null ? mLastDisplayedAccount.name : null;
	final boolean accountChanged = !user.getAccountName().equals(lastDisplayedAccountName);

There are however few calls to it slightly above it. I suspect that this crash comes from slightly different version that we have on master.

When looking at FileDisplayActivity.onStart method, we can see that there are couple of calls to getStorageManager(), but those calls are guarded by check for user object:

android/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

Lines 2539 to 2569 in 8daa9a2

	@Override
	public void onStart() {
	super.onStart();
	Optional<User> optionalUser = getUser();
	if (optionalUser.isPresent()) {
	/// Check whether the 'main' OCFile handled by the Activity is contained in the
	// current Account
	OCFile file = getFile();
	// get parent from path
	String parentPath = "";
	if (file != null) {
	if (file.isDown() && file.getLastSyncDateForProperties() == 0) {
	// upload in progress - right now, files are not inserted in the local
	// cache until the upload is successful get parent from path
	parentPath = file.getRemotePath().substring(0,
	file.getRemotePath().lastIndexOf(file.getFileName()));
	if (getStorageManager().getFileByPath(parentPath) == null) {
	file = null; // not able to know the directory where the file is uploading
	}
	} else {
	file = getStorageManager().getFileByPath(file.getRemotePath());
	// currentDir = null if not in the current Account
	}
	}
	if (file == null) {
	// fall back to root folder
	file = getStorageManager().getFileByPath(OCFile.ROOT_PATH); // never returns null
	}
	setFile(file);
	User user = optionalUser.get();

Please not that this piece of onStart() logic was moved there from onAccountSet callback that we used to call from various points of activity lifecycle. I remember that we had it called from different sub-classes and different lifecycle methods, so it was not easy to follow and I'd need to spend quite some time to recreate this flow again as it was anything by intuitive. The purpose of this change was to eliminate those complications tho, so we should not be alarmed here.

Now, storage manager should be created synchronously in BaseActivity while setting the user:

android/src/main/java/com/owncloud/android/ui/activity/BaseActivity.java

Lines 150 to 162 in 8daa9a2

	protected void setAccount(Account account, boolean savedAccount) {
	boolean validAccount = account != null && accountManager.setCurrentOwnCloudAccount(account.name);
	if (validAccount) {
	currentAccount = account;
	} else {
	swapToDefaultAccount();
	}
	if(currentAccount != null) {
	storageManager = new FileDataStorageManager(currentAccount, getContentResolver());
	capabilities = storageManager.getCapability(currentAccount.name);
	}
	}

Since the user setter also creates FileDataStorageManager when user is non-null, having a non-null user should guarantee having a non-null FileDataStorageManager as well.

When looking at BaseActivity.setAccount() you can see 1 dodgy place deserving closer investigation - line 155 - when we break the flow to jump out to account creation activity. I suspect that this is where the race condition is triggered: we don't set the account and FileDataStorageManager as well, but before account creation activity is launched, we somehow hit FileDisplayActivity.onStart().

We can do few things to fix this:

1. call finish(), as the file display activity is not functional without valid user account anyway; please give me a rope here, but as I understand we shall return via FirstRunActivity in this case, right?

2. do not call this block of code when storage manager is not available, rendering FileDisplayActivity uninitialized and not functional; I believe that this was the old behaviour before refactoring, when onAccountSet() was simply not called under such conditions, leaving the activty in not-fully-initialized-but-not-crashing-either state.

3. Create storage manager using anonymous user.

[ezaquarii]

Strange while debugging this seems to be some sort of race conditions, as i did not had this when debugging (which is obviously slower than a normal start).

I tested this flow quite heavily on my OnePlus 6 when doing this change, but never hit it. Given the above analysis - this is very possibly device-dependent behaviour. If you have a consitent reproduction steps, I'll need to ask you to test the fix, as I cannot reproduce it on my device.

[tobiasKaminsky]

If you have a consitent reproduction steps, I'll need to ask you to test the fix, as I cannot reproduce it on my device.

I am using an emulator and removed entire app and then installed it.

[tobiasKaminsky]

1. call `finish()`, as the file display activity is not functional without valid user account anyway; please give me a rope here, but as I understand we shall return via `FirstRunActivity` in this case, right?

Maybe this? With an info that adding account failed. Yes it should return back to FirstRunActivity then.

[ezaquarii]

Maybe this? With an info that adding account failed. Yes it should return back to FirstRunActivity then.

Hm... why should it fail?

This code path is triggered when there is simply no account available to switch to, ie. it could happen after you delete your currently selected account. It's not triggered when you failed to add account.