Don't retain the MTLTexture or MTLDevice in TestMetalContext

[gw280]

This fixes a memory leak where we weren't ever releasing the MTLTexture, MTLDevice or MTLTextureDescriptor in TestMetalContext.

Basically, in CreateMetalTexture we manually retain the MTLTexture and MTLTextureDescriptor when we create them, but this is unnecessary as we automatically take ownership of both of these. The MTLTextureDescriptor is automatically released at the end of the block by scoped_nsobject, but the MTLTexture needs to be released in the destructor.

Similarly in the TestMetalContext constructor, we manually retain the device returned by MTLCreateSystemDefaultDevice(), but as we are expected to have ownership of it this is unnecessary. The fml::scoped_nsprotocol/scoped_nsobject smart pointers adopt the ownership without calling retain, and release when going out of scope at the end of the constructor, which is fine because we assign them to the private members before then, with an additional retain call.

The additional retain calls at the end of the constructor are fine as they are balanced out by the release calls in the destructor.

This is poorly documented in the Apple docs, but I believe the "Create" rule described at https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-103029 should cover it.

[flutter-dashboard]

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.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

[gaaclarke]

This isn't necessary, those textures are stored in sk_cfp which calls release in its deconstructor. If you see this being necessary you probably have a double retain. It looks correct when you are making it to me, the apis are a bit confusing though.

[gw280]

Ah right - I missed that we were calling the sk_cfp copy ctor when assigning the MTLTexture into the textures_ map (and that the textures_ map holds the sk_cfp and not a raw pointer). This means that we don't need to release manually in this dtor, but I believe we still need to remove the extra retain call, right?

[gaaclarke]

Nope, I believe we should have the retain now. [device newTextureWithDescriptor:texture_descriptor.get()] should return an object that is on the autorelease pool. Which means it will get released when this event is done, so we'd need a retain to keep it alive in the smart pointer.

I'm not 100% certain because the semantics for an objc method called new* that return an objc object are a bit unclear to me. I think technically anything that isn't named create should be on the autorelease pool, but that isn't the case for -[NSObject new].

[gw280]

Unless I'm misunderstanding the guide at https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html, it says that any ObjC method with "new" in the name returns an object that the caller owns outright. That implies to me that it won't be in the autorelease pool.

That being said, I think that the MTLTextureDescriptor does fall under that category perhaps, as the ObjC method name does not include the word "new", "create" etc, so I'd expect that to be in the autorelease pool?

[gaaclarke]

Ah, yep, you're right, anything that begins with "new" means the caller owns it. No need to retain it again. You don't run into new methods often.

[gw280]

OK, the unit test that was failing was because the MTLTextureDescriptor is indeed returned already in the autorelease pool, so that retain still needs to be there.

With that fixed, all the unit tests are now passing locally.

[gaaclarke]

LGTM, thanks!