1931902 - port.onDisconnected not fired when port.disconnect() is called immediately after connect()

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Rob Wu [:robwu]]

Attachment: port-disconnect-onDisconnect.zip

When port.disconnect() is called, it should dispatch the port.onDisconnected event at the other end that received the port via runtime.onConnect.

This used to work (and also works in Chrome), but regressed by bug 1316748.

STR:

1. Load the attached extension.
2. At example.com click on the button that the extension injected.
3. Open the global Browser Console (Multiprocess to show Content Messages), or the background script's console via about:debugging`

Expected:

• "Port onDisconnect" message should be logged.

Actual:

• "Port onDisconnect" message is not logged.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1316748

:zombie, since you are the author of the regressor, bug 1316748, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Rob Wu [:robwu]]

I found out why; when I have a moment I'll put up a patch.

Short version is: ProxyMessenger is responsible for forwarding port disconnections.

When a port is disconnected while the onConnect event is being dispatched, it is possible for the disconnection to get lost.

To fix that, we should wait for the pending status in portPromises before forwarding the disconnection message, at https://searchfox.org/mozilla-central/rev/b477cd37e845005dac8881427fa06a2771d993db/toolkit/components/extensions/ExtensionParent.sys.mjs#415,421

Comment 3

[Rob Wu [:robwu]]

When I apply the fix as I suggested in comment 2, the following test now fails:

$ ./mach xpcshell-test toolkit/components/extensions/test/xpcshell/test_ext_background_runtime_connect_params.js --sequential --verbose
...
FAIL test_backgroundRuntimeConnectParams - [test_backgroundRuntimeConnectParams : 266] A promise chain failed to handle a rejection: JSWindowActorParent.sendAsyncMessage: JSWindowActorParent cannot send at the moment - stack: _doSend@resource://gre/modules/ConduitsChild.sys.mjs:64:11
_send@resource://gre/modules/ConduitsParent.sys.mjs:303:18
_cast/promises<@resource://gre/modules/ConduitsParent.sys.mjs:345:42
_cast@resource://gre/modules/ConduitsParent.sys.mjs:345:28
sendPortDisconnect@resource://gre/modules/ExtensionParent.sys.mjs:430:18
recvConduitClosed@resource://gre/modules/ExtensionParent.sys.mjs:420:10
recvConduitClosed@resource://gre/modules/ConduitsParent.sys.mjs:233:25
actorClosed@resource://gre/modules/ConduitsParent.sys.mjs:250:12
didDestroy@resource://gre/modules/ConduitsParent.sys.mjs:481:9

When I undo my fix, I see that this is a pre-existing error. The test used to report the following message instead:

JavaScript error: resource://gre/modules/ConduitsChild.sys.mjs, line 64: InvalidStateError: JSWindowActorParent.sendAsyncMessage: JSWindowActorParent cannot send at the moment
INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "InvalidStateError: JSWindowActorParent.sendAsyncMessage: JSWindowActorParent cannot send at the moment" {file: "resource://gre/modules/ConduitsChild.sys.mjs" line: 64}]
_doSend@resource://gre/modules/ConduitsChild.sys.mjs:64:11
...

Conclusion: the new test failure is not an issue with my patch, but pre-existing.
The error is not harmless: throwing from this point prevents the next line (this.ports.delete(portId);) from running, which results in memory leaks.

Higher up the stack, in ConduitsParent there is also other cleanup in actorClosed being skipped, potentially skipping recvConduitClosed calls in some cases. The byActor.delete(actor); call is also skipped, but might potentially be saved by byActor being a WeakMap (= will be GC'd, unless this setup keeps a strong reference).

I'll fix that while I'm at it.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1931902 - Improve port.onDisconnect reliability

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1931902 - Test case for disconnect() after connect()

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67cb497810b5
Improve port.onDisconnect reliability r=rpl
https://hg.mozilla.org/integration/autoland/rev/f55b9bf597ce
Test case for disconnect() after connect() r=rpl

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/67cb497810b5
https://hg.mozilla.org/mozilla-central/rev/f55b9bf597ce