1236956 - Crash in Storage: specialized SQLiteBookmarks.isBookmarked(String) -> Deferred<Maybe<Bool>> + 1144

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Aaron Train [:aaronmt]]

Incident Identifier: A1482E18-751D-4863-B144-F65883242DA8
CrashReporter Key:   89eb3536dffb2553a1f5f571c1b35820e89a1e2b
Hardware Model:      iPhone6,1
Process:             Client [5538]
Path:                /private/var/mobile/Containers/Bundle/Application/A4FCCC8E-1827-470E-A64E-DE1F9D93D237/Client.app/Client
Identifier:          org.mozilla.ios.Firefox
Version:             1310 (1.3)
Code Type:           ARM-64 (Native)
Parent Process:      launchd [1]

Date/Time:           2016-01-04 08:42:35.35 -0800
Launch Time:         2016-01-04 08:34:19.19 -0800
OS Version:          iOS 9.2 (13C75)
Report Version:      105

Exception Type:  EXC_BREAKPOINT (SIGTRAP)
Exception Codes: 0x0000000000000001, 0x0000000100ad9efc
Triggered by Thread:  0

Thread 0 name:
Thread 0 Crashed:
0   Storage                       	0x0000000100ad9efc specialized SQLiteBookmarks.isBookmarked(String) -> Deferred<Maybe<Bool>> + 1144 (BrowserDB.swift:241)
1   Storage                       	0x0000000100ad9d60 specialized SQLiteBookmarks.isBookmarked(String) -> Deferred<Maybe<Bool>> + 732 (BrowserDB.swift:240)
2   Storage                       	0x0000000100a70b1c protocol witness for BookmarksModelFactory.isBookmarked<A where ...>(String) -> Deferred<Maybe<Bool>> in conformance MergedSQLiteBookmarks + 96 (SQLiteBookmarks.swift:735)
3   Client                        	0x00000001001872fc specialized BrowserViewController.(updateURLBarDisplayURL in _432ECC1ACC532119E7710898743C92E9)(Browser) -> () + 1264 (BrowserViewController.swift:862)
4   Client                        	0x0000000100187bd8 specialized BrowserViewController.(updateToolbarStateForTraitCollection in _432ECC1ACC532119E7710898743C92E9)(UITraitCollection) -> () + 2040 (BrowserViewController.swift:185)
5   Client                        	0x0000000100187e48 specialized BrowserViewController.willTransitionToTraitCollection(UITraitCollection, withTransitionCoordinator : UIViewControllerTransitionCoordinator) -> () + 168 (BrowserViewController.swift:198)
6   Client                        	0x0000000100179db0 @objc BrowserViewController.willTransitionToTraitCollection(UITraitCollection, withTransitionCoordinator : UIViewControllerTransitionCoordinator) -> () + 68 (BrowserViewController.swift:0)
7   UIKit                         	0x000000018724d7e0 +[UIViewController _performWithoutDeferringTransitions:] + 128 (UIViewController.m:5902)

Comment 1

[Aaron Train [:aaronmt]]

~ #4 under the organizer for 1.3

Comment 2

[Richard Newman [:rnewman]]

    func withConnection<T>(flags flags: SwiftData.Flags, inout err: NSError?, callback: (connection: SQLiteDBConnection, inout err: NSError?) -> T) -> T {
        var res: T!
        err = db.withConnection(flags) { connection in
            var err: NSError? = nil
            res = callback(connection: connection, err: &err)
            return err
        }
        return res
    }

The error in this crash is in `return res`.

`res` is implicitly unwrapped. If the body doesn't run, or the callback somehow returns nil (which shouldn't be possible), then this will fail, because `res` is never assigned.

The body won't run in this case:

https://github.com/mozilla/firefox-ios/blob/v1.3b1309/Storage/ThirdParty/SwiftData.swift#L112

That is, when we're unable to obtain a DB connection, `res` won't have a value, and it will crash right here. That this isn't our top crash implies that our DB layer is fairly reliable!

`withConnection` is fundamentally wrong in this regard; it needs to return an optional, be provided a default, or be rewritten to use `throws` so it doesn't need to return, because it cannot guarantee that the provided closure will be called.

`inout` error parameters are the devil.


This doesn't strictly need STR; it's obvious what the bug is from code inspection. But if you want to try, just make `getSharedConnection` return nil sometimes.

Comment 3

[Richard Newman [:rnewman]]

Why getSharedConnection is failing is itself interesting. It's possible that the connection is being closed (forceClose does so, and could race), and it's possible that some other error is causing us to fail to open it in the first place.

I encourage whoever takes this bug to dig a little deeper, adding some kind of diagnostics if necessary, to find out how this can occur.

Comment 4

[Richard Newman [:rnewman]]

My hypothesis here is that we're attempting to check the bookmark state of a URL (twice, but that's a red herring) at the same time as a write is occurring on a different connection.

The attempt to fetch a connection in getSharedConnection will fail, presumably because of a SQLITE_BUSY. That'll lead to the callback not being invoked.

The question for reproducing this is then: how do we provoke a write on a different connection? And how do we do so prior to the first retrieval of the connection in the main app?

The latter points to this being the first launch of the browser -- much more work to do.

So:

* Clean install.
* [Perhaps] Share a URL to View Later. This will cause a tab to be opened immediately on launch, in case we don't check isBookmarked for about:home.
* Share a URL > Firefox > Add Bookmark.
* Very quickly, launch Firefox and open a page.

You might be able to reproduce this in the simulator:

* Create the profile by launching the app.
* Open a tab.
* Close and kill the app.
* sqlite3 browser.db
* Begin a transaction and start a very large write, probably by reading a file that creates a new table and inserts lots and lots of data into it.
* At the same time, relaunch the app.

I'd try that first.

Comment 5

[Emily Toop (:fluffyemily)]

Attachment: Pull request

There are a number of potential causes of the failure to fetch a connection. This patch adds a throws to the withConnection function in BrowserDB that simply throws the NSError returned by SwiftData and then calling functions either rethrow or handle that error appropriately. This should prevent the crash and provide extra understanding of the cause of crash by logging the error generated.

I have also added extra logging in SwiftData optional init in order to try and get a clearer understanding of why the connection failed.

Comment 6

[Emily Toop (:fluffyemily)]

Since the first accidental recreation I've not been able to provide an STR

Comment 7

[Stephan Leroux [:sleroux]]

The try/catches look good. Just need to also update where we call the throwing methods in StorageTests with trys.

Comment 8

[Richard Newman [:rnewman]]

I left some comments on the PR.

Given that this isn't a complete fix, I'd rather see one that's less invasive -- rather than forcing all callers to `try`, instead guarantee that the callback will always be called.

Callers should already be prepared for connection operations to fail, and will be checking the error parameter, so we can just make sure they get a connection that'll fail.

I'd also like to make sure that we can easily spot this bug locally. That means crashing for this in debug builds!

Comment 9

[Richard Newman [:rnewman]]

Attachment: Alternative patch.

Comment 10

[Richard Newman [:rnewman]]

https://github.com/mozilla/firefox-ios/commit/cf2b875babe8fbe6ba612df059dcdae92a163cc5

Note that the underlying issue still persists, so I'll file a follow-up with my analysis from Comment 2.