Closed Bug 1167288 Opened 4 years ago Closed 4 years ago

Startup crash in Browser.historyList

Categories

(Firefox for iOS :: General, defect)

Other
iOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec + ---

People

(Reporter: st3fan, Assigned: st3fan)

References

Details

(Keywords: crash)

Attachments

(1 file)

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.
Status: NEW → ASSIGNED
tracking-fennec: ? → +
Keywords: crash
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.
Attachment #8608886 - Flags: review?(rnewman) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Followup bug 1167310
You need to log in before you can comment on or make changes to this bug.