Kiwix-android: Creating a new note crashes the app

[abdulwd]

Describe the bug

The app crashes when I open a book and click on New Note.

Expected behavior

The app should not crash and New Note should be renamed to Notes and moved down the menu.

Steps to reproduce the behavior:

1. Open a book.
2. Go to menu -> New Note
   

Environment

• Version of Kiwix Android : develop
• Device : Redmi Note 3
• OS version : Android M

Logs

java.lang.StringIndexOutOfBoundsException: length=110; regionStart=43; regionLength=-8
        at java.lang.String.startEndAndLength(String.java:298)
        at java.lang.String.substring(String.java:1087)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.getTextAfterLastSlashWithoutExtension(AddNoteDialog.java:205)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.getArticleNotefileName(AddNoteDialog.java:188)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.onCreate(AddNoteDialog.java:105)
        at androidx.fragment.app.Fragment.performCreate(Fragment.java:2414)
        at androidx.fragment.app.FragmentManagerImpl.moveToState(FragmentManager.java:1418)

[Aditya-Sood]

@abdulwd you mean 'Add Note'? If so it doesn't happen on my device, even with a fresh install
Give more context about how it happened and more entries in the log if you can

[macgills]

@Aditya-Sood there is an error with the code as can be seen with the crash report.
at org.kiwix.kiwixmobile.main.AddNoteDialog.getTextAfterLastSlashWithoutExtension(AddNoteDialog.java:205)

if (rightmostSlash > -1 && rightmostDot > -1) {
      return (path.substring(rightmostSlash + 1, rightmostDot));
    }

some urls do not have extensions so it trys to create a substring(80, 40) for example. An update is required here.
@abdulwd the zim file you had open would have been beneficial to this bug report, Wikem would reproduce this

[Aditya-Sood]

some urls do not have extensions

@macgills are we sure about this? I was using article titles previously but @kelson42 had informed that they are never assured, while urls are

[macgills]

Positive, I did a release of 2.5.2 because some zim files would not load, the reason they would not load is they had no extensions and a NPE was happening in ZimContentProvider

[Aditya-Sood]

Did they have a name though? Like instead of .../Main_Page.html, atleast .../Main_Page?

[macgills]

yes, they are of that form

[Aditya-Sood]

So a fix wherein "if the file name doesn't contain a dot (.), I use the whole filename" works?

[macgills]

if it doesn't have an extension don't remove the extension.
substring(rightmostSlash + 1, rightmostDot>rightmostSlash?rightmostDot:string.length)

[abdulwd]

I'm getting a crash for all zims. ArchWiki 37.5 MB, InstallGentoo Wiki 1.8 MB, Vikidia 4.5 MB are some of them.

[Aditya-Sood]

That's weird - I have installgentoo & vikidia, they work fine. Was it a specific article that caused the crash or at the Main Page itself?

[Aditya-Sood]

@abdulwd can you add Log.d(TAG, "Url: " + articleUrl); after line 186 in AddNoteDialog and give us its output when it crashes?

[abdulwd]

@Aditya-Sood

08-14 16:50:14.789 18665-18665/org.kiwix.kiwixmobile D/AddNoteDialog: Url: content://org.kiwix.kiwixmobile.zim.base/A/%CE%9A%CF%8D%CF%81%CE%B9%CE%B1_%CE%A3%CE%B5%CE%BB%CE%AF%CE%B4%CE%B1

[abdulwd]

The crash is on line 204 because start index > end index.

[Aditya-Sood]

What article is this? (Name/title of the page that you opened)

[mhutti1]

I would make sure not to make assumptions about URLs. Be sure to correctly error handle and avoid crashing the app.

[julianharty]

FYI I reproduced this crash using Android Monkey which triggered the crash after a long stream of events.

E/kiwix   (10635): Exception reading article A/index.html from zim file
E/kiwix   (10635): java.io.IOException: write failed: EPIPE (Broken pipe)
E/kiwix   (10635):      at libcore.io.IoBridge.write(IoBridge.java:455)
E/kiwix   (10635):      at java.io.FileOutputStream.write(FileOutputStream.java:187)
E/kiwix   (10635):      at org.kiwix.kiwixmobile.data.ZimContentProvider$TransferThread.run(ZimContentProvider.java:486)
E/kiwix   (10635): Caused by: libcore.io.ErrnoException: write failed: EPIPE (Broken pipe)
E/kiwix   (10635):      at libcore.io.Posix.writeBytes(Native Method)
E/kiwix   (10635):      at libcore.io.Posix.write(Posix.java:202)
E/kiwix   (10635):      at libcore.io.BlockGuardOs.write(BlockGuardOs.java:197)
E/kiwix   (10635):      at libcore.io.IoBridge.write(IoBridge.java:450)
E/kiwix   (10635):      ... 2 more
D/kiwix   (10635): invalidUrl = false
D/kiwix   (10635): Loaded URL: null
W/AwContents(10635): nativeOnDraw failed; clearing to background color.
D/kiwix   (10635): Loaded URL: null
D/kiwix   (10635): invalidUrl = false

... lots of lines cut ...
E/ActivityManager(  821): App crashed! Process: org.kiwix.kiwixcustomphet
E/AndroidRuntime(10635): FATAL EXCEPTION: main
E/AndroidRuntime(10635): Process: org.kiwix.kiwixcustomphet, PID: 10635
E/AndroidRuntime(10635): java.lang.NullPointerException
E/AndroidRuntime(10635):        at org.kiwix.kiwixmobile.main.AddNoteDialog.getTextAfterLastSlashWithoutExtension(AddNoteDialog.java:201)
E/AndroidRuntime(10635):        at org.kiwix.kiwixmobile.main.AddNoteDialog.getArticleNotefileName(AddNoteDialog.java:188)
E/AndroidRuntime(10635):        at org.kiwix.kiwixmobile.main.AddNoteDialog.onCreate(AddNoteDialog.java:105)
E/AndroidRuntime(10635):        at androidx.fragment.app.Fragment.performCreate(Fragment.java:2414)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.moveToState(FragmentManager.java:1418)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.moveFragmentToExpectedState(FragmentManager.java:1784)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.moveToState(FragmentManager.java:1852)
E/AndroidRuntime(10635):        at androidx.fragment.app.BackStackRecord.executeOps(BackStackRecord.java:802)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.executeOps(FragmentManager.java:2625)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.executeOpsTogether(FragmentManager.java:2411)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManager.java:2366)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:2273)
E/AndroidRuntime(10635):        at androidx.fragment.app.FragmentManagerImpl$1.run(FragmentManager.java:733)
E/AndroidRuntime(10635):        at android.os.Handler.handleCallback(Handler.java:733)
E/AndroidRuntime(10635):        at android.os.Handler.dispatchMessage(Handler.java:95)
E/AndroidRuntime(10635):        at android.os.Looper.loop(Looper.java:157)
E/AndroidRuntime(10635):        at android.app.ActivityThread.main(ActivityThread.java:5633)
E/AndroidRuntime(10635):        at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime(10635):        at java.lang.reflect.Method.invoke(Method.java:515)
E/AndroidRuntime(10635):        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:896)
E/AndroidRuntime(10635):        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:712)
E/AndroidRuntime(10635):        at dalvik.system.NativeStart.main(Native Method)
W/ActivityManager(  821):   Force finishing activity org.kiwix.kiwixcustomphet/org.kiwix.kiwixmobile.main.MainActivity
D/PMS     (  821): acquireWL(426d0950): PARTIAL_WAKE_LOCK  ActivityManager-Launch 0x1 821 1000 null

My guess is that for whatever reason, the article was effectively null and therefore the call String notefileName = getTextAfterLastSlashWithoutExtension(articleUrl); passes an articleUrl of null which causes int rightmostSlash = path.lastIndexOf('/'); to crash with the NPE

[Aditya-Sood]

@mhutti1 as I had discussed previously, a unique article identifier is the basis for associating a note per article. Right now it's the name of the html file in the url of the article

I'm hopeful that once I know some of the specific articles which cause the crash I'll be able to pinpoint the cause of the error, but is there maybe an alternative fail-safe way of uniquely identifying articles that I can try?

[Aditya-Sood]

@julianharty I'm not familiar with using android monkey - does testing with it involve some interaction on your behalf?

the article was effectively null

Basically I am not clear about how the menu was accessible when there was no article open

[mhutti1]

@Aditya-Sood we should already have the logic to check this for bookmarks, maybe try there?

[Aditya-Sood]

@mhutti1 from what I could gather (& maybe evident from this line), bookmarks also use article urls to uniquely identify the bookmark

Only difference with notes is that here the file name at the end of the article (like Main_Page from .../Main_Page.html) is used to name the note file, while bookmarks use the whole url as is

[mhutti1]

Is there a reason you can't just use the whole URL?

[macgills]

A note is saved as a file so using a full url would create some less than glamorous paths I would think, it may be necessary to save it in a database.
right now it is saved in {External Storage}/Kiwix/Notes/ZimFileTitle/ArticleTitle.txt so user visible

[Aditya-Sood]

I'm using the file name to name the note file - Like Main_Page.txt for the url content://org.kiwix.kiwixmobile.zim.base/A/Main_Page.html
Using the whole url will, other than being unreadable, probably run afoul of the system by having a very long name

[Aditya-Sood]

@macgills saving it in a database will prevent the users from accessing the notes from outside the app
I think we don't need to abandon the approach just yet, waiting for one of the actual articles causing the crash

[macgills]

you can share just text itself, it does not have to be a file

[mhutti1]

Hmm. If you wanted to keep the file you could name them "Title - ID" and then link the url to id in a database to allow them to be found by the app.

[Aditya-Sood]

@macgills I think it will be cumbersome to share each note individually
@mhutti1 not sure I follow, could you clarify the new note file name you're suggesting?

[mhutti1]

Jurassic Park - Note #aedba31

[Aditya-Sood]

But this way users won't be able to tell one note from the other without opening it
One of the advantages of the current setup is that keeping them organised article-wise will help someone like a student keep notes corresponding to a specific topic (article) & quickly navigate to them

[Aditya-Sood]

@abdulwd could you share the name of the article for which you have given the url?

[julianharty]

@Aditya-Sood

@julianharty I'm not familiar with using android monkey - does testing with it involve some interaction on your behalf?

Android Monkey is a software tool that is provided by Google with the Android development tools. It has been available for many years and sends key events, etc. semi-randomly at Android apps. See https://github.com/kiwix/kiwix-android/wiki/Automated-Testing for some notes on using it with Kiwix. I'm using the terminal to run it. It might be possible to configure and run it in Android Studio too.

the article was effectively null

This can happen for many reasons (none of them ideal), so while we'd prefer it never to occur our software needs to cope with the article being 'null'

Basically I am not clear about how the menu was accessible when there was no article open

Good question, my guess is that other code continues even when the article is 'null' and therefore your code is enabled.

[julianharty]

@Aditya-Sood when the article is 'null' then there _isn't_ an article. Often this happens when there is a glitch reading content from the ZIM file. It's a bit like web browsers coping with HTTP errors for content that's requested by a web page, the browser may continue to do the best it can to render content even if there was an error retrieving an image file. I imagine Kiwix Android doing something similar.

[Aditya-Sood]

Thanks for explaining @julianharty, I'll add a null check for article url

[abdulwd]

@Aditya-Sood Here are the logs:

08-15 22:35:51.056 24128-24128/org.kiwix.kiwixmobile D/AddNoteDialog: Url: content://org.kiwix.kiwixmobile.zim.base/A/Main_page
08-15 22:35:51.056 24128-24128/org.kiwix.kiwixmobile D/AddNoteDialog: article: Main page
08-15 22:35:51.079 24128-24128/org.kiwix.kiwixmobile D/AndroidRuntime: Shutting down VM


    --------- beginning of crash
08-15 22:35:51.079 24128-24128/org.kiwix.kiwixmobile E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.kiwix.kiwixmobile, PID: 24128
    java.lang.StringIndexOutOfBoundsException: length=52; regionStart=43; regionLength=-8
        at java.lang.String.startEndAndLength(String.java:298)
        at java.lang.String.substring(String.java:1087)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.getTextAfterLastSlashWithoutExtension(AddNoteDialog.java:207)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.getArticleNotefileName(AddNoteDialog.java:190)
        at org.kiwix.kiwixmobile.main.AddNoteDialog.onCreate(AddNoteDialog.java:105)

And the title is Main page.

[Aditya-Sood]

@abdulwd which zim is this?

[abdulwd]

I mentioned a few of them. I think I used ArchWiki 37.5 MB.

[macgills]

I have also noticed a bug when you click on History with no zim file loaded, very easy to do just do it on a fresh install without first downloading. This seems to me to be a more fundamental problem with this ui being open and these options being present.
Not that there is anything else to be done for this ticket but something to consider for #1245 perhaps? Breaking the UI with the banner and list of books (NewTabView lets call it) into a new location/activity/fragment would also get rid of that bug where the view is scrollable because it is in a WebView #798