@chinmaygarde is the outstanding concern that this may break the engine? Or that the tests might break? If if made you more comfortable I could make this a complete noop for the engine:

template <typename NST, typename MemoryManagement = scoped_nsprotocol_memory_management<NST>>
class scoped_nsprotocol {
 public:
  explicit scoped_nsprotocol(NST object = nil) : object_(object) {}

  scoped_nsprotocol(const scoped_nsprotocol<NST>& that) : object_(MemoryManagement::retain(that.object_)) {}

  template <typename NSU>
  scoped_nsprotocol(const scoped_nsprotocol<NSU>& that) : object_(MemoryManagement::retain(that.get())) {}

  ~scoped_nsprotocol() { MemoryManagement::release(object_); }

  scoped_nsprotocol& operator=(const scoped_nsprotocol<NST>& that) {
    reset(MemoryManagement::retain(that.get()));
    return *this;
  }
  ...

For the engine code, this PR changes nothing other than adding an extra C function on top of memory management code (that I can eliminate if we want the extra complexity). For testing code, there is no such thing as hypothetical bugs because 100% of the testing code is tested.

This PR looks like what I remember our first attempt being. IIRC the issue here is something like: because it doesn't have the proper annotations, these functions behave differently when compiled in ARC vs non-ARC translation units. That means that if two TUs, one with ARC and one without, both include this header (which is going to be the case), you have two different implementations of the same function, and that violates the ODR. ODR violations result in undefined behavior. I saw fun things like ARC TUs getting the non-ARC codepath, and the other way around.

For testing code, there is no such thing as hypothetical bugs because 100% of the testing code is tested.

There is such a thing as undefined behavior though, and you can't test your away around it because it's undefined.

Is there a reason you're not just taking the working implementation from Chromium?

Thanks Stuart for the response.

these functions behave differently when compiled in ARC vs non-ARC translation units.

These functions are in a .mm file that is compiled into libFlutter and only compiled with non-ARC so there is no opportunity for them to behave differently. Ignore for a second that I'm doing this PR to support my ARC test. If you just look at the change you see this is the same behavior as the previous header when it is only included in non-ARC code.

ODR violations result in undefined behavior. I saw fun things like ARC TUs getting the non-ARC codepath, and the other way around.

We can see the non-ARC code will compile the same. With respect to ARC code, the only ARC code is test code which means we have 100% test coverage. If the tests pass there is no bug.

For testing code, there is no such thing as hypothetical bugs because 100% of the testing code is tested.

There is such a thing as undefined behavior though, and you can't test your away around it because it's undefined.

Undefined behavior isn't a problem for tests since everything is asserted to work a certain way. It is only a problem if the undefined behavior acts inconsistently across the same compiler and platform. There is no real way for me to prove that a compiler doesn't have undefined behavior, it is a risk we live with with all compiled code. I shouldn't have to prove 100% of all possible code creates expected results, especially when dealing with a situation where 100% of actual usage is known.

Is there a reason you're not just taking the working implementation from Chromium?

I noticed after the fact that they just threw an #error for ARC so it won't work.

Here's our options:

1. The current proposition. The benefit being it addresses the issue across the board. There is a non-proven belief that it may have undefined behaviors, but they are limited to testing code.
2. I could refactor AccessibilityBridge to use pimpl. I don't have to edit scoped_nsobject but it is also a game of whack-a-mole where we have to transition things to pimpl as they get included in headers. There is also a slight performance cost to production code.
3. I could limit this change to opt in TU's via a FLUTTER_ENABLE_SCOPED_NSOBJECT_ARC macro. If the concern that someone may use this header in production code this would reduce that risk.
4. I could rewrite the test to use manual reference counting. This would leave us with a confusing test bed where some tests are ARC, some are not. If we transitioned everything to non-ARC we would not be testing our code how the majority of people actually use our code. Also, I've had problems with OCMock in the past with non-ARC code (I don't miss the irony). Non-ARC code is a 3rd class citizen (behind Swift and ARC Objc) so it's easy to see why it isn't as tested for libraries like OCMock. We should be using it the way most people are using it when reasonable to do.

edit: cc @stuartmorgan

Undefined behavior isn't a problem for tests since everything is asserted to work a certain way. It is only a problem if the undefined behavior acts inconsistently across the same compiler and platform.

I don't see any reason to believe that the UB from an ODR violation wouldn't cause hard-to-find flaky test issues.

There is no real way for me to prove that a compiler doesn't have undefined behavior, it is a risk we live with with all compiled code. I shouldn't have to prove 100% of all possible code creates expected results, especially when dealing with a situation where 100% of actual usage is known.

There is a huge difference between "theoretically, all code could potentially have undefined behavior" and "this code is forked from code where I and some other people tried to solve exactly the problem you are trying to solve (i.e., transitive header includes of scoped_nsobject usage) and my recollection is that the approach you are trying is the one that gave us UB".

Is there a reason you're not just taking the working implementation from Chromium?

I noticed after the fact that they just threw an #error for ARC so it won't work.

That's a recent change, which apparently happened after they completed a full ARC transition and thus no longer needed to solve this problem. https://codereview.chromium.org/1855483004 is where they first made it work.

Here's our options:

1. The current proposition. The benefit being it addresses the issue across the board. There is a non-proven belief that it may have undefined behaviors, but they are limited to testing code.
2. I could refactor AccessibilityBridge to use pimpl. I don't have to edit scoped_nsobject but it is also a game of whack-a-mole where we have to transition things to pimpl as they get included in headers. There is also a slight performance cost to production code.
3. I could limit this change to opt in TU's via a FLUTTER_ENABLE_SCOPED_NSOBJECT_ARC macro. If the concern that someone may use this header in production code this would reduce that risk.
4. I could rewrite the test to use manual reference counting. This would leave us with a confusing test bed where some tests are ARC, some are not. If we transitioned everything to non-ARC we would not be testing our code how the majority of people actually use our code. Also, I've had problems with OCMock in the past with non-ARC code (I don't miss the irony). Non-ARC code is a 3rd class citizen (behind Swift and ARC Objc) so it's easy to see why it isn't as tested for libraries like OCMock. We should be using it the way most people are using it when reasonable to do.

5. We could update our copy of scoped_nsobject—which came from Chromium in the first place—to a version where Chromium engineers already did the work of making it work correctly for exactly this use case.

Which like 1 address this across the board, but unlike 1 has been used extensively in production code, thus we have a high degree of confidence that it doesn't have ODR violations.

@stuartmorgan thanks for the link.

I read through their implementation, there is 2 things they are doing different than this PR. They are using extra annotations __unsafe_unretained and ns_returns_not_retained and they are only using the those functions when __has_feature(objc_arc).

I can make those 2 changes, or I can bring their code over. Their code is a bit messy and spread out across multiple files. I'd prefer to make those 2 changes but if you feel strongly about it I can try to bring over their files verbatim.

@stuartmorgan There is a lot of baggage bringing over chromes code verbatim. I moved over the difference I saw, please review an let me know if I missed something.

There is one more thing chrome does that we don't, InvalidValue() One sec I'll add that.

Added InvalidValue from chrome. That seemed unnecessary but worth keeping it the same.

I talked to @stuartmorgan offline. The criteria for which he'd feel comfortable making any change are undefined since a risk is believed to exist but he has no way to prove a change presents a problem or not because the problem is not fully understood. Anything short of a verbatim copy from chromium code and its dependencies presents an unacceptable risk.

The one thing he is sure about is that the definitions should be the same across TU's so we should avoid ifdefs and the one thing I'm sure about is that we should have the annotations like __unsafe_unretained. That's where we are now.

Given what has been discussed I think there should be a unit test compiled and run with ARC and one for manual reference counting to make sure we get the annotations correct. That may be moot if stuart doesn't feel comfortable even with that assurance.

The criteria for which he'd feel comfortable making any change are undefined since a risk is believed to exist but he has no way to prove a change presents a problem or not because the problem is not fully understood. Anything short of a verbatim copy from chromium code and its dependencies presents an unacceptable risk.

This is not an accurate summary of my position.

Given that after several rounds of discussion we still don't seem to have reached a common understanding of the situation, I think the best option is to continue with another reviewer.

@stuartmorgan Ok, since this isn't a priority right now I'll drop this back to a draft, implement the unit tests that run on ARC and non-ARC, maintain the policy of no-ifdefs to avoid your concern about ODR violations and solicit the review from an elder statesman.

In general the ARC and mem management LGTM.

Well, that's what Rohit and I thought about the first broken version :)

Addressing a comment from earlier:

I think there should be a unit test compiled and run with ARC and one for manual reference counting to make sure we get the annotations correct.

Since that's how we figured out that the version we wrote had UB in the first place, I agree that this should be a requirement for landing any version of this change.

In general the ARC and mem management LGTM.

Well, that's what Rohit and I thought about the first broken version :)

I didn't actually see all of this background discussion until after I posted my comment. I was busy editing my previous post and then saw that you had already replied.

Addressing a comment from earlier:

I think there should be a unit test compiled and run with ARC and one for manual reference counting to make sure we get the annotations correct.

Since that's how we figured out that the version we wrote had UB in the first place, I agree that this should be a requirement for landing any version of this change.

Seems reasonable. As you mention it does not guarantee that ODR/UB problems don't exist, but as Aaron points out it gives us as good a bar as any of our other code, that is we wrote a test that executed the code paths that we were concerned about in the environments that we were concerned about, and it passed.

I'll be interested to see if Mark has any other thoughts.

I had @bdash look into this. He said there may be concerns with an ODR violation. He suspects that we might be able to get to a point where the annotations cause the compilation of this code with ARC and without ARC to be identical. If we could get to that point and show that they were identical there would be no possible ODR violation since whatever version of the machine code the loader grabbed, they would be identical. That gives this PR an avenue forward, I'll give it a look.

He also mentioned they use CFType in the past to work around this in webkit, but wasn't keen to recommend it as a first course of action. That worked since it was a way to get ARC to ignore your code and compile the same between ARC and MRC.

To be explicit, the ODR concern is that when compiled under ARC vs MRC, the compiler will generate different machine code for methods that deal directly with the Obj-C pointer type (either id or NST in this templated code) unless special care is taken. In particular, assignments to a local variable or member variable of that type may bring with them retain count operations in ARC. Returning a value of that type from a function may also result in retain / autorelease operations.

It's up to the linker which of the many definitions of an inline function is selected, and there's no guarantee it'll select the ARC vs MRC version for any given member function, which may lead to leaks or over-releases if they end up being mixed.

The approach I suggested was to ensure that every use of NST (or id) as a bare pointer type declaring a member, parameter, or local variable was annotated with __unsafe_unretained, and that any method with a return type of NST (or id) be annotated as ns_returns_not_retained. This should be sufficient to prevent the compiler from emitting any retain / release / autorelease operations under ARC, and should result in equivalent machine code for code in ARC / MRC translation units that use this type. I believe this is the approach that Chromium's equivalent type used, prior to the removal of its support for ARC.

Ok, I took a look. As the code stands today here is what the disassembly looks like for arc/mrc for the scoped_nsobject constructor:

main_arc`fml::scoped_nsobject<Foobar>::scoped_nsobject:
    0x100003c60 <+0>:   pushq  %rbp
    0x100003c61 <+1>:   movq   %rsp, %rbp
    0x100003c64 <+4>:   subq   $0x20, %rsp
    0x100003c68 <+8>:   movq   %rdi, -0x8(%rbp)
    0x100003c6c <+12>:  movq   $0x0, -0x10(%rbp)
    0x100003c74 <+20>:  leaq   -0x10(%rbp), %rdi
    0x100003c78 <+24>:  callq  0x100003ed0               ; symbol stub for: objc_storeStrong
    0x100003c7d <+29>:  movq   -0x8(%rbp), %rdi
->  0x100003c81 <+33>:  movq   -0x10(%rbp), %rsi
    0x100003c85 <+37>:  callq  0x100003d80               ; fml::scoped_nsobject<NSArray>::scoped_nsobject at scoped_nsobject.h:132
    0x100003c8a <+42>:  jmp    0x100003c8f               ; <+47> at scoped_nsobject.h
    0x100003c8f <+47>:  xorl   %eax, %eax
    0x100003c91 <+49>:  movl   %eax, %esi
    0x100003c93 <+51>:  leaq   -0x10(%rbp), %rcx
    0x100003c97 <+55>:  movq   %rcx, %rdi
    0x100003c9a <+58>:  callq  0x100003ed0               ; symbol stub for: objc_storeStrong
    0x100003c9f <+63>:  addq   $0x20, %rsp
    0x100003ca3 <+67>:  popq   %rbp
    0x100003ca4 <+68>:  retq   
    0x100003ca5 <+69>:  xorl   %ecx, %ecx
    0x100003ca7 <+71>:  movl   %ecx, %esi
    0x100003ca9 <+73>:  movq   %rax, -0x18(%rbp)
    0x100003cad <+77>:  movl   %edx, -0x1c(%rbp)
    0x100003cb0 <+80>:  leaq   -0x10(%rbp), %rax
    0x100003cb4 <+84>:  movq   %rax, %rdi
    0x100003cb7 <+87>:  callq  0x100003ed0               ; symbol stub for: objc_storeStrong
    0x100003cbc <+92>:  movq   -0x18(%rbp), %rdi
    0x100003cc0 <+96>:  callq  0x100003eac               ; symbol stub for: _Unwind_Resume
    0x100003cc5 <+101>: ud2    
    0x100003cc7 <+103>: nopw   (%rax,%rax)

main_mrc`fml::scoped_nsobject<Foobar>::scoped_nsobject:
    0x100003de0 <+0>:  pushq  %rbp
    0x100003de1 <+1>:  movq   %rsp, %rbp
    0x100003de4 <+4>:  subq   $0x10, %rsp
    0x100003de8 <+8>:  movq   %rdi, -0x8(%rbp)
    0x100003dec <+12>: movq   %rsi, -0x10(%rbp)
    0x100003df0 <+16>: movq   -0x8(%rbp), %rdi
->  0x100003df4 <+20>: movq   -0x10(%rbp), %rsi
    0x100003df8 <+24>: callq  0x100003e90               ; fml::scoped_nsobject<NSArray>::scoped_nsobject at scoped_nsobject.h:132
    0x100003dfd <+29>: addq   $0x10, %rsp
    0x100003e01 <+33>: popq   %rbp
    0x100003e02 <+34>: retq   
    0x100003e03 <+35>: nopw   %cs:(%rax,%rax)
    0x100003e0d <+45>: nopl   (%rax)

I added the following patch:

-  explicit scoped_nsobject(NST* object = Memory::InvalidValue())
+  explicit scoped_nsobject(__unsafe_unretained NST* object = Memory::InvalidValue())
       : scoped_nsprotocol<NST*>(object) {}

The disassembly for the arc version of the constructor after this change is identical. I'll ponder this a bit more. I think just making scoped_nsobject just a wrapper around scoped_nsobject could avoid this problem.

If anyone else wants to play around with it I posted my testbed: https://github.com/gaaclarke/scoped_nsobject_test

Notice that the symbols for instantiations of scoped_nsprotocol can't collide between arc and mrc because the fact it is an arc covered class is codified in the type (notice Foobar* __strong):

aaclarke-macbookpro2:scoped_nsobject_test aaclarke$ nm main_arc | c++filt 
0000000100003b50 unsigned short -[Foobar dealloc]
0000000100003efc short GCC_except_table1
0000000100003f0c short GCC_except_table2
0000000100003f1c short GCC_except_table5
0000000100003f30 short GCC_except_table7
0000000100008108 S _OBJC_CLASS_$_Foobar
                 U _OBJC_CLASS_$_NSObject
0000000100008130 S _OBJC_METACLASS_$_Foobar
                 U _OBJC_METACLASS_$_NSObject
0000000100008088 short __OBJC_$_INSTANCE_METHODS_Foobar
00000001000080a8 short __OBJC_CLASS_RO_$_Foobar
0000000100008040 short __OBJC_METACLASS_RO_$_Foobar
                 U __Unwind_Resume
0000000100008160 bool s_didDealloc
0000000100003c20 unsigned short fml::scoped_nsobject<Foobar>::scoped_nsobject(Foobar*)
0000000100003d40 unsigned short fml::scoped_nsobject<Foobar>::scoped_nsobject(Foobar*)
0000000100003c90 unsigned short fml::scoped_nsobject<Foobar>::~scoped_nsobject()
0000000100003cb0 unsigned short fml::scoped_nsobject<Foobar>::~scoped_nsobject()
0000000100003db0 unsigned short fml::scoped_nsprotocol<Foobar* __strong>::scoped_nsprotocol(Foobar*)
0000000100003cd0 unsigned short fml::scoped_nsprotocol<Foobar* __strong>::~scoped_nsprotocol()
0000000100003e30 T fml::scoped_nsprotocol_arc_memory_management::Autorelease(objc_object*)
0000000100003e10 T fml::scoped_nsprotocol_arc_memory_management::Retain(objc_object*)
0000000100003e50 T fml::scoped_nsprotocol_arc_memory_management::Release(objc_object*)
                 U std::terminate()
0000000100003d30 unsigned short ___clang_call_terminate
                 U ___cxa_begin_catch
                 U ___gxx_personality_v0
0000000100008158 double __dyld_private
0000000100000000 T __mh_execute_header
                 U __objc_empty_cache
0000000100003b90 T _main
                 U _objc_alloc_init
                 U _objc_autorelease
                 U _objc_msgSendSuper2
                 U _objc_release
                 U _objc_retain
                 U _objc_storeStrong
                 U _printf
                 U dyld_stub_binder

That means that there can't be an ODR violation for scoped_nsprotocol because they map to 2 different symbols depending on if a type is covered by arc or not.

The problem is that the way scoped_nsobject is implemented it scrapes off that type information here:

class scoped_nsobject : public scoped_nsprotocol<NST*>

If we can find a way to preserve that type information we wouldn't have a problem. That might mean having users say fml::scoped_nsobject<Foobar*> instead of fml::scoped_nsobject<Foobar>.

Ugh. From the generated code it looks like Clang is allocating a stack slot to hold the object, zeroing it, then using objc_storeStrong to assign the object parameter to it, before loading the value from that stack slot and passing it to the base class constructor. It then uses objc_storeStrong to clear the stack slot. This all has the effect of retaining / releasing the passed-in object for the duration of the constructor call. I'm a little surprised that it's doing that considering both the value being passed in and the destination parameter are __unsafe_unretained.

You're right that if the pointer type, rather than the class type, is used as the template parameter then you get different mangling which may be sufficient to avoid ODR problems. Relying on that would require changing the way these smart pointers are used, but it seems like it may be a safer approach than trying to coax Clang into generating exactly what you're after.

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Ok, I pushed a commit where switched scoped_nsobject so that it doesn't truncate the ARC specific types, this will make the symbols for the same "type" be different symbols so there isn't a chance for ODR violation (example Foobar* vs Foobar* __strong). Thanks @bdash for your help. @stuartmorgan please give it a look and let me know if you have any concerns.

I'm closing this PR. I think the best approach would be to delete scoped_nsobjects, which is a better alternative than the more complicated task of migrating them to the pimpl pattern and enforcing they aren't used in headers.