Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix Fix NPE FileDataStorageManager #13074

Merged
merged 9 commits into from
Jun 13, 2024

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Jun 6, 2024

  • Tests written, or not not needed

What this PR Does?

  • Get FileDataStorageManager only from DI, remove creation from SessionMix. Both are same no need to have both of it.
  • Make account non nullable

How to Test?

  1. Have 2 account in the app, switch accounts and check files for each account.
  2. Delete app and make fresh install
  3. Have 2 account in app and switch accounts and check FileDataStorageManager and DefaultAccount those variable must contain selected account from ManageAccount Dialog

@alperozturk96 alperozturk96 changed the title Use DI if getStorageManager is null Bugfix Fix NPE FileDataStorageManager Jun 6, 2024
@tobiasKaminsky
Copy link
Member

Hm. DI uses CurrentAccountProvider.
SessionMixin uses UserAccountManager, which extends from CurrentAccountProvider.

So I guess it is okay to use them.

But the real fix would be to find out why it is null in first place.
Else app might then bail out on next NPE inside SessionMixin.

Can you see a bit more of stracktrace to guess when this might happen?

@alperozturk96
Copy link
Collaborator Author

alperozturk96 commented Jun 6, 2024

Hm. DI uses CurrentAccountProvider. SessionMixin uses UserAccountManager, which extends from CurrentAccountProvider.

So I guess it is okay to use them.

But the real fix would be to find out why it is null in first place. Else app might then bail out on next NPE inside SessionMixin.

Can you see a bit more of stracktrace to guess when this might happen?

fun setAccount(account: Account?) {
        val validAccount = account != null && accountManager.setCurrentOwnCloudAccount(account.name)
        if (validAccount) {
            currentAccount = account
        } else {
            swapToDefaultAccount()
        }

        currentAccount?.let {
            val storageManager = FileDataStorageManager(getUser().get(), contentResolver)
            this.storageManager = storageManager
        }
    }

This function set the storage manager inside SessionMixin passed parameter can be null so currentAccount become a null.

So can we say both are the same?

@alperozturk96 alperozturk96 force-pushed the bugfix/fix-npe-FileDataStorageManager branch from 3365117 to f9ef6da Compare June 10, 2024 13:20
@alperozturk96 alperozturk96 force-pushed the bugfix/fix-npe-FileDataStorageManager branch from 8bf8de1 to 18194f8 Compare June 11, 2024 07:32
@alperozturk96 alperozturk96 dismissed ZetaTom’s stale review June 11, 2024 08:01

Not valid anymore

@alperozturk96 alperozturk96 requested a review from ZetaTom June 11, 2024 08:01
@alperozturk96
Copy link
Collaborator Author

/rebase

Copy link
Collaborator

@ZetaTom ZetaTom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my test device the app crashes on the first launch after a fresh install.

Logcat

FATAL EXCEPTION: main
Process: com.nextcloud.client, PID: 4224
java.lang.RuntimeException: Unable to create application com.owncloud.android.MainApp: java.lang.ArrayIndexOutOfBoundsException: length=0; index=0
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6717)
        at android.app.ActivityThread.access$1300(ActivityThread.java:237)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
Caused by: java.lang.ArrayIndexOutOfBoundsException: length=0; index=0
        at com.nextcloud.client.account.UserAccountManagerImpl.getCurrentAccount(UserAccountManagerImpl.java:157)
        at com.nextcloud.client.account.UserAccountManagerImpl.getUser(UserAccountManagerImpl.java:225) 
        at com.owncloud.android.utils.theme.MaterialSchemesProviderImpl.getMaterialSchemesForCurrentUser(MaterialSchemesProviderImpl.kt:48)
        at com.nextcloud.client.di.ThemeModule$Companion.provideMaterialSchemes(ThemeModule.kt:42)
        at com.nextcloud.client.di.ThemeModule_Companion_ProvideMaterialSchemesFactory.provideMaterialSchemes(ThemeModule_Companion_ProvideMaterialSchemesFactory.java:47)
        at com.nextcloud.client.di.ThemeModule_Companion_ProvideMaterialSchemesFactory.get(ThemeModule_Companion_ProvideMaterialSchemesFactory.java:37)
        at com.nextcloud.client.di.ThemeModule_Companion_ProvideMaterialSchemesFactory.get(ThemeModule_Companion_ProvideMaterialSchemesFactory.java:13)
        at com.owncloud.android.utils.theme.ViewThemeUtils_Factory.get(ViewThemeUtils_Factory.java:39)  
        at com.owncloud.android.utils.theme.ViewThemeUtils_Factory.get(ViewThemeUtils_Factory.java:12)  
        at com.owncloud.android.MainApp.onCreate(MainApp.java:301)
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1192)
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6712)
        at android.app.ActivityThread.access$1300(ActivityThread.java:237) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:223) 
        at android.app.ActivityThread.main(ActivityThread.java:7656) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

@alperozturk96
Copy link
Collaborator Author

UserAccountManagerImpl

AnonymousAccount logic added for fresh install.

@alperozturk96 alperozturk96 dismissed ZetaTom’s stale review June 11, 2024 12:22

Not valid anymore

@alperozturk96 alperozturk96 requested a review from ZetaTom June 11, 2024 12:22
@tobiasKaminsky
Copy link
Member

test-Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/13074-Unit-test-15-03

@alperozturk96 this seems to be related to your changes.

@tobiasKaminsky
Copy link
Member

Have you tested multiple accounts?

@alperozturk96
Copy link
Collaborator Author

Have you tested multiple accounts?

Yes. App is working as expected during my tests. I will fix test and update PR.

@alperozturk96 alperozturk96 force-pushed the bugfix/fix-npe-FileDataStorageManager branch from 032bd46 to fb9e2e0 Compare June 12, 2024 07:24
@alperozturk96
Copy link
Collaborator Author

test-Unit test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/13074-Unit-test-15-03

@alperozturk96 this seems to be related to your changes.

This test is not valid anymore. Account can't be null so I removed the test.

Copy link

Codacy

Lint

TypemasterPR
Warnings6969
Errors33

SpotBugs

CategoryBaseNew
Bad practice6464
Correctness6868
Dodgy code344344
Experimental11
Internationalization77
Multithreaded correctness66
Performance5757
Security1818
Total565565

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/13074.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@alperozturk96 alperozturk96 enabled auto-merge June 12, 2024 09:26
@alperozturk96 alperozturk96 merged commit a352271 into master Jun 13, 2024
21 checks passed
@alperozturk96
Copy link
Collaborator Author

/backport to stable-3.29

@tobiasKaminsky tobiasKaminsky deleted the bugfix/fix-npe-FileDataStorageManager branch October 7, 2024 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants