Startup crash in Browser.historyList

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: st3fan, Assigned: st3fan)

Tracking

({crash})

unspecified
Other
iOS
crash
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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+
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 3

3 years ago
Followup bug 1167310
Blocks: 1167310
You need to log in before you can comment on or make changes to this bug.