Open Bug 1223425 Opened 4 years ago Updated Yesterday

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

Categories

(WebExtensions :: General, defect, P3)

44 Branch
defect

Tracking

(Not tracked)

People

(Reporter: maciej.cwiek9, Assigned: Tomislav)

References

(Depends on 1 open bug)

Details

(Whiteboard: [runtime] triaged)

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.
Whiteboard: [runtime][berlin]
Whiteboard: [runtime][berlin] → [runtime][berlin][good first bug]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [runtime][berlin][good first bug] → [runtime]
Assignee: nobody → aswan
Whiteboard: [runtime] → [runtime] triaged
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.
Assignee: aswan → rob
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P3
Is it possible to implement this behavior by Fx57?
Duplicate of this bug: 1368830
Duplicate of this bug: 1370368
Duplicate of this bug: 1392067
Hey Rob, are you planning to work on this soon?
Flags: needinfo?(rob)
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.
Flags: needinfo?(rob)
Priority: P3 → P2
Ok, let me know if it slips off your schedule, I can take it.
Duplicate of this bug: 1390813
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?
(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).
> 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.
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.
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.
Rob, looks like we missed this for 57, would it make sense for zombie to take it, as per comment 8?
@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).
Assignee: rob → tomica
Product: Toolkit → WebExtensions
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.
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 )

+1

Might look into as part of bug 1583484.

Depends on: 1583484
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.