All tabs in STG are lost if icon is clicked prior to loading sequence

[zkhcohen]

Describe the bug
If you start Firefox and click the STG icon in the extensions bar before the initial loading sequence (black, rotating loading icon), all of the tabs in the STGs will be lost. The STGs themselves will remain, but the tab count will appear as "0" in each STG. Clicking on the tab groups or restarting Firefox does not restore the tabs. The only solution is to restore from backup.

To Reproduce
Steps to reproduce the behavior:

1. Open Firefox. Wait a moment for STG to load.
2. Close Firefox.
3. Quickly open Firefox and click on the STG icon as fast as possible. If done successfully, when you click on STG again, all of your tab groups will be empty.

Expected behavior
Following the steps above would not result in the loss of your saved tabs.

Desktop (please complete the following information):

• OS and version: Windows 11
• Firefox version: 112.0.1
• Simple Tab Groups version: 5.1

Additional context
I'll try to do some additional testing to narrow down the root cause. Let me know if logs or settings would assist in the troubleshooting process.

[Drive4ik]

Yes, you can enable debug mode in the settings and try to reproduce the problem. You can email me the log with a link to this ticket. I can't reproduce the problem yet.
You can even record a GIF animation)) https://www.cockos.com/licecap/

[zkhcohen]

Yes, you can enable debug mode in the settings and try to reproduce the problem. You can email me the log with a link to this ticket. I can't reproduce the problem yet. You can even record a GIF animation)) https://www.cockos.com/licecap/

Sent logs.

Download GIF here

[rifat0153]

I am facing a similar issue. After opening a tab group, if I close Firefox during loading I will lose all my saved tabs.

Gif Download

Firefox: 112.0.2 (64-bit)
STG: 5.2
OS: Windows 11

[zkhcohen]

Update 5/12/2024: The issue occurs without any other extensions added to Firefox, and without any of the default Firefox settings changed. I've also found that a reliable way to replicate the issue is to start and then close Firefox over and over again until the tabs disappear from the groups.

I think I'm getting closer to tracking down the specific culprit in the handling of storage. I also have a working dev build that I'm modifying, deploying and debugging locally. Note for anyone else who tries to build this extension locally, you need NPM 17.9.1 - ver. 22.1.0 doesn't work.

I think the stack looks something like this (with my own presumptive comments added):

At the top-level, the highlighted line in the following snippet returns nothing, causing the tab restoration process to abort:

    let [
        { tabsToRestore: tabsToRestoreNotModified },
        windows,
    ] = await Promise.all([
         // set tabsToRestoreNotModified equal to tabsToRestore
>       Storage.get({ tabsToRestore: [] }), 
        Windows.load(),
    ]);

Another level deeper, the highlighted line removes the "tabsToRestore" key from the local storage database. Commenting this out fixes the issue. Since this either requires "tabsToRestoreNotModified" == "tabsToRestore", and "tabsToRestoreNotModified" is defined by "tabsToRestore" above, prior to the restoration process, this either means that "tabsToRestore" still equals "tabsToRestoreNotModified", or, based on the logic below, "tabsToRestore" was emptied:

// set tabsInDB equal to tabsToRestore from local storage. we know tabsToRestore can't be null, otherwise we wouldn't have gotten this far into the function.
let { tabsToRestore: tabsInDB } = await Storage.get({ tabsToRestore: [] });
    // filter tabsInDB based on values in tabsToRestoreNotModified
    tabsInDB = tabsInDB.filter(t => {
        // filter out all of the tabs that exist in both lists, leaving just the unique tabs
        return !tabsToRestoreNotModified.some(tab => t.groupId === tab.groupId && t.url === tab.url && t.cookieStoreId === tab.cookieStoreId);
    });

    // if the lists are the same or tabsInDB is an empty array (due to tabsToRestore being empty), the length will be 0
    if (tabsInDB.length) {
        // if there are tabs that haven't been restored, set tabsToRestore to those tabs
        await Storage.set({ tabsToRestore: tabsInDB });
    } else {
           // if there are no tabs left to restore (because the lists match), then remove the tabsToRestore from the local storage DB
>        await Storage.remove('tabsToRestore');
    }

The lack of documented code is making it extremely difficult to determine exactly what the root cause is, but I'm fairly confident it was introduced with the following commit, at least symptomatically: 0f9e185

Cross-referencing it with the logs, it appears that even on runs where Storage.remove('tabsToRestore'); was called, subsequent runs don't necessarily fail. Maybe this suggests that the mechanism which is supposed to re-set the tabsToRestore fails in certain cases, and by not removing tabsToRestore from the database, it serves as a fallback?

Testing that theory, it appears to hold partially true. By opening Firefox, adding a tab to a group, closing Firefox and reopening it, repeating these steps multiple times, I am able to confirm that in some cases the new tab is lost. Interestingly, removing tabs doesn't prevent them from being reopened. In other words, you can only add tabs, not remove them from the list that will be restored.

Looking back at the old commits and cross-referencing it with the logs / behavior of the current 5.2 code, regardless of the new "missed tabs" functionality, the "tabsToRestore" needs to get reset at the end of the restore operation. This means that the "fix" of commenting-out the "await Storage.remove('tabsToRestore');" was purely obscuring the underlying issue.

This makes me believe that there's some race condition between the following two functions associated with event listeners. If "onRemovedWindow" was called before "onRemovedTab" (and/or "isWindowClosing" == false) and "tabsToRestore" had already been zeroed, then nothing would get appended to "tabsToRestore" so it would stay zeroed for the next session and trigger the failure.

function onRemovedTab(tabId, { isWindowClosing, windowId }) {
    const log = logger.start('onRemovedTab', tabId, { isWindowClosing, windowId });

    if (isWindowClosing) {
>       reCreateTabsOnRemoveWindow.push(tabId);
        log.stop('add to reCreateTabsOnRemoveWindow');
    } else {
        Cache.removeTab(tabId);
        log.stop('tab removed from Cache');
    }
}

const onRemovedWindow = catchFunc(async function (windowId) {
    const log = logger.start(['info', 'onRemovedWindow'], windowId);

    let groupId = Cache.getWindowGroup(windowId);

    if (groupId) {
        sendMessage('window-closed', { windowId });
    }

    Cache.removeWindow(windowId);

>   let tabsToRestore = Cache.getTabsSessionAndRemove(reCreateTabsOnRemoveWindow);

    reCreateTabsOnRemoveWindow = [];

    if (tabsToRestore.length) {
        log.info('start merge tabs');
        let { tabsToRestore: prevRestore } = await Storage.get({ tabsToRestore: [] });
        tabsToRestore = tabsToRestore.filter(tab => !prevRestore.some(t => t.groupId === tab.groupId && t.url === tab.url && t.cookieStoreId === tab.cookieStoreId));
>       await Storage.set({
            tabsToRestore: [...prevRestore, ...tabsToRestore]
        });

        log.info('stop merge tabs > start restoring tabs');

        restoringMissedTabsPromise = tryRestoreMissedTabs();
        await restoringMissedTabsPromise.catch(log.onCatch('tryRestoreMissedTabs'));
        restoringMissedTabsPromise = null;
        log.info('stop restoring tabs');
    }

    log.stop();
});