Skip to content

Replace nsISocketTransportService with TCPSocket for TB 115+#1145

Open
heeen wants to merge 7 commits intothsmi:masterfrom
heeen:fix-tcpsocket-migration
Open

Replace nsISocketTransportService with TCPSocket for TB 115+#1145
heeen wants to merge 7 commits intothsmi:masterfrom
heeen:fix-tcpsocket-migration

Conversation

@heeen
Copy link
Copy Markdown
Contributor

@heeen heeen commented Mar 25, 2026

Summary

Replaces the broken socket experiment API with one based on Thunderbird's internal TCPSocket component, fixing the extension for TB 115+ (fixes #893, fixes #934).

Background

The old SieveSocketApi.js used nsISocketTransportService and nsISSLSocketControl.StartTLS() directly. In TB 115+, StartTLS can only be triggered from the network thread while JS runs on the main thread, making the old approach impossible. Every major TB release since TB 24 has broken the socket API in some way.

Approach

Uses the same TCPSocket component that Thunderbird itself uses for SMTP/IMAP (accessed via Cu.getGlobalForObject). The new experiment API is designed as a generic browser.tcpSocket.* API that aligns with the shape a future official WebExtension TCPSocket API might take.

Changes

  • New TCPSocket experiment API (ext-tcp-socket.js) with race-prevention proxy pattern -- registers listeners before connecting to avoid threading races between WebExtension and network threads
  • Async propagation through SieveAbstractClient.isAlive(), SieveAbstractClient.startTLS(), SieveAbstractSession.isConnected() and all callers
  • Integer port parsing in SieveUrl (TCPSocket expects integer port)
  • Updated wx client to use browser.tcpSocket.* API
  • Shutdown race fix -- guards event unregister callbacks with hasSocket() to prevent errors during extension reload
  • Stays on MV2 -- MV3's Limited Event Pages would suspend background scripts and kill active sieve connections
  • Minimum version bumped to 115.0
  • Old SieveSocketApi.js and SieveSocketApi.json removed (1042+ lines of broken code)

Based on work from the 893-does-not-work-with-tb-115 branch with bug fixes applied (Services.jsm removal, shutdownSockets typo, error handler variable shadowing, schema/implementation alignment).

Test plan

  • Build with npx gulp wx:package
  • Load in TB 128+ via about:debugging -> Load Temporary Add-on
  • Connect to a sieve server -- verify server greeting
  • Verify StartTLS upgrade works without crash
  • Test cert error handling with self-signed cert
  • Test disconnect/reconnect cycle
  • Reload extension -- verify no "Unknown socket" errors
  • Run unit tests: npm test (261 tests pass)

heeen added 7 commits March 25, 2026 13:26
Replace the broken nsISocketTransportService approach with a new
experiment API wrapping Thunderbird's internal TCPSocket component.
This is the same TCPSocket that TB uses for its own SMTP/IMAP
implementations, avoiding the StartTLS threading issue that broke
the extension in TB 115+.

The API separates socket creation from connection to prevent races
between the WebExtension thread and the network thread -- listeners
are registered on a proxy object before TCPSocket is constructed.

Based on work from the 893-does-not-work-with-tb-115 branch with
fixes: removed dead Services.jsm fallback, fixed shutdownSockets()
method call, aligned schema with implementation (getSSL, onError
parameters).
The new TCPSocket experiment API is fully async (cross-thread
WebExtension calls). Propagate async through the abstract base
classes and all callers to prepare for the socket API switch.
The TCPSocket API expects an integer port. Parse the port from the
URL string into a number at construction time and validate it,
rather than lazily stringifying the default port on access.
Follow-up to the SieveUrl integer port change -- update test
assertions from string "4190"/"1234" to integer 4190/1234.
Replace the broken browser.sieve.socket.* API (which used
nsISocketTransportService directly) with the new browser.tcpSocket.*
experiment API (which uses Thunderbird's internal TCPSocket component).

Key changes:
- create() no longer takes host/port; connect() takes them separately
- startTLS() replaced by upgradeToSecure()
- isAlive() now checks getReadyState() for "open"
- send() replaced by sendBytes() with ArrayBuffer
- Error handler correctly dispatches on type string parameter
- Minimum Thunderbird version bumped to 115.0

Fixes thsmi#893
The handlers use await for isConnected() but were not declared
as async functions, causing a SyntaxError at runtime.
When the extension reloads, shutdownSockets() destroys all sockets
before ExtensionCommon's EventManager fires removeListener callbacks.
Guard each unregister callback with hasSocket() to avoid "Unknown
socket" errors during shutdown.
@thsmi
Copy link
Copy Markdown
Owner

thsmi commented Apr 20, 2026

@heeen what is the intention behind this pull request?
It just seems do have cherrypicked files from the master...893-does-not-work-with-tb-115 and by doing so it misses the really important fixes.

Or did I miss something? Or is this just AI slop?

@heeen
Copy link
Copy Markdown
Contributor Author

heeen commented Apr 21, 2026

Fair question -- let me explain the intent and be transparent about what's original vs. derived.

The two new files (ext-tcp-socket.js and tcp_socket.json) are based on the ones from 893-does-not-work-with-tb-115. I wanted to get the socket fix out in a reviewable form, separated from the other work on that branch (MV3 migration, SieveSpace refactor, parser rework, UI polish, bootstrap updates).

What I added on top:

Removed the Services.jsm fallback - .jsm was removed in TB 128
Fixed shutdownSockets(): this.get(id) -> this.getSocket(id)
Fixed an error handler bug in SieveClient.mjs where error.type was checked after error had been reassigned to the type string parameter
Aligned the schema with the implementation: isSecure → getSSL, added the missing onError event parameters
Made account-connected handlers async (the branch has await inside non-async functions, which throws SyntaxError at load time)
Added hasSocket() guards to event unregister callbacks so extension reload doesn't throw "Unknown socket"

What I deliberately left out:

MV3 migration - MV2 is still supported in TB 128+, and Limited Event Pages suspending the background would kill active sieve connections. Staying on MV2 felt like a safer baseline for a bugfix-only PR.
SieveSpace, parser rework, UI fixes, bootstrap update - unrelated to the socket issue.

Testing: I tested it in TB 128 -- the extension loads, connects to a sieve server, StartTLS works, reload is clean. Unit tests pass.

Re: "AI slop" -- I used Claude Code as a pair programmer to help research the TB API history and wire up the changes. The bug fixes above came out of actually running it and hitting errors.

Also opened #1144 (hostname lookup: realHostName → hostName for TB 102+) and #1146 (AppImage tool download -- AppImageKit release 13 renamed assets with an obsolete- prefix). Those are independent of this PR.

@thsmi
Copy link
Copy Markdown
Owner

thsmi commented Apr 21, 2026

Still don't get what the intention behind this pull request should be? It just duplicates existing fixes which are already present in a different branch but it completely ignores any of the real blockers.

The really really big blocker is that the graphical editor does not work reliably anymore in Thunderbird. The way drag an drop works changed slightly. Firefox adjusted his behavior to chrome and chrome to Firefox and thus events fire slightly different. Which results in elements being dropped at the wrong spot in Thunderbird. Thus the UI and the drag an drop editor can get out of sync. Which is the worst thing to happen for such an editor. Because what you see on the screen no more aligns with the script. Which is a pretty serious bug. And why I started a major parser rewrite.

Additionally codemirror 5 is replaced by codemirror 6 and as the author writes nicely support is "slowly fading" and suffers from the same subtle bug due to bit-rot.

And then there is the manifest V2 vs V3 dilemma. The Thunderbird devs try to maintain V2 compatibility as long as possible but if upstream breaks this addon will be basically dead. It needs a background service and experiments to work.
The only way to keep it alive is to upstream the TCP-API experiment to Thunderbird. So that Thunderbird natively exposes TCP/IP sockets to extensions. They generally agreed to accept such a contribution but they clearly stated they can't implement it by their own. And the UI needs to be switches to places because, the way menus are added it completely incompatible with V3 this also relies upon an experiment. Here only places is an option because upstreaming the menu experiment is no wanted. They don't want that addons can add arbitrary menu items.

@szotsaki
Copy link
Copy Markdown

Btw. the answer to your question was also generated by AI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Does not work with TB 115+ Broken with TB 128 again :(

3 participants