Skip to content
This repository was archived by the owner on Jan 28, 2024. It is now read-only.

Reuse Blocks in ObjCBlock.fromFunction #477

Closed
wants to merge 5 commits into from
Closed

Reuse Blocks in ObjCBlock.fromFunction #477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a985fef
Zero init unused Block fields
liamappelbe Oct 14, 2022
95e5487
fmt
liamappelbe Oct 14, 2022
5519737
Fix bug
liamappelbe Oct 14, 2022
e671dbd
Reuse function Blocks to reduce memory leak
liamappelbe Oct 19, 2022
1f20c46
Merge branch 'zeroinit' into reuse_block
liamappelbe Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions lib/src/code_generator/objc_block.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ $voidPtr $registerClosure(Function fn) {
final exceptionalReturn = defaultValue == null ? '' : ', $defaultValue';
s.write('''
class $name extends _ObjCBlockBase {
$name._(${blockPtr.getCType(w)} id, ${w.className} lib) :
super._(id, lib, retain: false, release: true);
$name._(${blockPtr.getCType(w)} id, ${w.className} lib, {
bool retain = false}) : super._(id, lib, retain: retain, release: true);

/// Creates a block from a C function pointer.
$name.fromFunctionPointer(${w.className} lib, $natFnPtr ptr) :
Expand All @@ -115,12 +115,20 @@ class $name extends _ObjCBlockBase {
static $voidPtr? _cFuncTrampoline;

/// Creates a block from a Dart function.
$name.fromFunction(${w.className} lib, ${funcType.getDartType(w)} fn) :
this._(lib.${builtInFunctions.newBlock.name}(
static $name fromFunction(${w.className} lib, ${funcType.getDartType(w)} fn) {
final oldBlock = _funcToBlock[fn];
if (oldBlock != null) {
return $name._(oldBlock, lib, retain: true);
}
final newBlock = lib.${builtInFunctions.newBlock.name}(
_dartFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
${trampFuncType.getCType(w)}>($closureTrampoline
$exceptionalReturn).cast(), $registerClosure(fn)), lib);
$exceptionalReturn).cast(), $registerClosure(fn));
_funcToBlock[fn] = newBlock;
return $name._(newBlock, lib);
Copy link

Choose a reason for hiding this comment

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

We don't increase refcount for newBlock here.

What happens if the underlying obj-c block is de-rereferenced and freed while still being in the expando map? If one creates another block for same closure, we have a use-after-free, don't we?

I'll comment more on the github issue

Copy link

Choose a reason for hiding this comment

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

I guess the assumption is that the dart wrapper will keep a refcount as long as it's alive?

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, the Dart wrapper always holds one reference. In all the other cases where we construct the wrapper function, we're using a block pointer from Block_copy, which already has an incremented ref count.

}
static $voidPtr? _dartFuncTrampoline;
static final _funcToBlock = Expando<${blockPtr.getCType(w)}>('$name');
''');

// Call method.
Expand Down
35 changes: 34 additions & 1 deletion test/native_objc_test/block_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ void main() {
return block.pointer.cast();
}

test('Function pointer block ref counting', () {
test('Function block ref counting', () {
final rawBlock = funcBlockRefCountTest();
doGC();
expect(BlockTester.getBlockRetainCount_(lib, rawBlock.cast()), 0);
Expand All @@ -111,6 +111,39 @@ void main() {
expect(descPtr.ref.dispose_helper, nullptr);
expect(descPtr.ref.signature, nullptr);
});

test('Generating a block for the same function returns the same block', () {
final func = makeAdder(4000);
final block1 = ObjCBlock.fromFunction(lib, func);
final block2 = ObjCBlock.fromFunction(lib, func);
expect(block1.pointer, block2.pointer);

final block3 = ObjCBlock.fromFunction(lib, makeAdder(4000));
expect(block1.pointer, isNot(block3.pointer));
});

void reusedBlockRefCountTestInner(int Function(int) func) {
final block2 = ObjCBlock.fromFunction(lib, func);
expect(BlockTester.getBlockRetainCount_(lib, block2.pointer.cast()), 2);
}

Pointer<Void> reusedBlockRefCountTest() {
final func = makeAdder(4000);
final block1 = ObjCBlock.fromFunction(lib, func);
expect(BlockTester.getBlockRetainCount_(lib, block1.pointer.cast()), 1);

reusedBlockRefCountTestInner(func);
doGC();
expect(BlockTester.getBlockRetainCount_(lib, block1.pointer.cast()), 1);

return block1.pointer.cast();
}

test('Reused function blocks ref count correctly', () {
final rawBlock = reusedBlockRefCountTest();
doGC();
expect(BlockTester.getBlockRetainCount_(lib, rawBlock.cast()), 0);
});
});
}

Expand Down