iOS: Migrate accessibility_bridge to ARC

[cbracken]

Keeps fml::scoped_* wrappers used in C++ class declarations in headers, since those are injected into both ARC and non-ARC translation units and these classes are ARC-safe. Once we've migrated all users of those headers to ARC, we can use the underlying classes directly.

No semantic changes, therefore not changes to tests.

Issue: flutter/flutter#137801

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[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, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[matanlurey]

test-exempt: Semantically identical.

[hellohuanlin]

This was odd practice here before. Did we add this line to fix a crash before? If so, this line didn't 100% fix that crash since it's still used in the previous 3 lines.

[cbracken]

Ah, yes! I noticed this line too and was going to leave a comment here referencing the commit here: #31351

This bumps the refcount to ensure that oldObject isn't released before it's used again below, if the refcount is decremented in this method itself. It's a relatively common pattern in old non-ARC tree-related Obj-C code.

I didn't mentally work through the gymnastics of how oldObject's refcount could be indirectly knocked down one in this method, but we are manipulating the children of our parent node here, which seems like the exact sort of case where that can happen.

This is the sort of case that motivated the creation of ARC -- and IIRC a very old WWDC presentation where ARC was introduced covered exactly this scenario (as well as the more basic cases of incorrect setter implementations that don't check for assignment of the same value already held by the ivar.

[cbracken]

Let's try mentally working through it.

1. We get the position the list of oldObject within its parent's children into positionInChildlist.
2. We remove oldObject from objects, which releases oldObject. If that was the only thing that had retained it, then oldObject is dead and is dealloc'ed.
3. We call oldObject.parent ... on the next line. Boom.

To fix this, we need to ensure that we retain it for the duration we need it. We could have retained it, then manually released it after the call to oldObject.parent, but autorelease is much more future-proof.

[cbracken]

Ah in fact there's a comment that @stuartmorgan left pointing out this exact scenario if you scroll down far enough on the patch.

[cbracken]

It's 11 years old, but I think this is the talk I was thinking of that covers a simpler version of this scenario, though I swear there was a talk where they went through more complex scenarios like trees.

https://www.youtube.com/watch?v=ANlpDUIF-1o#t=9m19s

Edit: might have been this one around 48 minutes or so:
https://devstreaming-cdn.apple.com/videos/wwdc/2011/323/ipad_c.m3u8

[cbracken]

I think the TL;DR here is that this line is an excellent argument in favour of ARC migration.

[stuartmorgan-g]

This is the sort of case that motivated the creation of ARC

Yes, I think it's a great sign of how successful ARC was that there are now a lot of people whose reaction to this line is "that's weird" rather than immediately knowing exactly why it's there and what it's doing. Everyone learning this pattern (usually the hard way) is definitely something we can do without 🙂

I think the TL;DR here is that this line is an excellent argument in favour of ARC migration.

Both the line and the discussion! The line because not having this kind of foot gun in the first place is clearly a safety win, but also the discussion because it illustrates that being able to correctly maintain non-ARC code is a shrinking/fading knowledge domain, so any non-ARC code we have will be an increasing liability over time.