liamappelbe
commented
2 weeks ago Looks like I used the wrong base branch. Commits e4a844e and later are real.
Closed liamappelbe closed 1 week ago
liamappelbe
commented
2 weeks ago Looks like I used the wrong base branch. Commits e4a844e and later are real.
github-actions[bot]
commented
2 weeks ago 
coveralls
commented
2 weeks ago coverage: 91.246% (+0.6%) from 90.676% when pulling 49de3a4a306cf24d62631788f93b8a17ec52383e on memerror into 6a9282c7e2abd5d5ad89a2de1eb985ee9fc63262 on main.
liamappelbe
commented
2 weeks ago Looks like the object ref counting test sometimes crashes (2/7 times I've run it). The pointer passed to object_getClass doesn't need to be a valid object, but it does need to be readable. I guess the memory page that the object was sitting in was unmapped right after the object was deleted.
I can work around this for the test by using one of the Mac virtual memory functions to test if the pointer is readable. These return an error code on failure rather than crashing. I'll do this for both the object and block tests.
I could potentially do this virtual memory test in the assert itself, but I feel like that would be overkill. I've never seen this crash on my local machine, so I think it's fairly rare. And unlike the test, if an unreadable pointer is passed to the assert, we were about to crash anyway, and the assert is just to give a best effort stack trace.
dcharkes
commented
2 weeks ago I could potentially do this virtual memory test in the assert itself, but I feel like that would be overkill. I've never seen this crash on my local machine, so I think it's fairly rare. And unlike the test, if an unreadable pointer is passed to the assert, we were about to crash anyway, and the assert is just to give a best effort stack trace.
Hm, maybe we should do it in asserts. There could be a chance that it's more common on a different type of machine. We don't pay any costs for asserts in release mode, so I'm not worried about the speed.
We should probably also make some clear documentation on the FFIgen and ObjectiveC package readme's that if there's segfaults the first thing users should do is run with asserts enabled.
liamappelbe
commented
2 weeks ago Hm, maybe we should do it in asserts. There could be a chance that it's more common on a different type of machine. We don't pay any costs for asserts in release mode, so I'm not worried about the speed.
Done.
Actually, it looks like that check is too strict to use in general code. ObjC sometimes uses "tagged pointers" (this means something different than in Dart, so I can't just mask off the tag).
We should probably also make some clear documentation on the FFIgen and ObjectiveC package readme's that if there's segfaults the first thing users should do is run with asserts enabled.
Yeah, I need to write a readme for package:objective_c. At the moment it just has a placeholder readme. When I do I'll include a note about this.
When retaining or releasing a reference to an ObjC object or block, assert that the pointer is valid. The goal is to catch memory management errors (most importantly, retaining a ref to a dead object) in Dart where we can get a nice stack trace. Without this change, the retain/release call will seg fault in ObjC land, which gives no useful debugging information.
The key functions are
isValidBlock(in objective_c.c) andisValidObject(in internal.dart).isValidBlockis based on thegetBlockRetainCountfunction from the ffigen tests. Blocks have a field namedisa, which describes what kind of block it is. I tracked down all possible values of this field (there are 6), and so I can just check that the value is one of these. When a block is destroyed the isa field is cleared, so the odds of falsely declaring a block as valid are pretty low.isValidObjectis similar. Objects have an 8 byte header, part of which is a pointer to a class object. Theobject_getClassfunction extracts that pointer, and then we check if it's valid by looking it up in a set that contains every valid class object (there are typically a few thousand). We construct this set usingobjc_copyClassList. Like blocks, this header is cleared when the object is destroyed, so false positives are unlikely. See https://github.com/dart-lang/native/issues/1038 for more info.Fixes https://github.com/dart-lang/native/issues/1038