Fallback to Shadow Tree measurement if native measure fails by hgarfinkle · Pull Request #49 · discord/react-native

hgarfinkle · 2023-12-08T19:56:01Z

When tapping a hyperlink that exists as a child of another <Text> element, the app crashes. This happens because the NativeFabricMeasurerTurboModule is called for the hyperlink's node tag as it exists in the c++ shadow tree, and that tag is (for reasons we don't understand) different from the actual tag that exists in the native layout. Thus, the TurboModule ends up looking for info on a node tag that doesn't exist, which crashes the native app. To resolve this, this PR gracefully handles that error, and in such a case we fallback to determining layout information from the shadow tree, as we were doing before Alex's TurboModule.

hgarfinkle · 2023-12-22T22:25:20Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+            turboModuleCalled = true;
          }

          if (turboModuleCalled) {


we're setting turboModuleCalled = true two lines above this. why check now if it's set? why not just return Value::undefined() above?

Probably was a rough edge around the initial implementation of this. Your recommendation sounds like a cleaner way to do it.

pmick

Let me re-describe how I perceive this to work and you let me know if I'm getting all of this.

With the old code, we'd measure the native component. This would work by using the view's tag to find the view in native land and then measure that native view in java. This was throwing an exception though which would silently fail resulting in links being untappable. (or would it crash the app?) And this was because we couldn't actually find the native view?

The new code now catches that exception and will fire a native measure failure callback to the cpp layer. Once the cpp layer gets a failure callback it'll attempt to instead measure the component using the layout metrics from the shadow tree.

As a result of the change, the majority of measurements will continue to work exactly the same. Only in instances where we're looking for a view by an invalid react tag will we choose to measure by the shadow tree. This is probably anywhere that we're moving views around in the native view heirarchy after they've been measured and positioned by rn.

One other thought. Is this cpp exclusive to android or are we potentially changing some iOS implementation too?

pmick · 2024-01-03T19:32:58Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+                              // We return onSuccessFunction out of the onFail
+                              // instead of using the one defined above to avoid
+                              // a scoping bug.


We might want to rewrite this comment to describe a bit better what the scoping bug is. Essentially something about how functions can't be passed into capture lists because they can't be copied so we need to get a new reference to the function. Unsure of how to phrase that 🤔

hgarfinkle · 2024-01-03T19:45:31Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

  uiManager_->setDelegate(nullptr);
 }

+void measureFromShadowTree(


I wanted to break this work out into a method. It's my understanding c++ doesn't allow you to introduce functions inside functions, so I had to do it here. Is that right? Is there not a way for this code to show up closer to where it's used?

This seems correct. You could maybe move it closer by making a lambda, or a macro, but those introduce their own set of tradeoffs.

You probably want to make this a proper private method of the UIManagerBinding class though.

joemun · 2024-01-03T21:01:34Z

Wondering if we should add a log message to surface how often this fallback logic is happening, since ultimately this is workaround that shouldn't exist ideally.

Flewp

After addressing comments, I think this would probably be okay to land. At the end of the day, this is hopefully a stopgap.

My biggest concern here is the usage of createFromHostFunction to create a callback to pass to Java/Obj-C for two reasons:

I don't know if calling createFromHostFunction with the same name repeatedly every measure call is going to just rewrite the previous definition of the function, or produce errors in certain environments.
We're entangling C++ and Java here that feel against the grain. We're getting around C++ lambda capture by passing the success callback back to C++ as an argument of the failure callback; that doesn't feel right. If we needed to do this for iOS, I'm not sure if that platform would behave similarly or have means to do that.

From a high level, these measure and measureInWindow functions reside in the UIManagerBinding::get, which is a JSI HostObject override, and UIManagerBinding represents the global.nativeFabricUIManager object in JS. Therefore, whenever we call global.nativeFabricUIManager.METHOD_NAME, it'll call this get function with "METHOD_NAME" as the name argument. A potential way to clean this up and not have to do additional createFromHostFunction shenans is to introduce a new parameter to measure and measureInWindow: forceShadowTree. If forceShadowTree is true, it doesn't try to call measureNatively, and instead skips to the shadow tree logic immediately. In the JS impl where it's calling measure, if you get back an invalid measure return value, you can call measure again with forceShadowTree = true. It'll allow you to manage your callbacks and such completely in JS.

If you're down for it, give the above a shot. Otherwise, we can keep iterating on this when we return to Fabric work. Thanks for the spike into this, and nice job finding the underlying issue 👍

Flewp · 2024-01-08T15:52:03Z

ReactAndroid/src/main/java/com/facebook/react/animated/NativeFabricMeasurerModule.java

+        onSuccess.invoke(x, y, width, height);
+      }
+      catch (IllegalViewOperationException e) {
+        onFail.invoke(onSuccess);


If keeping this approach, I'd put the above comment about scoping here too.

Flewp · 2024-01-08T17:10:48Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

  uiManager_->setDelegate(nullptr);
 }

+void measureFromShadowTree(


This seems correct. You could maybe move it closer by making a lambda, or a macro, but those introduce their own set of tradeoffs.

You probably want to make this a proper private method of the UIManagerBinding class though.

Flewp · 2024-01-08T17:14:04Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+            turboModuleCalled = true;
          }

          if (turboModuleCalled) {


Probably was a rough edge around the initial implementation of this. Your recommendation sounds like a cleaner way to do it.

Flewp · 2024-01-08T17:16:45Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+                  .asObject(runtime)
+                  .asFunction(runtime)
+                  .call(runtime, "NativeFabricMeasurerTurboModule");
+          auto onSuccessFunction =


Move this into the if statement

Flewp · 2024-01-08T17:17:22Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+                  .asObject(runtime)
+                  .asFunction(runtime)
+                  .call(runtime, "NativeFabricMeasurerTurboModule");
+          auto onSuccessFunction =


Same as the if comment above

Flewp · 2024-01-08T17:19:06Z

ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp

+                        jsi::Function::createFromHostFunction(
+                            runtime,
+                            jsi::PropNameID::forAscii(
+                                runtime, "onNativeMeasureFail"),


createFromHostFunction is decorating the JS runtime, and I'm not sure what the behavior would be if you had conflicting names. I'd rename this to something like onNativeMeasureInWindowFail

Isaac Garfinkle added 9 commits December 8, 2023 11:50

WIP fallback to shadow tree measurement if node tag not found

4e7fe95

fix type issues, c++ paradigms

60fda14

fix type error with dereference

d1b44dc

no type issues, untested

7513657

app still crashes when you tap anything but it starts

7fe78d0

two separate callbacks, pass successfunction back

de30705

formatting and readability

cbd01c0

unneeded #includes

3bbd06d

revert overzealous formatting

0275515

hgarfinkle commented Dec 22, 2023

View reviewed changes

hgarfinkle marked this pull request as ready for review December 22, 2023 22:25

hgarfinkle requested review from Flewp, joemun and pmick December 22, 2023 22:26

hgarfinkle requested a review from meishuu January 3, 2024 18:34

pmick reviewed Jan 3, 2024

View reviewed changes

hgarfinkle commented Jan 3, 2024

View reviewed changes

Flewp approved these changes Jan 8, 2024

View reviewed changes

Isaac Garfinkle added 2 commits January 8, 2024 16:11

no need for checking boolean to return, just return

6a7d884

more verbose comment

9759572

yayvery force-pushed the 0.72.3-discord-1 branch from bd6a5ae to 66de003 Compare January 11, 2024 23:46

DDRBoxman force-pushed the 0.72.3-discord-1 branch from 00fe71c to 723d2f4 Compare February 20, 2024 22:20

Conversation

hgarfinkle commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joemun commented Jan 3, 2024

Uh oh!

Flewp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hgarfinkle commented Dec 8, 2023 •

edited

Loading