Self testing internal error

IzzySoft commented 6 years ago

I finally found the time to install the app and give it a quick look. Optically, it's already a clear improvement over the 2014 version :wink:

But: whenever I open my safe, I get the self_test_internal_error ("unbehandelter Fehler während des Integritätstests"). As the app currently is still read-only, I don't worry much about what might be going on there (it won't save back a probably broken file) – but that should be fixed before editing becomes possible, or users might get very nervous.

OldSparkyMI commented 6 years ago

Hello @IzzySoft,

are you able to provide a test file? All my files, created with Revelation 0.4.14, worked flawless. With which revelation version did you created your file?

thanks in advance P.S.: Thanks for your time and support!

IzzySoft commented 6 years ago

Well, as there hasn't been a new version for years it's v0.4.13-3 here on Linux Mint. And sorry if I cannot attach the example file as test file, because its contents are confidential :wink: I can try creating a new file and see how that behaves, though, and report back with my findings later.

Apart from the error message, I cannot see any misbehavior so far. The file seems to work fine.

OldSparkyMI commented 6 years ago

@IzzySoft any new information on this issue?

IzzySoft commented 6 years ago

Sorry, had no time yet to play with a new file (quite busy these … well, all days). Not forgotten, though!

OldSparkyMI commented 6 years ago

I will close this bug in 11 days, if there is no further response.

IzzySoft commented 6 years ago

It still yields with the latest version. But I haven't yet found time to check with another database, sorry. Any ideas on how to "repair" the database are welcome, if you think the problem is there. Some "debug code" might help as well, to put the finger closer on what's going on.

IzzySoft commented 6 years ago

OK, I've used the Linux version to export my password store to XML, then created a new database and imported from that XML. Copied the resulting file to my Android device, opened it with aRevelation – and the error message appeared again.

What next? Let's check some logcat:

# during startup (seems irrelevant, as several other apps cause this as well)
W/System  (21770): ClassLoader referenced unknown path: /data/app/de.igloffstein.maik.aRevelation-2/lib/arm64
W/System  (21814): ClassLoader referenced unknown path: /system/priv-app/CMLogger/lib/arm64
# opening the file
D/Documents(22094): onFinished() [content://com.android.externalstorage.documents/document/primary%3ADocuments%2FIzzyTest]
I/System.out(21770): Revelation file opened, result code is -1, file is content://com.android.externalstorage.documents/document/primary%3ADocuments%2FIzzyTest
D/ARevelation(21770): openAskPasswordDialog on onActivityResult()
D/ARevelation(21770): state already cleared
# around here the error happens – but no more corresponding log entries exist

Created another blank file, added a single record. Saved, copied to device, opened – same error still. Added an entry, saved the database, copied back to PC and tried to open it in Revelation: "The file IzzyTest contains invalid data". File cannot be opened, I get a stack trace instead. Note that while Revelation fails opening this, aRevelation still can open it.

As the "New" button in the app is inoperative, I cannot try with a fresh file from there.

Here comes the file before editing it in aRevelation:

IzzyTest2.zip

Password is your user name :wink:

OldSparkyMI commented 6 years ago

Thank you @IzzySoft for you report.

Copied the resulting file to my Android device, opened it with aRevelation – and the error message appeared again.

I can't reproduce this. aRevelation always displays:

Self testing passed. Editing is safe

But, I can reproduce the Revelation Error:

Traceback (most recent call last):
  File "/usr/bin/revelation", line 560, in __cb_drag_dest
    self.file_open(files[0])
  File "/usr/bin/revelation", line 1481, in file_open
    entrystore = self.__file_load(file, password)
  File "/usr/bin/revelation", line 1019, in __file_load
    return result
UnboundLocalError: local variable 'result' referenced before assignment

I will give it a look.

IzzySoft commented 6 years ago

Strange you cannot reproduce the error in aRevelation. Maybe something with the environment? On my end: Wileyfox Swift running CyanogenOS 13 (Android 6.0.1).

I've just verified again with v1.1, the error still persists. If you could send me a dummy you've created, I could test it on my device – who knows, might be something strange with Revelation. Unfortunately, aRevelation cannot yet create its own database, or I had tested that as well.

As for the error in Revelation: Apart from the underlying problem, that's a bug (unhandled exception: the variable result wasn't initialized; so if the outer try crashes before that, it's not set). Obviously it runs on an exception that is not caught – so we can rule out those defined in __file_load. My bet would be line 988 raising that "unknown exception" (in fact, that's the only place that could do so). But I've no idea what "other error" that could be (I don't programm in Python that frequently).

OldSparkyMI commented 6 years ago

So, Revelation can read aRevelation files for the first time. I wonder that nobody created an issue for @MarmaladeSky. It is fixed in aRevelation REL-1.1.1.

Nevertheless, the main reason this issue is all about is still unfixed. I currently test the application only with Android 7.1.2 (LineageOS). I have an old Sony Xperia Go with Android 5.1.1, maybe I can reproduce the error there.

IzzySoft commented 6 years ago

That's a good start! And why noone complained to MarmaladeSky is easy: that app is/was read-only, so the password file copy was only unidirectional (PC → Android, never back unless one lost the original).

Looking forward to part II. As I already indicated: give me an .apk with additional debug details, and I'll check with it. Maybe it's something that can be safely ignored, and the exception just needs to be caught.

OldSparkyMI commented 6 years ago

@IzzySoft please check with 1.1.2 again. The error should be gone.

IzzySoft commented 6 years ago

Will do so as soon as it pops up in my repo (tomorrow). Just tested with v1.1.1 and it was still there (as expected, as you fixed it only afterwards).

IzzySoft commented 6 years ago

Ah, so what. Triggered a manual update :innocent: And yes, indeed: instead of the error, it now reports success. Hope you didn't just change the text of the error message? :stuck_out_tongue_winking_eye:

OldSparkyMI commented 6 years ago

In my opinion the problem was very trivial, but hard to find because of a bad code style. So a little lecture for all programmers out there:

Try to avoid encapsulated IF-ELSE statements like this:

if (testing == SelfTestingResult.Different) {
  res.toastMessage = R.string.self_testing_super_warning;
} else if (testing == SelfTestingResult.Similar) {
  res.toastMessage = R.string.self_testing_warning;
} else if (BuildConfig.DEBUG && testing == SelfTestingResult.Identical) {
 res.toastMessage = R.string.self_testing_passed_message;
} else
  res.toastMessage = R.string.self_testing_internal_error;

Rule: Always use brackets, just always!

Why? Read this: https://martinfowler.com/articles/testing-culture.html

Why? Because of the indentation. If you fly over the code above, you very easily miss the "BuildConfig.DEBUG &&", if you write your code like this, it's harder to miss but not impossible:

if (testing == SelfTestingResult.Different) {
res.toastMessage = R.string.self_testing_super_warning;
} else {
if (testing == SelfTestingResult.Similar) {
res.toastMessage = R.string.self_testing_warning;
} else {
if (BuildConfig.DEBUG && testing == SelfTestingResult.Identical) {
  res.toastMessage = R.string.self_testing_passed_message;
} else {
  res.toastMessage = R.string.self_testing_internal_error;
}

But now, it is painful to read because of the language syntax, so better write:

switch (testing) {
case Different:
res.toastMessage = R.string.self_testing_super_warning;
break;
case Similar:
res.toastMessage = R.string.self_testing_warning;
break;
case Identical:  // may be fallthrough
if (BuildConfig.DEBUG) {
  res.toastMessage = R.string.self_testing_passed_message;
  break;
}
default:
res.toastMessage = R.string.self_testing_internal_error;
}

Finally, as bonus: Always write fallthrough (https://en.wikipedia.org/wiki/Switch_statement#Fallthrough) behind a case statement, if you like to fall through, e.g.:

switch (something) {
case ONE:  // fallthrough
...
case TWO:
...
break;
}

If you write this code without the "// fallthrough", I would reject your code in a code review.

IzzySoft commented 6 years ago

At the end of that first code block my head was already screaming: switch, case! Everything that has more than if…elseif…else (3 cases) should be made a switch. Even 2 cases, if you already expect more to pop up with a future version. Glad you came to the same result :rofl:

As for the "fallthrough": A harsh one to reject just for a comment missing, and looking petty at a first glance – but thinking about it, it's absolutely understandable: "Was the break left out intentionally – or is it a bug?" So in a code review, I'd mark such with "please either add the break or comment fallthrough if intended".

OK, final report: Everything discussed here seems to be fine:

no more self-testing error
file edited in aRevelation can be opened without issues in Revelation

:+1:

OldSparkyMI / aRevelation

Self testing internal error #2