From b5cd9e4db1bc91ac3fe6cad3d408f8f99616bb10 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Wed, 12 Jun 2019 21:13:46 -0700 Subject: [PATCH] Fabric: Moving `[super updateProps:props oldProps:oldProps];` to the end of the methods Summary: In some cases, the view class is the only that retains stored `props_` variable. At the same time the `[super updateProps:props oldProps:oldProps];` actually resets the pointer with a pointer to new props value which sometimes causes the deallocation of the old value. All that is okay unless the implementation of `updateProps:oldProps:` in superclasses stores a raw reference to an old value in the very beginning of the method (for convenience and perf reasons). So, to prevent preliminary deallocation of the old value pointed by `_props` we moved all `[super updateProps:props oldProps:oldProps];` calls to the end of overloaded methods. Reviewed By: mdvacca Differential Revision: D15770068 fbshipit-source-id: af36b3e70560ab00846cd26b0963bbc059e977bc --- .../RCTActivityIndicatorViewComponentView.mm | 4 ++-- .../Mounting/ComponentViews/Image/RCTImageComponentView.mm | 4 ++-- .../ComponentViews/ScrollView/RCTScrollViewComponentView.mm | 4 ++-- .../ScrollView/RNPullToRefreshViewComponentView.mm | 4 ++-- .../Mounting/ComponentViews/Slider/RCTSliderComponentView.mm | 4 ++-- .../Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm | 4 ++-- .../Mounting/ComponentViews/Text/RCTParagraphComponentView.mm | 4 ++-- .../RCTUnimplementedNativeComponentView.mm | 4 ++-- .../Mounting/ComponentViews/View/RCTViewComponentView.h | 1 + .../Mounting/ComponentViews/View/RCTViewComponentView.mm | 4 ++-- 10 files changed, 19 insertions(+), 18 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm index 4dbb3150a28..f8ccc1024d0 100644 --- a/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ActivityIndicator/RCTActivityIndicatorViewComponentView.mm @@ -63,8 +63,6 @@ static UIActivityIndicatorViewStyle convertActivityIndicatorViewStyle(const Acti const auto &oldViewProps = *std::static_pointer_cast(_props); const auto &newViewProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - if (oldViewProps.animating != newViewProps.animating) { if (newViewProps.animating) { [_activityIndicatorView startAnimating]; @@ -85,6 +83,8 @@ static UIActivityIndicatorViewStyle convertActivityIndicatorViewStyle(const Acti if (oldViewProps.size != newViewProps.size) { _activityIndicatorView.activityIndicatorViewStyle = convertActivityIndicatorViewStyle(newViewProps.size); } + + [super updateProps:props oldProps:oldProps]; } @end diff --git a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm b/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm index d20eba79641..955a315eb4d 100644 --- a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm @@ -55,8 +55,6 @@ const auto &oldImageProps = *std::static_pointer_cast(_props); const auto &newImageProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - // `resizeMode` if (oldImageProps.resizeMode != newImageProps.resizeMode) { if (newImageProps.resizeMode == ImageResizeMode::Repeat) { @@ -72,6 +70,8 @@ if (oldImageProps.tintColor != newImageProps.tintColor) { _imageView.tintColor = [UIColor colorWithCGColor:newImageProps.tintColor.get()]; } + + [super updateProps:props oldProps:oldProps]; } - (void)updateLocalData:(SharedLocalData)localData oldLocalData:(SharedLocalData)oldLocalData diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm index faba12d0783..6bc664b43c0 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm @@ -74,8 +74,6 @@ using namespace facebook::react; const auto &oldScrollViewProps = *std::static_pointer_cast(_props); const auto &newScrollViewProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - #define REMAP_PROP(reactName, localName, target) \ if (oldScrollViewProps.reactName != newScrollViewProps.reactName) { \ target.localName = newScrollViewProps.reactName; \ @@ -113,6 +111,8 @@ using namespace facebook::react; // MAP_SCROLL_VIEW_PROP(scrollIndicatorInsets); // MAP_SCROLL_VIEW_PROP(snapToInterval); // MAP_SCROLL_VIEW_PROP(snapToAlignment); + + [super updateProps:props oldProps:oldProps]; } - (void)updateState:(State::Shared const &)state oldState:(State::Shared const &)oldState diff --git a/React/Fabric/Mounting/ComponentViews/ScrollView/RNPullToRefreshViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/ScrollView/RNPullToRefreshViewComponentView.mm index 1186ad90507..b8a3aa4aade 100644 --- a/React/Fabric/Mounting/ComponentViews/ScrollView/RNPullToRefreshViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/ScrollView/RNPullToRefreshViewComponentView.mm @@ -53,8 +53,6 @@ using namespace facebook::react; auto const &oldConcreteProps = *std::static_pointer_cast(_props); auto const &newConcreteProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - if (newConcreteProps.refreshing != oldConcreteProps.refreshing) { if (newConcreteProps.refreshing) { [_refreshControl beginRefreshing]; @@ -76,6 +74,8 @@ using namespace facebook::react; if (needsUpdateTitle) { [self _updateTitle]; } + + [super updateProps:props oldProps:oldProps]; } #pragma mark - diff --git a/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm b/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm index dd650140398..cb0bb3757b6 100644 --- a/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm @@ -112,8 +112,6 @@ using namespace facebook::react; const auto &oldSliderProps = *std::static_pointer_cast(_props); const auto &newSliderProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - // `value` if (oldSliderProps.value != newSliderProps.value) { _sliderView.value = newSliderProps.value; @@ -149,6 +147,8 @@ using namespace facebook::react; if (oldSliderProps.maximumTrackTintColor != newSliderProps.maximumTrackTintColor) { _sliderView.maximumTrackTintColor = [UIColor colorWithCGColor:newSliderProps.maximumTrackTintColor.get()]; } + + [super updateProps:props oldProps:oldProps]; } - (void)updateLocalData:(SharedLocalData)localData oldLocalData:(SharedLocalData)oldLocalData diff --git a/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm b/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm index bc5f7fc323c..c4183aecb17 100644 --- a/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Switch/RCTSwitchComponentView.mm @@ -48,8 +48,6 @@ using namespace facebook::react; const auto &oldSwitchProps = *std::static_pointer_cast(_props); const auto &newSwitchProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - // `value` if (oldSwitchProps.value != newSwitchProps.value) { _switchView.on = newSwitchProps.value; @@ -75,6 +73,8 @@ using namespace facebook::react; if (oldSwitchProps.thumbTintColor != newSwitchProps.thumbTintColor) { _switchView.thumbTintColor = [UIColor colorWithCGColor:newSwitchProps.thumbTintColor.get()]; } + + [super updateProps:props oldProps:oldProps]; } - (void)onChange:(UISwitch *)sender diff --git a/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm b/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm index 1da31b99f15..50cc341874d 100644 --- a/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/Text/RCTParagraphComponentView.mm @@ -57,10 +57,10 @@ using namespace facebook::react; { const auto ¶graphProps = std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - assert(paragraphProps); _paragraphAttributes = paragraphProps->paragraphAttributes; + + [super updateProps:props oldProps:oldProps]; } - (void)updateLocalData:(SharedLocalData)localData oldLocalData:(SharedLocalData)oldLocalData diff --git a/React/Fabric/Mounting/ComponentViews/UnimplementedComponent/RCTUnimplementedNativeComponentView.mm b/React/Fabric/Mounting/ComponentViews/UnimplementedComponent/RCTUnimplementedNativeComponentView.mm index bbc1ba305ca..dd9555959db 100644 --- a/React/Fabric/Mounting/ComponentViews/UnimplementedComponent/RCTUnimplementedNativeComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/UnimplementedComponent/RCTUnimplementedNativeComponentView.mm @@ -50,11 +50,11 @@ using namespace facebook::react; const auto &oldViewProps = *std::static_pointer_cast(_props); const auto &newViewProps = *std::static_pointer_cast(props); - [super updateProps:props oldProps:oldProps]; - if (oldViewProps.name != newViewProps.name) { _label.text = [NSString stringWithFormat:@"'%s' is not Fabric compatible yet.", newViewProps.name.c_str()]; } + + [super updateProps:props oldProps:oldProps]; } @end diff --git a/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.h b/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.h index c4803845ad7..2b989fcffe7 100644 --- a/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.h +++ b/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.h @@ -70,6 +70,7 @@ NS_ASSUME_NONNULL_BEGIN /** * Enforcing `call super` semantic for overridden methods from `RCTComponentViewProtocol`. + * The methods update the instance variables. */ - (void)updateProps:(facebook::react::Props::Shared const &)props oldProps:(facebook::react::Props::Shared const &)oldProps NS_REQUIRES_SUPER; diff --git a/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm b/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm index b627db40cbc..4efa6a34370 100644 --- a/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm +++ b/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm @@ -110,8 +110,6 @@ using namespace facebook::react; auto const &oldViewProps = *std::static_pointer_cast(_props); auto const &newViewProps = *std::static_pointer_cast(props); - _props = std::static_pointer_cast(props); - BOOL needsInvalidateLayer = NO; // `opacity` @@ -241,6 +239,8 @@ using namespace facebook::react; } _needsInvalidateLayer = _needsInvalidateLayer || needsInvalidateLayer; + + _props = std::static_pointer_cast(props); } - (void)updateEventEmitter:(EventEmitter::Shared const &)eventEmitter