1223425 - port.onDisconnect isn't fired when extension is disabled/uninstalled

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Maciej Ćwiek]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

// background script
chrome.runtime.onConnect.addListener(function (port) {}); // just accept connection

// content script
var port = chrome.runtime.connect({name: "foo"});
port.onDisconnect.addListener(function () {
    console.log('disconnected');
});


Actual results:

The listener is not fired when either disabling or uninstalling the extension.
It only works when port.disconnect() is called explicitly.


Expected results:

The listener is should be fired when either disabling or uninstalling the extension, so the behaviour is aligned with Google Chrome API.

Comment 1

[Rob Wu [:robwu]]

This is not an uncommon pattern in Chrome extensions: Open a port to the background script in the content script, and if onDisconnect is triggered, assume that the extension has unloaded.

Comment 2

[Martin Kimmerle]

Is it possible to implement this behavior by Fx57?

Comment 6

[Tomislav Jovanovic :zombie]

Hey Rob, are you planning to work on this soon?

Comment 7

[Rob Wu [:robwu]]

I'll try to get this in 57, because extension messaging is a critical part of the extension API, and interoperability on that part would be nice.

Comment 8

[Tomislav Jovanovic :zombie]

Ok, let me know if it slips off your schedule, I can take it.

Comment 10

[Rob Wu [:robwu]]

This bug is about onDisconnect not firing upon uninstall/disable of an extension. Other bugs about not firing onDisconnect upon unload have a different root cause - e.g. bug 1370368, bug 1392067.


In Chrome, content scripts stay alive even after an extension is uninstalled (https://crbug.com/36403).
In Firefox, the content script sandbox is nuked when an extension is disabled.

The behavior of what a content script does upon unload is NOT SPECIFIED, and implementation-dependent:
- In Chrome, when an extension is unloaded, the exposed extension API becomes garbage, i.e. APIs can return undefined, throw an error and/or forget about existing API calls (never invoking callback).
- In Firefox, the API is also rendered useless before nuking the sandbox.

About port.onDisconnect under the circumstance of uninstalling an extension:
- In Chrome, port.onDisconnect could be called. Whether it happens or not is not documented nor guaranteed.
- In Firefox, port.onDisconnect is fired asynchronously when a context is unloaded. Since the sandbox is nuked upon nuking the sandbox, the onDisconnect event is not triggered either.

I guess that the "problem" of onDisconnect not firing upon uninstall will go away if we nuke the sandbox asynchronously (or dispatch onDisconnect synchronously).
However, I'm not sure if that is the best way to solve this bug. AFAIK, the only use for firing onDisconnected upon extension unload is to clean up the page (e.g. removing DOM elements). To support this scenario, it would be more useful to introduce a new event that fires after invalidating extension APIs, but before nuking the sandbox.
There is a similar feature request in Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=414213


To all who are following this bug:
- Why do you need onDisconnect when an extension unloads.
- Would you agree with not firing onDisconnect upon unload if there is another event that is fired in a content script when the extension is disabled?

Comment 11

[Tomislav Jovanovic :zombie]

(In reply to Rob Wu [:robwu] from comment #10)
> To all who are following this bug:
> - Why do you need onDisconnect when an extension unloads.
> - Would you agree with not firing onDisconnect upon unload if there is
> another event that is fired in a content script when the extension is
> disabled?

Unless another usecase is presented, I believe "cleanup" is the main need here.

And another event might be fine if that's significantly easier than onDisconnect.  But whatever the event is, if the listener returns a promise, we should delay nuking the sandbox until it resolves (for up to say 1 second) to enable async cleanup.

(timeout is needed because for addon upgrades, extension shutdown should block on this to avoid weird states of multiple versions of content scripts running on the same page).

Comment 12

[eejdoowad]

> Why do you need onDisconnect when an extension unloads.

My extension Saka Key https://github.com/lusakasa/saka-key provides keyboard-only browsing by intercepting all keydown/keyup DOM events. When the extension updates, the old event listeners must be removed, and new event listeners must be installed. Otherwise, the old event listeners will continue to be called, which causes errors and blocks the event from being received by the new event listeners.

> - Would you agree with not firing onDisconnect upon unload if there is
> another event that is fired in a content script when the extension is
> disabled?

That would suffice. I prefer solutions that work across browsers, but I already have several Firefox-specific workarounds related to extension messaging and script loading.

Comment 13

[e.bolgov]

Hello,

we have same problems in SEOquake extension - we add some DOM elements to page, to display additional data, but we can't clear them after add-on disable because of nuke of content script. It's important for our product to keep users page clean and don't require him to reload pages.

Especially you can meet this problem during live reload of add-on on it's update, for example after some period of working in browser AMO provides new version, and add-on auto-reloaded (or reloaded by button from UI) and all pages gets nasty not working elements from previous version.

Comment 14

[noitidart]

I haven't tested recently but here are my replies.

- Why do you need onDisconnect when an extension unloads.

I create ports to talk to things (content scripts and other stuff). On extension unload in the onDisconnect I have some clean up. A while ago, in Firefox and Edge, I recall onDisconnect not firing even when a tab is closed. I needed that tab closed onDisconnect to do clean up on the background page. I think Firefox is fixed for on tab close now.

- Would you agree with not firing onDisconnect upon unload if there is another event that is fired in a content script when the extension is disabled?

On extension disable, in my needs, it expects onDisconnect to fire in all browsers whenever either side of the port dies, I'm pretty sure that's how it works in Chrome.

Comment 15

[Andy McKay]

Rob, looks like we missed this for 57, would it make sense for zombie to take it, as per comment 8?

Comment 16

[Rob Wu [:robwu]]

@Andy sure.

To those add-on devs who are interested in detecting when an add-on is disabled in order to clean up the page, I explained how to do that even without any changes to Firefox's APIs:

https://stackoverflow.com/a/47361379/938089 (answer to question "Firefox WebExtension: How Do I Run Code Prior to Disable/Uninstall?").

I have successfully applied the method to an extension of mine, https://addons.mozilla.org/firefox/addon/youtube-lyrics-by-rob-w/ (version 3.14).
The content script part is in data/yt_lyrics.js (search for garbageTracker),
the background script part is in background.js (search for garbageTracker_newSheet).

Comment 17

[Thomas Wisniewski [:twisniewski]]

I'd just like to chime in here and say that my webcompat helper addon, Tinker Tester Developer Spy, would benefit from this. It hooks early into various DOM and script APIs, to let users diagnose how a page is behaving. It tries to clean up after itself using a trick like the one Rob mentioned in comment 16 (in its case, having the contentscript send a heartbeat signal to the pagescript), and having a more deterministic way to know that all listeners on a messageport are gone would simplify the logic there and make it much more reliable in edge cases like the addon being reloaded in about:debugging.

If there's already an idea for how we'd like to implement this, I wouldn't mind helping to implement it.

Comment 18

[Rob Wu [:robwu]]

I've talked with twisniewski on IRC, and among the use cases are restoring original JS objects in the page context (Object.defineProperty) and disabling listeners (such as Mutation Observers).
Currently their logic runs in the page. If content scripts could detect that they were about to be nuked, then the logic can be moved into the content script, and the clean-up logic be called upon extension unload/reload.

The event listener can be fired synchronously, there is no need for allowing the sandbox to continue to exist when the event listeners return.

This event will not fire page is in the bfcache (and neither retroactively when the page is revived). Extensions can use the pageshow/pagehide events in this case.

( I am open to including the pagehide event as a trigger for this new event, because otherwise there is no guarantee that the extension's pagehide listener is the last to execute.
If the extension replaces built-in methods with the intent of preventing pages from using it, then it is important that the extension's listeners run after the scripts in the page complete execution,)
( I have still not determined whether this event is tied to port.onDisconnect, or whether to introduce a new event. If tied to port.onDisconnect, then the question of whether to dispatch on pagehide is part of bug 1370368 )

Comment 19

[goldenratio]

+1

Comment 20

[Tomislav Jovanovic :zombie]

Might look into as part of bug 1583484.

Comment 21

[Shane Hughes [:aminomancer]]

Is there any news on this? I was hoping there would be a way to detect disable/uninstall from a background script alone. My extension has content scripts, but they're not loaded 100% of the time. They're only loaded on certain URLs. They receive messages from the background script and send messages back, and it's a long story to explain what exactly the purpose is, but they end up setting some global "variables" that are used by other extensions too. It's important that everything goes back to normal when the extension is updated, disabled, or uninstalled, regardless of the conditions under which the background script "disconnected."

There are a lot of things I've done to try to ensure that the variables are cleaned up when content scripts are unloaded or websockets are closed, but I'm trying to solve edge cases like the extension being disabled, uninstalled, or updated. In this case it wouldn't actually be enough to allow content scripts to receive events on disconnect from the background script, or even vice versa. A certain amount of jankiness is tolerable of course, but it would be ideal to have an event like "beforeOnDisconnect" which, like the name implies, fires once, just before an extension is disabled/uninstalled/updated. That way the background script itself can listen to the event (in cases where it is the ONLY script running in the extension) and execute its own cleanup.

I get that that could get pretty complicated — like, does allowing it to execute cleanup behavior mean allowing it to block the whole uninstall/disable/update process? That seems pretty dirty and even prone to abuse by malicious extensions, and would probably be an onerous burden to implement. I don't know how silly any of this sounds so bear with me lol. Maybe scrap the blocking but just allow the extension half a second to clean up? It'd be inconsistent but in my case, this would be just fine, since all that needs to happen on disconnect are a few assignments and messages. I think it'd be better than nothing. Alternatively, maybe the blocking could be implemented but limited to some given duration.

This would be much more restrictive, but it's just an idea: what if instead of executing its own cleanup, an extension could define some cleanup function in its manifest.json, and when the extension is initialized, that code is delivered to some internal module. It wouldn't share scopes or execution contexts with the extension so its functionality would be limited, but again, still way better than nothing at all. Basically save the code in some kind of special sandbox so the browser can execute it, even after (or while) the extension's contexts are popping off the stack.