708192 - Detecting extension uninstall from content script should be documented

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Wladimir Palant]

Bug 666335 solves the issue with mid-session uninstall for one specific scenario. However, there are still other scenarios where the extension is uninstalled but the content script code stays around. Most common one should be event listeners - they aren't removed and will still be active.

After some digging through code and experimenting I figured out that adding these lines to my event listeners will make them "drop dead" if the extension is uninstalled:

  if (typeof self.port == "undefined")
    return;

I'm all but sure that this is the best way to detect the extension going away. At the very least, this needs to be properly documented. Of course, implementing and documenting an event that the content script could listen to ("extension is uninstalled, remove all your listeners now") would be even better.

Comment 1

[littlespark]

One possible way (using tabs, worker, postMessage)?

main.js:
const data = require("self").data;
const tabs = require("tabs");
var include = /.../, worker;

exports.main = function(options, callbacks) { 
    function addListener(tab)
    {
	if (tab != tabs.activeTab) return;
	if (typeof worker == "object") worker.postMessage("removeListener"); 
		
	if (!include.test(tab.url)) return;
        worker = tab.attach({ contentScriptFile: data.url('page.js'), });
        worker.postMessage("addListener");
    }
    if (options.loadReason != "startup") addListener(tabs.activeTab);
    tabs.on("ready", function (tab) { addListener(tab); });
    tabs.on("activate", function (tab) { addListener(tab); });
};

exports.onUnload = function (reason) {
    if (!reason || !/uninstall|disable/.test(reason)) return;
    if (typeof worker == "object") worker.postMessage("removeListener");
};

page.js:
self.on('message', function(message) {
    if (message == "addListener")
        window.addEventListener("mousedown", mousedown, true);
    else if (message == "removeListener")
	window.removeEventListener("mousedown", mousedown, true);
});

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Good: documenting the best practice.

Better: adding an event to make the best practice better.

Best: automatically removing those event listeners, destroying the content script contexts, and freeing up their memory when an addon is uninstalled.

Let's go for better or, ideally, best here, so moving this to the General component.

Comment 3

[Nils Maier [:nmaier]]

I did some tests myself, and found out that the ways I anticipated simply won't work (any longer?). Am I missing something obvious?

- Using require("unload").when does not work, as the messages are async and won't arrive as the content scope is destroyed in between.
- Using Worker.on("detach") does not work, as one cannot post messages to the worker when that event arrives. Anyway, it would have had the same problem as with "unload".

There is no way to get a Worker.postMessage/Worker.port.emit through.
Meaning there is actually no "best practice" to remove listeners again that can be documented...

Automatically removing event listeners would be pretty hard, as it is not just window.addEventListener, but also Node.addEventListener.

If we took AMO review policies[1] seriously this would mean we would have to deny full review and only grant prelim. to all SDK add-ons that use window./Node.addEventListener in page-mod and tab content-scripts.

[1] https://wiki.mozilla.org/AMO:Editors/EditorGuide/SpecialAddonTypes#Bootstrapped_.28Restartless.29_Add-ons

Comment 4

[Alexandre Poirot [:ochameau]]

I've already hit multiple times this missing "destroy"/"unload" event in workers. Without them we can say that it is almost impossible to handle this story properly.
Content scripts have been priliminary designed as something very close to greasemonkey/userscripts. And there is no way to handle unload in these projects.

About AMO review policies, I'd say that they meant listeners set to chrome content. In most cases we do not care that much about listeners set to web pages, they will most likely be collected when the tab is closed.

Havind said that, we are thinking about implementing "chrome-mod", an equivalent for page-mod but for chrome browser documents! In this usecase, we have to handle unloading step properly. So that I'd expect we are going to fix issues mentioned here while working on this new API. 
In the meantime, I think we really should land ASAP a destroy event.

Comment 5

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562