[API Improvement] Uninit disabled helper extensions

[Lej77]

Short description

Tree Style Tab helper extensions can inject style sheets or create sub panels that change how Tree Style Tab's sidebar looks. When any of these extensions are disabled the changes they made to Tree Style Tab's sidebar remain until Tree Style Tab is restarted. It would be nice if these changes instead could be reverted immediately when the helper extension is disabled.

Steps to reproduce

1. Start Firefox with clean profile.
2. Install TST.
3. Install TST Bookmarks Subpanel
4. Disable TST Bookmarks Subpanel from the about:addons page.

Expected result

The sub panel in Tree Style Tab's sidebar should be removed immediately,

Actual result

The sub panel in Tree Style Tab's sidebar remains until Tree Style Tab is restarted.

Proposed Solution

The code for detecting if the helper extension is disabled should be similar to #2128 but the helper extension needs to accept the message instead of Tree Style Tab.

This could be done with a new Tree Style Tab event. A helper extension would specify the event when it registers and then Tree Style Tab would send the event message and await the response (while catching any errors). When a true response (we are ignoring false responses to allow canceling) is recieved (or an error is caught) the helper extension's style sheets and sub panels are removed (The same as if the extension used the unregister-self message).

Here is an example helper extension that uses such an event:

async function registerToTST() {
    try {
        await browser.runtime.sendMessage(TST_ID, {
            type: 'register-self',
            name: browser.i18n.getMessage('extensionName'),
            listeningTypes: ['wait-for-shutdown'],
            style: `
.tab.unread {
    color: red;
}`,
        });
    }
    catch (e) {
        // TST is not available
    }
}

const onClosePromise = new Promise((resolve, reject) => {
    // If this promise doesn't do anything then there seems to be a timeout so it only works if the tracked extension (this extension) is disabled within about 10 seconds
    // after this promise is used as a response to a message. After that it will not throw an error for the waiting extension.

    // If we use the following then the returned promise will be rejected when the extension is disabled even for longer times:
    window.addEventListener('beforeunload', () => resolve(true));
});

browser.runtime.onMessageExternal.addListener((message, sender) => {
    switch (sender.id) {
        case TST_ID:
            switch (message.type) {
                case 'ready':
                case 'permissions-changed':
                    registerToTST(); // passive registration for secondary (or after) startup
                    break;
                case 'wait-for-shutdown':
                    return onClosePromise;
            }
            break;
    }
});
registerToTST(); // aggressive registration on initial installation

This example code would color unread tabs and if the extension is disabled then the style changes would be reverted immediately.

Edit: Limit the number of messages

After some more thought on this it might be smart to attempt to limit the number of messages that are kept open. I have done some simple testing and it doesn't seem like having a lot of open messages are a problem but it doesn't hurt to be cautious. The largest reason for many messages should be from extension's that re-register a lot, for example if they need to change the registered style sheet or listeningTypes. If Tree Style Tab sends a new wait-for-shutdown event message for every registration then that might become a lot of messages quite fast.

I can think of two ways to limit the number of messages:

1. Store wait-for-shutdown messages in an object

Every time Tree Style Tab sends a wait-for-shutdown message it could remember that message for the specific extension and re-use it if it hasn't been resolved.

Some code like this somewhere in Tree Style Tab could be used to allow this behavior:

const currentWaitForShutdownMessages = {};

// Handle helper extensions message:
browser.runtime.onMessageExternal.addListener((message, sender) => {
    switch (message.type) {
        case 'register-self':
            if (message.listeningTypes && message.listeningTypes.includes('wait-for-shutdown')) {
                // Handle `wait-for-shutdown` event.
                if (!currentWaitForShutdownMessages[sender.id]) {
                    currentWaitForShutdownMessages[sender.id] = (async function () {
                        try {
                            const shouldUninit = await browser.runtime.sendMessage(sender.id, { type: 'wait-for-shutdown' });
                            if (!shouldUninit) return;
                        } catch (error) {
                            // Extension was disabled.
                        } finally {
                            // Allow new wait message to be sent:
                            delete currentWaitForShutdownMessages[sender.id];
                        }
                        // Uninit the helper extension...
                    })();
                }
            }
            break;
    }
});

2. Allow helper extensions to cancel the wait-for-shutdown event message

The promise that the helper extension returned could be canceled by resolving it to a false value. If this is allowed then the helper extension could ensure that only one message is ever keep open.

Example of code that could be used in the helper extension to only ever keep one wait message open:

let resolveLastWaitMessage = null;
window.addEventListener('beforeunload', () => {
    if (resolveLastWaitMessage) resolveLastWaitMessage(true)
});

browser.runtime.onMessageExternal.addListener((message, sender) => {
    switch (sender.id) {
        case TST_ID:
            switch (message.type) {
                case 'wait-for-shutdown':
                    return new Promise(resolve => {
                        // If this promise doesn't do anything then there seems to be a timeout so it only works if the tracked extension (this extension) is disabled within about 10 seconds
                        // after this promise is used as a response to a message. After that it will not throw an error for the waiting extension.

                        // If we resolve the promise when the `beforeunload` event is triggered then the returned promise will be rejected when the extension is disabled even for longer times:

                        // Handle wait canceling:
                        if (resolveLastWaitMessage) resolveLastWaitMessage(false); // Cancel the last returned promise by resolving it to `false`.
                        resolveLastWaitMessage = resolve;
                    });
            }
            break;
    }
});

Environment

• Platform (OS): Windows 10
• Version of Firefox: 67
• Version (or revision) of Tree Style Tab: 3.1.1

[piroor]

Thank you for proposing! I've implemented that, and they looks to be working as expected.

[piroor]

I still need to add document for this mechanism.

[Lej77]

Thanks for implementing this!

I updated the wiki with info about this event. If you don't like something I wrote then feel free to change it however you like!

I was also thinking about updating the sub panel wiki to include the unregister event but I don't know if that would needlessly complicate it.

[piroor]

Thanks a lot! I've updated examples in those sections and updated the document of Subpanel API.

[Binly42]

just to be clear...

IIUC, a naive empty promise should be simply keeping pending, i.e. there should not be a timeout ...

I just tested in devtool console, might also test for real webextension scene whenever spare ~

---

PS: what i talk above is just about:

from (wiki api doc](https://github.com/piroor/treestyletab/wiki/API-for-other-addons#unregister-from-tst):

const promisedShutdown = new Promise((resolve, reject) => {
  // If this promise doesn't do anything then there seems to be a timeout
  // so it only works if the tracked extension (this extension) is disabled
  // within about 10 seconds after this promise is used as a response to a
  // message. After that it will not throw an error for the waiting extension.

  // If we use the following then the returned promise will be rejected when
  // this extension is disabled even for longer times:
  window.addEventListener('beforeunload', () => resolve(true));
});

[Lej77]

@Binly42

The mentioned "timeout" would be in Firefox's code somewhere not in the extension's JavaScript code.

I tested the code by doing something like:

1. A WebExtension is installed/enabled.
2. TST sends a message to the new extension using browser.runtime.sendMessage.
3. That new extension returns promisedShutdown from its browser.runtime.onMessageExternal event handler.
4. We wait X number of seconds.
5. We disabled the new extension.

If we define the promise as const promisedShutdown = new Promise(() => {}) then the promise TST got from browser.runtime.sendMessage will be rejected with an error if X is less than about 10 seconds, it we waited longer than that then the promise will remain pending.

If we instead define the promise as

const promisedShutdown = new Promise((resolve, reject) => {
  window.addEventListener('beforeunload', () => resolve(true));
});

then the promise TST gets from browser.runtime.sendMessage will always be rejected with an error no matter how many seconds X is.

[piroor]

@Lej77 Thanks, I've updated the section https://github.com/piroor/treestyletab/wiki/API-for-other-addons#unregister-from-tst to recommend using non-blank promise.