Fix crash in getChildDrawingOrder #40859

toby-coding · 2023-10-12T14:06:46Z

Summary:

Problem Causes: In ReactViewGroup, there is a conflict between the zIndex attribute and the removeClippedSubviews optimization attribute. When both are used at the same time, the array mDrawingOrderIndices in ViewGroupDrawingOrderHelper that records the rendering order of subviews is not reset when super is called in the updateSubviewClipStatus method to add and remove subviews.

Solution: Because there are many third-party components that inherit from or depend on ReactViewGroup, all methods for adding and removing subviews in ViewGroup need to be override in ReactViewGroup, and ViewGroupDrawingOrderHelper corresponding to handleAddView and handleRemoveView needs to be called in these methods. And all the precautions for directly calling super to add and remove subviews are changed to calling the overridden method by ReactViewGroup.

Special Note: All addView related methods in ViewGroup will eventually be called to the addView(View child, int index, LayoutParams params) method, except addViewInLayout. Regarding the method of adding subviews, we only need to override addView(View child, int index, LayoutParams params) and addViewInLayout(View child, int index, LayoutParams params,boolean preventRequestLayout) in ReactViewGroup.

Changelog:

[Android] [Fixed] - Fix the crash in ReactViewGroup of #30785

facebook-github-bot · 2023-10-12T14:06:53Z

Hi @toby-coding!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

analysis-bot · 2023-10-12T14:37:41Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,222,145	+7,519
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,592,192	+4,817
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: a2f1189
Branch: main

facebook-github-bot · 2023-10-12T15:09:28Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

javache · 2023-10-12T15:24:00Z

There's a comment that says

"This will get called for every overload of addView so there is not need to override every"

Is this not accurate anymore?

github-actions · 2023-10-12T15:38:02Z

	Warnings
⚠️	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
⚠️	One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against e9effe1

toby-coding · 2023-10-13T02:09:32Z

There's a comment that says

"This will get called for every overload of addView so there is not need to override every"

Is this not accurate anymore?
Because
All addView related methods in ViewGroup will eventually be called to the addView(View child, int index, LayoutParams params) method, except addViewInLayout.So we only need to override addView(View child, int index, LayoutParams params) and addViewInLayout.

javache

Looks good to me. Some small comments

javache · 2023-10-13T09:27:19Z

...ive/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupDrawingOrderHelper.java

@@ -65,6 +65,11 @@ public boolean shouldEnableCustomDrawingOrder() {
   * ViewGroup#getChildDrawingOrder}.
   */
  public int getChildDrawingOrder(int childCount, int index) {
+    if (mDrawingOrderIndices != null
+      && (index >= mDrawingOrderIndices.length || mDrawingOrderIndices[index] >= childCount)) {


Is this ever expected to happen under normal conditions? Can we log a warning here that mDrawingOrderIndices is out-of-sync?

It will not happen under normal circumstances, but it is not ruled out that some third-party components may add or remove subview in the working thread.

javache · 2023-10-13T09:28:10Z

...es/react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java

@@ -524,22 +519,60 @@ public void removeView(View view) {
    } else {
      setChildrenDrawingOrderEnabled(false);
    }
+  }
+
+  public void handleRemoveViews(int start, int count) {


Should be private

Should be private

done

javache · 2023-10-16T08:40:53Z

...ive/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupDrawingOrderHelper.java

+      FLog.w(ReactConstants.TAG,
+        "getChildDrawingOrder index out of bounds! please check your operations of addding and " +


FLog takes a format string. Let's also not make this too verbose, you can always attach a debugger to inspect the full state.

Suggested change

FLog.w(ReactConstants.TAG,

"getChildDrawingOrder index out of bounds! please check your operations of addding and " +

FLog.w(ReactConstants.TAG,

"getChildDrawingOrder index out of bounds! Please check any costom view manipulations you may have done. childCount = %d, index = %d", ..)

FLog takes a format string. Let's also not make this too verbose, you can always attach a debugger to inspect the full state.

done

facebook-github-bot · 2023-10-16T12:27:00Z

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

facebook-github-bot · 2023-10-17T13:30:05Z

@javache merged this pull request in b941831.

Summary: Problem Causes: In ReactViewGroup, there is a conflict between the zIndex attribute and the removeClippedSubviews optimization attribute. When both are used at the same time, the array mDrawingOrderIndices in ViewGroupDrawingOrderHelper that records the rendering order of subviews is not reset when super is called in the updateSubviewClipStatus method to add and remove subviews. Solution:�Because there are many third-party components that inherit from or depend on ReactViewGroup, all methods for adding and removing subviews in ViewGroup need to be override in ReactViewGroup, and ViewGroupDrawingOrderHelper corresponding to handleAddView and handleRemoveView needs to be called in these methods. And all the precautions for directly calling super to add and remove subviews are changed to calling the overridden method by ReactViewGroup. Special Note: All addView related methods in ViewGroup will eventually be called to the addView(View child, int index, LayoutParams params) method, except addViewInLayout. Regarding the method of adding subviews, we only need to override addView(View child, int index, LayoutParams params) and addViewInLayout(View child, int index, LayoutParams params,boolean preventRequestLayout) in ReactViewGroup. ## Changelog: [Android] [Fixed] - Fix the crash in ReactViewGroup of facebook#30785 Pull Request resolved: facebook#40859 Reviewed By: NickGerleman Differential Revision: D50321718 Pulled By: javache fbshipit-source-id: 7fa7069937b8c2afb9f30dd10554370b1be5d515

Summary: Problem Causes: In ReactViewGroup, there is a conflict between the zIndex attribute and the removeClippedSubviews optimization attribute. When both are used at the same time, the array mDrawingOrderIndices in ViewGroupDrawingOrderHelper that records the rendering order of subviews is not reset when super is called in the updateSubviewClipStatus method to add and remove subviews. Solution:�Because there are many third-party components that inherit from or depend on ReactViewGroup, all methods for adding and removing subviews in ViewGroup need to be override in ReactViewGroup, and ViewGroupDrawingOrderHelper corresponding to handleAddView and handleRemoveView needs to be called in these methods. And all the precautions for directly calling super to add and remove subviews are changed to calling the overridden method by ReactViewGroup. Special Note: All addView related methods in ViewGroup will eventually be called to the addView(View child, int index, LayoutParams params) method, except addViewInLayout. Regarding the method of adding subviews, we only need to override addView(View child, int index, LayoutParams params) and addViewInLayout(View child, int index, LayoutParams params,boolean preventRequestLayout) in ReactViewGroup. ## Changelog: [Android] [Fixed] - Fix the crash in ReactViewGroup of #30785 Pull Request resolved: #40859 Reviewed By: NickGerleman Differential Revision: D50321718 Pulled By: javache fbshipit-source-id: 7fa7069937b8c2afb9f30dd10554370b1be5d515

Summary: Problem Causes: In ReactViewGroup, there is a conflict between the zIndex attribute and the removeClippedSubviews optimization attribute. When both are used at the same time, the array mDrawingOrderIndices in ViewGroupDrawingOrderHelper that records the rendering order of subviews is not reset when super is called in the updateSubviewClipStatus method to add and remove subviews. Solution:�Because there are many third-party components that inherit from or depend on ReactViewGroup, all methods for adding and removing subviews in ViewGroup need to be override in ReactViewGroup, and ViewGroupDrawingOrderHelper corresponding to handleAddView and handleRemoveView needs to be called in these methods. And all the precautions for directly calling super to add and remove subviews are changed to calling the overridden method by ReactViewGroup. Special Note: All addView related methods in ViewGroup will eventually be called to the addView(View child, int index, LayoutParams params) method, except addViewInLayout. Regarding the method of adding subviews, we only need to override addView(View child, int index, LayoutParams params) and addViewInLayout(View child, int index, LayoutParams params,boolean preventRequestLayout) in ReactViewGroup. [Android] [Fixed] - Fix the crash in ReactViewGroup of facebook/react-native#30785 Pull Request resolved: facebook/react-native#40859 Reviewed By: NickGerleman Differential Revision: D50321718 Pulled By: javache fbshipit-source-id: 7fa7069937b8c2afb9f30dd10554370b1be5d515

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 12, 2023

javache changed the title ~~fix the crash of https://github.com/facebook/react-native/issues/30785~~ Fix crash in getChildDrawingOrder Oct 13, 2023

javache reviewed Oct 13, 2023

View reviewed changes

huangtaibin added 2 commits October 14, 2023 19:28

fix the crash of facebook#30785

e6ea9b3

fix:add warning log

9258998

toby-coding force-pushed the main branch from 91f98c1 to 9258998 Compare October 14, 2023 11:53

toby-coding requested a review from javache October 15, 2023 07:53

javache reviewed Oct 16, 2023

View reviewed changes

fix:Optimize log format

e9effe1

facebook-github-bot closed this in b941831 Oct 17, 2023

facebook-github-bot added the Merged This PR has been merged. label Oct 17, 2023

This was referenced Feb 29, 2024

[RN][iOS]Rename BUILD_FROM_SOURCE to RCT_BUILD_HERMES_FROM_SOURCE #43247

Merged

[RN][iOS][0.72] Fix flipper for Xcode 15.3 #43344

Merged

jacobmolby mentioned this pull request Mar 20, 2024

[0.73] "Fix crash in getChildDrawingOrder" (#40859) is included in 0.72.11 and 0.74. But not in 0.73.6 reactwg/react-native-releases#164

Closed

NOUIY mentioned this pull request May 10, 2024

[Snyk] Upgrade react-native from 0.63.3 to 0.73.7 NOUIY/react-native-macos#1292

Open

cipolleschi mentioned this pull request Jun 28, 2024

[CI] Move machines to M1 for 0.73 #45214

Merged

cipolleschi mentioned this pull request Jun 28, 2024

[CI] Move machines to M1 for 0.72 #45215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash in getChildDrawingOrder #40859

Fix crash in getChildDrawingOrder #40859

toby-coding commented Oct 12, 2023 •

edited

Loading

facebook-github-bot commented Oct 12, 2023

analysis-bot commented Oct 12, 2023 •

edited

Loading

facebook-github-bot commented Oct 12, 2023

javache commented Oct 12, 2023

github-actions bot commented Oct 12, 2023 •

edited

Loading

toby-coding commented Oct 13, 2023 •

edited

Loading

javache left a comment

javache Oct 13, 2023

toby-coding Oct 14, 2023

javache Oct 13, 2023

toby-coding Oct 14, 2023

javache Oct 16, 2023

toby-coding Oct 16, 2023

facebook-github-bot commented Oct 16, 2023

facebook-github-bot commented Oct 17, 2023

		FLog.w(ReactConstants.TAG,
		"getChildDrawingOrder index out of bounds! please check your operations of addding and " +

Fix crash in getChildDrawingOrder #40859

Fix crash in getChildDrawingOrder #40859

Conversation

toby-coding commented Oct 12, 2023 • edited Loading

Summary:

Changelog:

facebook-github-bot commented Oct 12, 2023

Action Required

Process

analysis-bot commented Oct 12, 2023 • edited Loading

facebook-github-bot commented Oct 12, 2023

javache commented Oct 12, 2023

github-actions bot commented Oct 12, 2023 • edited Loading

toby-coding commented Oct 13, 2023 • edited Loading

javache left a comment

Choose a reason for hiding this comment

javache Oct 13, 2023

Choose a reason for hiding this comment

toby-coding Oct 14, 2023

Choose a reason for hiding this comment

javache Oct 13, 2023

Choose a reason for hiding this comment

toby-coding Oct 14, 2023

Choose a reason for hiding this comment

javache Oct 16, 2023

Choose a reason for hiding this comment

toby-coding Oct 16, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Oct 16, 2023

facebook-github-bot commented Oct 17, 2023

toby-coding commented Oct 12, 2023 •

edited

Loading

analysis-bot commented Oct 12, 2023 •

edited

Loading

github-actions bot commented Oct 12, 2023 •

edited

Loading

toby-coding commented Oct 13, 2023 •

edited

Loading