[Urgent] Version 2.6.6 crash

[misaochan]

New crash in version 2.6.6 affecting 12 users with 79 reports. :/ It appears to be caused by uploading. I tried to contact the user to get more information, but we'll see if he responds.

java.lang.NullPointerException: 
  at fr.free.nrw.commons.modifications.ModificationsSyncAdapter.onPerformSync (ModificationsSyncAdapter.java:105)
  at android.content.AbstractThreadedSyncAdapter$SyncThread.run (AbstractThreadedSyncAdapter.java:272)

(Yes, the stack trace is only 2 lines. Not sure why)

Android 5.1 | 60 | 75.9%
Android 7.0 | 10 | 12.7%
Android 8.0 | 5 | 6.3%
Android 6.0 | 2 | 2.5%
Others | 2 | 2.5%

A37f (A37f) | 59 | 74.7%
ZenFone 3 Max (ZC520TL) (ASUS_X008_1) | 8 | 10.1%
Mi A1 (tissot_sprout) | 3 | 3.8%
OnePlus3T (OnePlus3T) | 2 | 2.5%
Others | 7 | 8.9%

[misaochan]

@psh @maskaravivek This appears to be caused by a NPE from the line:

if (contrib.getState() == Contribution.STATE_COMPLETED)

[psh]

Reading the code in context I see something straight away

contributionCursor.moveToFirst();
contrib = contributionDao.fromCursor(contributionCursor);

if (contrib.getState() == Contribution.STATE_COMPLETED){
    ...
}

Firstly, we are not checking the return value of contributionCursor.moveToFirst() - it will return false if the cursor we are interacting with was empty (ie, we are unable to move anywhere). It's a common mistake to forget to check the return value of moveToFirst() and moveToNext() and leads to bugs every time.

I believe what's happening is that the cursor is empty, as there's a guard in fromCursor() -

if (cursor.getCount() > 0) {
    return ...
}
return null;

That's the most obvious reason I can see for the contribution being null on the line you mention. I think it would also be valid to add a simple null check

if (contrib != null && contrib.getState() == Contribution.STATE_COMPLETED){
    ...
}

but I am curious about the relationship between "modifications" and "contributions" - how can the data be out of sync in such a way that there's no corresponding contribution for the modification in question?

[misaochan]

Thanks @psh !

Unfortunately much of our modifications and contributions code is legacy code from 4+ years ago, and the logic behind them confuses me sometimes. :/ One of the reasons I can guess is that the contribution (upload) failed, but the modification (category selection) went through without checking for whether the upload worked.

However, if that was the case, that doesn't answer the question of why this only became an issue recently? Our first report of this crash ever was on 17 Jan, but the numbers were small (1 or 2 reports) before 21 Jan (2.6.6) where it exploded to 13 users and 132 reports.

I also wonder if the // This code is fraught with possibilities of race conditions, but lalalalala I can't hear you! line might be relevant to this problem, lol.

[misaochan]

From @whym at #1090 (copying over here for continuity):

Looks good to me. Upload went through for me, but I had't experienced the crash, so this is kind of expected.

Any idea on helping people report this issue without making them experiencing crashes?

We could perhaps add a log message when a null is detected in one of those variables. (Well, it may not be much of help, but at least people who can use logcat can help us.)

[neslihanturan]

Hi team, I am able to reproduce the bug with devices 5.0.2 and 4.42 API. It happens with category and without category uploads. When I follow debug break points, weirdly ModificationSyncAdapter is called even if I don't add any categories.

• I am able to upload with web site and see my new uploads from app's interface.
• When I upload an image with category it is normal ModificationSyncAdapter is being called. But according to my debug, it is called on uploads without categories and even with no uploads. And here is @misaochan 's great theory comes, whenever a category of previous uploads didn't added, ModificationSysncAdapter being called to add this missing category. It was a known issue Categories aren't added #310 . But currently it may trigger NPE.
• I was able to upload while testing our master before release, but at some point I couldn't be able to upload anymore (and I don't think this is API level dependent), guess what one of my category information is not added yet.

[misaochan]

@neslihanturan Thanks, this is very helpful! :) Additionally, could you please link the image that hasn't had categories added yet?

@psh The recent changes to ModificationsSyncAdapter were mostly the Dao injections that were added in #1047 . Do you think that could be relevant to this problem?

[neslihanturan]

Here: https://commons.wikimedia.org/wiki/File:Special_bread_from_Turkey.jpg

[neslihanturan]

Hi team, since this is one of our most critical issue I tried to investigate. I think it is related with #1042 and @Inject's creating an instance when it is necessary. But I am not a Dagger expert yet to detect the problem. However, since I am able to upload form my beta account and not able to upload from my production, I prepared a comparison to catch something. Sharing with you and hoping it will make a flash of inspiration of you. I have marked the differences between called methods.

If you want a detailed investigation of any other informations such as object references etc. please let me know. It seems like it is all I can do to help this discussion.

[misaochan]

Hi guys, apologies for my absence as I have been away this weekend. This is a fairly critical bug with the crash rate being 7.79% at the moment impacting ~300 user sessions. Unfortunately if a fix cannot be found soon we may need to revert the recent changes to ModificationsSyncAdapter for the time being. :(

[whym]

If the revert has to be done, will the result of the revert be the new master, or will it be done in a separate branch?

Ah I didn't see the link above. So it's in https://github.com/commons-app/apps-android-commons/tree/2.6.7 ?

[misaochan]

@whym Yeah, the revert is done in the 2.6.7 branch. That is also the branch that I will be releasing if it solves the crash for @neslihanturan . I am still unsure if it should be merged into master later though, or if we should keep it separate for the time being. What do you think?

Also, I should note that the 2.6.7 branch works well for me with the exception of the number of uploads (in the action bar) not being incremented after each upload. The revert seems to have broken that (AFAIK, no other changes were made, but I might have made a mistake in one of the conflict resolutions).