fix omnibar crash when encountering tabs without title

[fbenkstein]

The "url" and "title" properties of tab objects are optional an can be null sometimes. When a tab with either of these properties being null is encountered the interactive search just breaks without updating the completion suggestions.

I'm using Firefox 121.0 on macOS 13.6.1 (Apple Silicon). I have too many tabs open and apparently sometimes tabs can end up with an url of about:blank and and no title. Both the
url and the title property are documented as "optional" in Firefox and Chrome which I
assume means they can both be null.

I've adapted the code accordingly. This fixes the issue for me and should not introduce any new
bugs.

[fbenkstein]

@philc: could you have a look at this, please? I currently have to reload the addon manually each time I restart firefox...

[philc]

Hey @fbenkstein, I spent quite some time trying to reproduce this and couldn't.

In Firefox, I tried accessing the URL and title of tabs using tabs.query from Vimium's background page under these cases:

• With Vimium missing the all-hosts permission
• With about:newtab and other restricted tabs open (e.g. about:addons)
• With other extension's options pages open
• With private new tab windows open
• Navigating to google.com with network disabled (resulting in a "Server Not Found" page)
• Navigating to about://abc (resulting in an "Invalid URL" page)

In all cases, the tabs had non-null urls and titles.

Could you try to further isolate under what conditions this issue occurs?

The Chrome and Firefox documentation says that these two properties can be null if Vimium does not have the all-hosts permission (which presumably your installation does), although even with this permission disabled, I couldn't get this to occur. Secondly, the Chrome docs say that these properties can be null if the tab URL has never been "committed", but I'm not sure how that could possibly interact with Vimium's Vomnibar.

It's clearly happening to you in the wild, so your change seems reasonable. However, for future "delete protection", we could use some documentation about why we're checking for null on these fields. Without a comment explaining further, the check will likely get removed in the future when doing routine simplification of the code.

[philc]

This null-check seems redundant if we're populating the url and title with empty strings above, right?

Of the two checks you have, I think I would prefer keeping only the second: allow these fields to be null, document why they can be null, and update the matching code to handle null fields.

[fbenkstein]

There were other places in the code like https://github.com/philc/vimium/blob/master/background_scripts/completion.js#L417 that suggest there is a built-in assumption that the url field is never null:

    const result = new Suggestion({
      queryTerms,
      description: "domain",
      // This should be the URL or the domain, or an empty string, but not null.
      url: domainsAndScores[0]?.[0] || "",
      relevancy: 2.0,
    });

In https://github.com/philc/vimium/blob/master/background_scripts/completion.js#L484 we're calling matches directly on tabs:

    const tabs = await chrome.tabs.query({});
    const results = tabs.filter((tab) => RankingUtils.matches(queryTerms, tab.url, tab.title));

That's why I had to do both changes.

[fbenkstein]

I've responded to your comments above, please let me know if you still want me to change something.

Regarding reproducability: IIRC when this occurred I often had many tabs (> 250) and many browser windows (> 8) open. Some of them run sites which use a lot of memory and/or CPU. Apparently such sites can get killed or unloaded in the background sometimes. I notice this when I revisit the tab and reloads, even if it had already been loaded. I think this might also happen when tabs are lazily reloaded when the session is restored after a restart.

After a browser update a few days ago I decided to close a bunch of windows and tabs which I hadn't looked at for weeks. Because installing the extension each time is cumbersome I tried to reinstall from https://addons.mozilla.org/en-US/firefox/addon/vimium-ff/ instead. It didn't crash! To reproduce I manually killed some tabs or browser processes but couldn't trigger the issue like this either. That means right now, I can't reproduce the issue. This is good news for me but bad news for this PR and potentially other folks who also experience this issue.

I understand your maintainability concerns. What do you think of gradually moving to TypeScript? I'm not sure how feasible this is but if it had a realistic chance of getting accepted as a PR, I'd like to give this a go. I believe using TypeScript would have exposed this issue and might also find other, similar issues, see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b13461134d5a379f41f0355dcf7e2259a56d38fb/types/webextension-polyfill/namespaces/tabs.d.ts#L148.

[fbenkstein]

The issue is happening again to me. I'm using Firefox 122.0 on an Apple M2 MacBook Air.

I build from the master branch (839c38e) and managed to catch it in the debugger this time. The tab object doesn't have a title property. It's only on this one tab, though.

Callstack

Tab object properties

Too many open tabs

I managed to track down the actual tab by looking at the window id and retrieving it from the console. At first I thought it was a tab was opened by what looks like a userscript auto-update. It had an URL but no content. When I reloaded it it downloaded the userscript (Content-Disposition: attachment). Closing it didn't fix the problem. Next to it was a tab which showed as title undefined and also displayed no content. I believe this might have been a tab created by tampermonkey when it tried to update and install the userscript in a previous session. Closing that as well makes the problem go away and the omnibar completion works again.

While I now have a workaround, i.e load extension from local filesystem, set breakpoint on line that raises the exception (background_scripts/completion.js:696), set breakpoint condition to thing == null, try to find actual tab and close it, it's still a bit cumbersome and might happen to other Vimium users as well.