Feature/database errors #85

charlesmchen · 2017-01-06T16:41:00Z

michaelkirk · 2017-01-06T16:48:02Z

src/Storage/TSStorageManager.m

@@ -92,24 +110,52 @@ + (instancetype)sharedManager {
 - (instancetype)initDefault
 {
    self = [super init];
+
+    if (![self tryToLoadDatabase]) {
+        // Failing to load the database is a catastrophic.


you a word again.

michaelkirk · 2017-01-06T16:52:46Z

src/Storage/TSStorageManager.m

+        // or the keychain has become corrupt.  Either way, we want to get back to a
+        // "known good state" and behave like a new install.
+
+        BOOL shouldHavePassword = [[NSUserDefaults standardUserDefaults] objectForKey:TSStorageManagerHasDatabasePassword];


I think using fileExistsAtPath would get you the same information and save a couple lines farther down. It might even be more conceptually to-the-point since that's the file that this password corresponds to.

not a blocking concern.

Hmm, you're right. I was trying to avoid filesystem access, but "less state = more surface area for bugs."

michaelkirk · 2017-01-06T16:53:45Z

src/Storage/TSStorageManager.m

        }
+
+        [self deletePasswordFromKeychain];


Same as before:

We are missing the call to [TSAttachmentStream deleteAttachments]; which deletes any on-disk attachments. Instead of these lines, wipeSignalStorage is almost what we want but won't work as is.

Yup, see above.

Although... as discussed offline, I think we should convert our "media/blob" cache to a LRU cache where contents get evacuated based on some maximum size on disk. My understanding is that the app disk usage currently grows in an unbounded way, which is:

Burdensome on/troublesome for users.

A vector of serious "out of disk space" bugs.

Many users keep their phones very close to full at all times.

michaelkirk · 2017-01-06T17:00:44Z

src/Storage/TSStorageManager.m

+        OWSAnalyticsCritical(@"Could not load database");
+
+        // Try to reset app by deleting database.
+        [self deletePasswordFromKeychain];


We are missing the call to [TSAttachmentStream deleteAttachments]; which deletes any on-disk attachments. Instead of these lines, wipeSignalStorage is almost what we want but won't work as is - though see below for my comment there.

Okay, I split wipeSignalStorage into two methods to DRY this up.

michaelkirk · 2017-01-06T17:05:12Z

src/Storage/TSStorageManager.m

    [TSAttachmentStream deleteAttachments];
-
+    
    [[self init] setupDatabase];


I don't even think this setupDatabase bit is necessary, so maybe you could use this in multiple places above if you just remove this line.

It's unnecessary, since (IIRC) the only place this is called, we immediately kill the app. In which case the database would be setup on next launch. (Users access this functionality in Settings > Delete Account)

Though should we be killing the app is a different question. Probably not.

Huh! Also, why was this calling init?!? I'm going to take your suggestion remove this line and undo the "splitting" I mentioned above.

michaelkirk · 2017-01-06T17:17:41Z

src/Storage/TSStorageManager.m

+    // We determine the database password first, since a side effect of
+    // this can be deleting any existing database file (if we're recovering
+    // from a corrupt keychain).
+    NSData *databasePassword = [self databasePassword];


You know. THIS change made me curious, and I found out we were calling the cipher block which queries the keychain 25 times in quick succession on startup. =/

charlesmchen · 2017-01-06T18:05:43Z

PTAL @michaelkirk

michaelkirk · 2017-01-09T09:14:36Z

src/Util/OWSAnalytics.h

+//
+//  OWSAnalytics.h
+//
+//  Copyright (c) 2017 Open Whisper Systems. All rights reserved.


wanna run precommit on all these?

michaelkirk

LGTM

michaelkirk · 2017-01-09T09:16:02Z

src/Storage/TSStorageManager.h

@@ -24,7 +24,7 @@ extern NSString *const TSUIDatabaseConnectionDidUpdateNotification;
 - (void)setupDatabase;
 - (void)deleteThreadsAndMessages;
 - (BOOL)databasePasswordAccessible;
- (void)wipeSignalStorage;
+- (void)resetSignalStorage;


Just a heads up that this is used in Signal-iOS. Generally if there are breaking changes in ssk, try to open the corresponding Signal-iOS PR that can run it simultaneously.

Done signalapp/Signal-iOS#1594

michaelkirk · 2017-01-09T09:23:20Z

src/Storage/TSStorageManager.m

    self.database = nil;
+    _dbConnection = nil;


I prefer property getters wherever possible (even internally). My rationale is that some assumptions become easier when only getters, setters, dealloc, and init touch it.

I could be persuaded to always use ivars internally, but I don't think we should mix and match arbitrarily.

Sure, that sounds quite reasonable. In fact, the only reason I accessed the ivar here is because this property is declared as readonly in the header and there's no readwrite property as one might expect in a private anonymous category. Going forward I'll fix these as I find them!

// FREEBIE

…l and database load errors. // FREEBIE

// FREEBIE

michaelkirk suggested changes Jan 6, 2017

View reviewed changes

michaelkirk reviewed Jan 9, 2017

View reviewed changes

michaelkirk approved these changes Jan 9, 2017

View reviewed changes

charlesmchen force-pushed the feature/databaseErrors branch from 4ee4752 to 2aa0797 Compare January 10, 2017 21:46

charlesmchen added a commit to signalapp/Signal-iOS that referenced this pull request Jan 10, 2017

Update to reflect changes in SSK signalapp/SignalServiceKit#85.

1fc7f2c

// FREEBIE

charlesmchen mentioned this pull request Jan 10, 2017

Update to reflect changes in SSK https://github.com/WhisperSystems/Si… signalapp/Signal-iOS#1594

Merged

Detect, warn about and try to recover from database password retrieva…

c5cf79c

…l and database load errors. // FREEBIE

charlesmchen force-pushed the feature/databaseErrors branch from 2aa0797 to c5cf79c Compare January 10, 2017 21:56

charlesmchen added a commit to signalapp/Signal-iOS that referenced this pull request Jan 11, 2017

Update to reflect changes in SSK pod signalapp/SignalServiceKit#85.

80e3591

// FREEBIE

charlesmchen added a commit to signalapp/Signal-iOS that referenced this pull request Jan 11, 2017

Update to reflect changes in SSK signalapp/SignalServiceKit#85.

6106326

// FREEBIE

charlesmchen merged commit c5cf79c into master Jan 11, 2017

charlesmchen deleted the feature/databaseErrors branch January 11, 2017 22:13

charlesmchen added a commit to signalapp/Signal-iOS that referenced this pull request Jan 11, 2017

Update to reflect changes in SSK signalapp/SignalServiceKit#85.

ee63e91

// FREEBIE

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/database errors #85

Feature/database errors #85

charlesmchen commented Jan 6, 2017 •

edited

Loading

michaelkirk Jan 6, 2017

charlesmchen Jan 6, 2017

michaelkirk Jan 6, 2017

charlesmchen Jan 6, 2017

michaelkirk Jan 6, 2017

charlesmchen Jan 6, 2017 •

edited

Loading

charlesmchen Jan 6, 2017

michaelkirk Jan 6, 2017

charlesmchen Jan 6, 2017

michaelkirk Jan 6, 2017

charlesmchen Jan 6, 2017

michaelkirk Jan 6, 2017

charlesmchen commented Jan 6, 2017

michaelkirk Jan 9, 2017

michaelkirk left a comment

michaelkirk Jan 9, 2017

charlesmchen Jan 10, 2017

michaelkirk Jan 9, 2017

charlesmchen Jan 10, 2017

		[TSAttachmentStream deleteAttachments];


		[[self init] setupDatabase];

Feature/database errors #85

Feature/database errors #85

Conversation

charlesmchen commented Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesmchen Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesmchen commented Jan 6, 2017

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesmchen commented Jan 6, 2017 •

edited

Loading

charlesmchen Jan 6, 2017 •

edited

Loading