fix: Avoid keeping strong reference to self instance in delegates by mykola-mokhnach · Pull Request #1123 · appium/WebDriverAgent

mykola-mokhnach · 2026-04-09T15:37:54Z

This PR adds explicit MJPEG stop/run-state handling, breaks retention chains during shutdown (delegate/server/broadcaster cleanup), and improves load behavior by bounding MJPEG write timeouts and coalescing image-scaling work. It also updates route blocks to avoid strong self capture where applicable.

Result: cleaner teardown, lower risk of retained streaming objects, and more stable memory usage during long-running/slow-client streaming sessions.

Dan-Maor · 2026-04-09T17:09:59Z

Interesting.

I'll take a look at it and run some tests on Sunday once I'm back at work.

Copilot

Pull request overview

This PR aims to reduce retention/teardown issues in the MJPEG screenshot streaming pipeline and web server shutdown flow by adding explicit stop handling, weakening delegate references, bounding streaming write timeouts, and coalescing image scaling work.

Changes:

Add stopStreaming + run-state tracking to FBMjpegServer, and enforce bounded socket write timeouts with periodic stats logging.
Coalesce scaling work in FBImageProcessor so repeated submissions don’t schedule redundant scaling blocks.
Break potential retention chains during shutdown by weakening FBTCPSocket.delegate, clearing delegates on stop, and avoiding strong self capture in route blocks.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
WebDriverAgentLib/Utilities/FBMjpegServer.m	Adds streaming lifecycle flag, bounded write timeout, periodic stats, and explicit shutdown behavior.
WebDriverAgentLib/Utilities/FBMjpegServer.h	Exposes `stopStreaming` API for orderly shutdown.
WebDriverAgentLib/Utilities/FBImageProcessor.m	Coalesces/drains scaling work via a scheduled flag and a loop on the scaling queue.
WebDriverAgentLib/Routing/FBWebServer.m	Ensures broadcaster shutdown on dealloc/stop and avoids strong `self` capture in route blocks.
WebDriverAgentLib/Routing/FBTCPSocket.m	Makes delegate callbacks nil-safe and clears delegate on stop.
WebDriverAgentLib/Routing/FBTCPSocket.h	Changes delegate to `weak` (when supported) to prevent retention.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T05:06:21Z

  self.nextImage = image;
+  BOOL shouldSchedule = !self.isScalingScheduled;
+  if (shouldSchedule) {
+    self.isScalingScheduled = YES;
+  }
  [self.nextImageLock unlock];
+  if (!shouldSchedule) {
+    return;
+  }


submitImageData: now coalesces/schedules scaling work via isScalingScheduled and a draining while loop. Existing FBImageProcessorTests cover scaling correctness but don’t appear to exercise the new scheduling/coalescing behavior (e.g., multiple rapid submitImageData: calls resulting in expected number/order of completion callbacks). Adding a test for the new queueing/coalescing semantics would help prevent regressions.

KazuCocoa · 2026-04-10T05:32:39Z

I haven't dug into it deeply yet, but this branch didn't generate any images for 9100 when I opened it on my browser, while the current master did.

mykola-mokhnach · 2026-04-10T08:33:36Z

I haven't dug into it deeply yet, but this branch didn't generate any images for 9100 when I opened it on my browser, while the current master did.

yes, there was a bug related to weak referencing - should be ok now

KazuCocoa · 2026-04-10T15:55:40Z

(I'll test out again tonight)

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T16:00:34Z

+  id<FBTCPSocketDelegate> delegate = self.screenshotsBroadcaster.delegate;
+  if ([(NSObject *)delegate respondsToSelector:@selector(stopStreaming)]) {
+    [(FBMjpegServer *)delegate stopStreaming];


delegate is typed as id<FBTCPSocketDelegate>, but this code checks respondsToSelector: via an NSObject * cast and then force-casts to FBMjpegServer * to call stopStreaming. This is brittle (and will crash if a different delegate type is ever used). Prefer calling stopStreaming on the strongly-held self.mjpegServer, or narrow the type check/cast safely before invoking the method.

Suggested change

id<FBTCPSocketDelegate> delegate = self.screenshotsBroadcaster.delegate;

if ([(NSObject *)delegate respondsToSelector:@selector(stopStreaming)]) {

[(FBMjpegServer *)delegate stopStreaming];

if (nil != self.mjpegServer) {

[self.mjpegServer stopStreaming];

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-11T04:56:21Z

        const proxyTimeout = noSessionProxy.timeout;
-        noSessionProxy.timeout = 1000;
+        (noSessionProxy as any).timeout = 1000;
        try {
          currentStatus = (await noSessionProxy.command('/status', 'GET')) as StringRecord;
-          if (currentStatus && currentStatus.ios && (currentStatus.ios as any).ip) {
-            this.agentUrl = (currentStatus.ios as any).ip;
+          if (currentStatus?.ios?.ip) {
+            this.agentUrl = currentStatus.ios.ip as string | undefined;
          }
          this.log.debug(`WebDriverAgent information:`);
          this.log.debug(JSON.stringify(currentStatus, null, 2));
        } catch (err: any) {
          throw new Error(`Unable to connect to running WebDriverAgent: ${err.message}`);
        } finally {
-          noSessionProxy.timeout = proxyTimeout;
+          (noSessionProxy as any).timeout = proxyTimeout;
        }


The timeout mutation is now done via (noSessionProxy as any).timeout, which bypasses the type system and could mask a real API change (e.g., if timeout becomes a getter-only property, this assignment may throw at runtime or silently shadow the intended value). Prefer a typed approach here, such as creating a short-timeout NoSessionProxy instance just for the /status polling, or extending NoSessionProxy/JWProxy typings with a supported way to set the request timeout.

appium/appium#22154

Dan-Maor

Went through the code and tested on the devices I have available with Xcode 16.2 and 26.3.
Looks good 👍

## [11.4.2](v11.4.1...v11.4.2) (2026-04-12) ### Bug Fixes * Avoid keeping strong reference to self instance in delegates ([#1123](#1123)) ([dd15f48](dd15f48))

github-actions · 2026-04-12T08:01:45Z

🎉 This PR is included in version 11.4.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: Avoid keeping strong reference to self instance in delegates

2f20864

mykola-mokhnach requested review from Dan-Maor and KazuCocoa April 9, 2026 15:38

Fix compilation

720bfe2

KazuCocoa requested a review from Copilot April 10, 2026 05:01

Copilot started reviewing on behalf of KazuCocoa April 10, 2026 05:01 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

mykola-mokhnach added 3 commits April 10, 2026 10:30

address comments

6bf09b8

moar

830d551

typo

b6cfe5c

mykola-mokhnach added 2 commits April 10, 2026 10:38

fix build

e485a8b

fix build

e93099b

KazuCocoa requested a review from Copilot April 10, 2026 15:55

Copilot started reviewing on behalf of KazuCocoa April 10, 2026 15:56 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

Address comments

7ab55db

KazuCocoa approved these changes Apr 11, 2026

View reviewed changes

KazuCocoa requested a review from Copilot April 11, 2026 04:51

Copilot started reviewing on behalf of KazuCocoa April 11, 2026 04:51 View session

Copilot AI reviewed Apr 11, 2026

View reviewed changes

Address comments

3498781

mykola-mokhnach mentioned this pull request Apr 11, 2026

fix(base-driver): Make proxy timeout mutable again appium/appium#22154

Merged

Dan-Maor approved these changes Apr 12, 2026

View reviewed changes

mykola-mokhnach merged commit dd15f48 into master Apr 12, 2026
46 of 47 checks passed

mykola-mokhnach deleted the leak branch April 12, 2026 07:57

github-actions Bot pushed a commit that referenced this pull request Apr 12, 2026

chore(release): 11.4.2 [skip ci]

71e5659

## [11.4.2](v11.4.1...v11.4.2) (2026-04-12) ### Bug Fixes * Avoid keeping strong reference to self instance in delegates ([#1123](#1123)) ([dd15f48](dd15f48))

github-actions Bot added the released label Apr 12, 2026

Conversation

mykola-mokhnach commented Apr 9, 2026

Uh oh!

Dan-Maor commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KazuCocoa commented Apr 10, 2026

Uh oh!

mykola-mokhnach commented Apr 10, 2026

Uh oh!

KazuCocoa commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dan-Maor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants