Skip to content

[ffigen] Stop using NSProxy #2029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 27, 2025
Merged

[ffigen] Stop using NSProxy #2029

merged 10 commits into from
Feb 27, 2025

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Feb 26, 2025

Remove all use of NSProxy from our protocol implementation infrastructure, because it's brittle. Instead, each ObjCProtocolBuilder creates a new class at runtime (objc_allocateClassPair), and adds the implemented methods to it (class_addMethod`). From the user's perspective, this change shouldn't make a difference.

The plumbing remains broadly similar to the old NSProxy approach (DOBJCDartProtocolBuilder and DOBJCDartProtocol are mostly the same as DOBJCDartProxyBuilder and DOBJCDartProxy). We still need the underlying object to hold a map from method names to blocks (explanation). class_addMethod takes an ordinary C function pointer for the method implementation, so we have a trampoline (for each method signature) which looks up the block in the map and invokes the block. Eg, a method with signature BOOL(id) has this trampoline:

typedef BOOL  (^_ProtocolTrampoline6)(void * sel, id arg1);
__attribute__((visibility("default"))) __attribute__((used))
BOOL  _ObjectiveCBindings_protocolTrampoline_3su7tt(id target, void * sel, id arg1) {
  // Essentially: [target getDOBJCDartProtocolMethodForSelector: sel](sel, arg1);
  return ((_ProtocolTrampoline6)((id (*)(id, SEL, SEL))objc_msgSend)(
      target, @selector(getDOBJCDartProtocolMethodForSelector:), sel))(
          sel, arg1);
}

One very minor breaking change is that we need to call objc_registerClassPair before using the new class, and after that we can't add more methods. So it's no longer possible to call ObjCProtocolBuilder.build(), then add more methods, then call ObjCProtocolBuilder.build() again.

Fixes #2027

Copy link

github-actions bot commented Feb 26, 2025

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
objective_c Breaking 6.0.0 7.0.0-wip 7.0.0 ✔️
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ⚠️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
objective_c _Version

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@coveralls
Copy link

coveralls commented Feb 26, 2025

Coverage Status

coverage: 88.032% (+0.5%) from 87.507%
when pulling 63b4b1b on no_proxy
into 2965c48 on main.

@liamappelbe liamappelbe marked this pull request as ready for review February 26, 2025 04:14
$buildImplementations
return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
}

/// Adds the implementation of the $originalName protocol to an existing
/// [$protocolBuilder].
///
/// Note: You cannot call this method after you have called `builder.build`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a boolean and throw in case someone tries this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what ObjCProtocolBuilder._built does.

final argRecv = argsReceived.join(', ');
final argPass = argsPassed.join(', ');
final fnName = protocolTrampoline!.func.name;
final blk = w.objCLevelUniqueNamer.makeUnique('_ProtocolTrampoline');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is not immediately obvious (to me) that this is "block"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@liamappelbe liamappelbe merged commit 31dcb84 into main Feb 27, 2025
22 checks passed
@liamappelbe liamappelbe deleted the no_proxy branch February 27, 2025 00:48
ObjCBlockBase,
getClass,
msgSendPointer,
registerName;
import 'objective_c_bindings_generated.dart' as objc;
import 'selector.dart';

/// Helper class for building Objective C objects that implement protocols.
class ObjCProtocolBuilder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned that this now just leaks classes, which it creates. The way things written now every time you create a new CupertinoClient you will build a new class to represent a custom protocol implementation and then effectively leak it. I think it is dangerous and can lead to hard to diagnose leaks.

I think we need to find a way to fix this.

PS. I also think we should try to find a way to streamline the whole setup with blocks and copying dictionaries which is being used to implement this. It seems rather complicated and high overhead with many layers between things - I do understand that some amount of trampolining is required to compensate for differences in calling conventions, but the current setup seems excessively complex.

I also think we should again look back at approaches we could take to map Objective-C protocols to proper Dart classes and move away from the setup with building the implementations in this very imperative fashion. I started sketching something and will get once I have something concrete.

Copy link
Contributor Author

@liamappelbe liamappelbe Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can fix the leak by deleting the class using objc_disposeClassPair. Multiple objects can share the class, so I'll have DOBJCDartProtocol hold a reference to a DOBJCClass object which holds a pointer to the class and deletes it in its dealloc method.

streamline the whole setup with blocks

Using blocks lets us reuse all the block infra. If I didn't use blocks I'd need to duplicate a lot of logic, and I'd only save one direct C function invocation. All the other trampolining and map lookups would still be needed.

and copying dictionaries

Just realised the dictionary copy isn't needed anymore. I needed it before this PR because it was possible to add methods to a builder, build an instance, then add more methods. But that's not allowed anymore, so I can directly use the builder's dictionary in all the created objects.

Filed #2042

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple objects can share the class, so I'll have DOBJCDartProtocol hold a reference to a DOBJCClass object which holds a pointer to the class and deletes it in its dealloc method.

I would make both builder and classes hold the reference to it and have a native finalizer attached to the builder for good measure. (BTW feel free to send PR for review to me when you do it).

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

Successfully merging this pull request may close these issues.

[ffigen] Investigate dart-lang/http#1702
4 participants