Fix use of uninitialized memory when unsetting flex property

[jamesreggio]

This PR fixes a bug that can be reproduced on iOS 9.3 with v0.41.1 using this simple app. It's important to note that the bug pertains to uninitialized memory and is therefore not guaranteed to reproduce at all.

Description of the issue and fix

When a property on a node is set for the first time, we read and store the property's initial value before updating it. Later, if the property is nullified in JavaScript, we'll write the earlier-obtained initial value to the native representation.

In order to obtain the initial value, there must be a property getter that corresponds to the setter. If no getter exists, the slot to store the initial value will remain silently uninitialized. If the property is
nullified from JavaScript, we'll write the garbage value of that uninitialized variable into the native representation, and chaos will ensue.

This commit makes three changes to resolve this issue:

1. It adds an assertion that a getter exists every time a property is written. It would be better to enforce this statically, but I'm not aware of a means to accomplish this.

2. It adds a getter for the flex property, which previously lacked one. Prior to this change, setting then removing the flex property on a view would result in layout glitches and redboxes about NaN and Infinity in the bound and offset of a view.

3. It adds a getter for the on property for RCTSwitch, which can never be undefined, but is triggering the newly-added assertion.

There have been a number of complaints (#2616, #3406, to name a few) about transient redboxes with messages about illegal NaNs and Infinitys. Because this bug pertains to uninitialized memory which non-deterministic by nature, it's possible that a number of these difficult-to-repro issues will be resolved by this bugfix.

Test plan

Use the simple repro app to confirm that the issue has been resolved.

Concerns

I'm not in love with the idea of adding a dynamic assertion for asymmetrical (setter-only) properties. I would appreciate any suggestions of alternatives.

---

I discovered this issue when I was trying to figure out why react-native-foldview was repeatedly redboxing on iOS 9.3. It looks like other people have reported this, too. (cc: @jmurzy)

I believe that this regression pertains to commit 62177db by @nicklockwood (reviewed by @javache), so it would be great to have either of your eyes on it.

[javache]

Thanks for fixing this!

[facebook-github-bot]

@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[jamesreggio]

Thanks @javache.

I realized from the failing tests that my solution isn't quite acceptable. It requires that we discover all asymmetrical properties through the triggering of a runtime assertion, which isn't a sustainable approach. (By asymmetrical, I mean properties that do not follow the foo: + setFoo: convention.)

Some (but not all) of the boolean properties in the standard iOS libraries shirk the convention, opting for an isFoo: + setFoo: pairing. Furthermore, asymmetrical properties in third-party native modules can also trip this assertion.

At this point, I think it's useful to step back and revisit the challenge underpinning this bug. The challenge is that we have no consistent way to signal a JavaScript null value for primitive (i.e., unboxed) properties. To return to the flex example which led me to discover this bug, the flex property in Yoga is a primitive float. If the user defines flex: 1 in JavaScript then later sets flex: undefined, what value should we write to the primitive float?

The original attempt to solve this problem (in 62177db) used a heuristic approach, opting to consider the initial value of the property to be its null representation. Our inability to read the initial value for properties that lack a getter led to the use of uninitialized memory, which is clearly a bug.

In an ideal world, we could mandate that all native props exposed to JavaScript be boxed types, but that ship has clearly sailed. To further improve the heuristic approach (while also avoiding a noisy assertion), I propose we do the following:

1. Stop using convention-based name-mangling to discover the getter and setter for a given property. Right now, we're constructing accessor selectors by tacking set onto a property name. The Obj-C runtime exposes reflection methods for this purpose; notably class_getProperty, which will return the customer getter and setter selectors if they've been overridden. If we switch to using proper runtime property reflection, we'll address the case above where boolean properties have overridden the getter to have an is prefix.

2. Fall back to intuitive null values for each individual primitive type. In the event that a getter simply doesn't exist, we should use the value that most intuitively reflects null. For numbers, this is the value 0; for C strings, the empty string; for ids, the value nil. This isn't perfect, but it's certainly better than using random data. The alternative is that we leave the assertion in and force developers (especially of third-party modules) to always provide symmetric native properties. If we proceed down this route, we'll have to consider this a breaking change.

I'd like to hear your thoughts. If you think the combined approach above is enough to lay this issue to rest, I'll code it up and update the branch. If you think we should treat asymmetrical properties as errors, we'll need to announce this change as breaking (and suss out whether there are any further asymmetrical properties in the core react-native libs themselves).

[javache]

Actually, the simpler option here may be to use valueForKey which automatically checks methods like isFoo when finding a getter: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/KeyValueCoding/SearchImplementation.html#//apple_ref/doc/uid/20000955

[javache]

And I think it would be fine for this to be a breaking change. I haven't checked the internal tests so I'm not sure how much breakage this will cause, but perhaps to limit the impact of the breakage we could only enforce the assert for primitive type and default to nil for everything that's an object?

[facebook-github-bot]

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

[javache]

Actually, the simpler option here may be to use valueForKey which automatically checks methods like isFoo when finding a getter:

Except that we use this macro for all getting all sorts of types, so it doesn't play nicely with primitives.

[jamesreggio]

Regarding valueForKey:... ugh. What a horrible programming model. I guessed it's an Apple-blessed heuristic for 'finding' a property, but that whole thing deserves to be burned to the ground. Could you explain a little more about this comment?

Except that we use this macro for all getting all sorts of types, so it doesn't play nicely with primitives.

According to the docs, valueForKey: will box primitives in either an NSNumber or NSValue instance, which requires additional logic for selecting the correct method for unboxing to the desired primitive type. I'm not entirely sure it's worth the added effort.

If we make this a breaking change, it's worth noting that it will impact almost all third-party modules, much in the way that 0.40 basically forced everybody to rev their modules. Are you really sure the RN team would be on board with that?

[javache]

A simpler way to just avoid a crash here is to extend the RCT_CASE macro with a default param. This could be used when no default getter is found.

[hramos]

This PR appears to be abandoned. Closing for now.