1167288 - Startup crash in Browser.historyList

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This crash starts here:

extension TabManager : WKNavigationDelegate {
    func webView(webView: WKWebView, didFinishNavigation navigation: WKNavigation!) {
        storeChanges()
    }
}

Which goes to:

private func TabManager.storeChanges() {
    let storedTabs: [RemoteTab] = tabs.map(Browser.toTab)
    storage?.insertOrUpdateTabs(storedTabs)
}

Then calls:

class func Browser.toTab(browser: Browser) -> RemoteTab {
    return RemoteTab(clientGUID: nil,
        URL: browser.displayURL ?? NSURL(),
        title: browser.displayTitle,
        history: browser.historyList,
        lastUsed: Timestamp(),
        icon: nil)
}

And finally calls:

var Browser.historyList: [NSURL] {
    func listToUrl(item: WKBackForwardListItem) -> NSURL { return item.URL }
    var tabs = self.backList?.map(listToUrl) ?? [NSURL]()
    tabs.append(self.url!) <<< Crash because self.url is nil
    return tabs
}

Why is self.url nil? I think that is because i have multiple tabs open so at app startup, when the first webView(didFinishNavigation:) is fired, there is a change that the other tabs have not started loading yet so their URLs are nil.

However, this storeChanges code that is triggered loops over ALL tabs and assumes they are all loaded.

The quick fix is that URL is an optional and should thus not be assumed to be non-nil.

A bigger fix would be to delay storeChanges() at startup and not call it until all tabs have loaded. Or not call it at all because the tabs are most likely exacrtly the same as when the app was quit.

The latter is pribably difficult because of async loading and maintaining that state.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/488

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8608886 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/488

Let's file a follow-up to do this once, not N times.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Followup bug 1167310