Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS [RCTShadowText buildTextStorageForWidth: widthMode:] will cause app infinite-loop and memory-blast in a very small probability #11412

Closed
ywz2010 opened this issue Dec 12, 2016 · 7 comments
Labels
Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@ywz2010
Copy link

ywz2010 commented Dec 12, 2016

We use react-native 0.33.0. And we find that in some situation, the memory will non-stop increase and cause the app crashed. At the meaning time the react-native page is just empty. This often happen after we upgrade the app from appStore.

It happen by chance when I run app in Xcode , and I recored the thread and find that [RCTShadowText buildTextStorageForWidth:width widthMode:widthMode] cause app infinite-loop and memory-blast.

I find it like #7851, but I can't reproduce it.
14814384939130

@ywz2010 ywz2010 changed the title iOS [RCTShadowText buildTextStorageForWidth:width widthMode:widthMode] will cause app infinite-loop and memory-blast in a very small probability iOS [RCTShadowText buildTextStorageForWidth: widthMode:] will cause app infinite-loop and memory-blast in a very small probability Dec 12, 2016
@littlesome
Copy link

Hi I got the same error. I think it's caused by buildTextStorageForWidth width passed in is NaN. I just add a check and return, things works and not get this stuck again. This is a temp workaround, maybe someone can help to check why NaN is passed in.

@camellieeee
Copy link

+1

@shergin shergin added the Platform: iOS iOS applications. label Mar 16, 2017
@ywz2010 ywz2010 closed this as completed Jun 15, 2017
@atticoos
Copy link
Contributor

atticoos commented Jan 23, 2018

I noticed this got closed, but noticed there are no commits, PRs, or other issues linking to this.

Did this get fixed, and if so, do you happen to recall any diffs that can be referenced?

We're seeing noticeable growth by this method as well. 8.5KiB every so often in the app.

@ywz2010
Copy link
Author

ywz2010 commented Jan 24, 2018

@ajwhite I closed this issue because I used some protective code and this never happen again. But I still don't know why this happen and what the protective code will cause in this case, so I don't submit PR.

In my situation, shadowQueue is busy, so the RN page will not render any new View.

The protective code below is inspired by littlesome.

- (NSTextStorage *)buildTextStorageForWidth:(CGFloat)width widthMode:(YGMeasureMode)widthMode
{

... 

  textContainer.maximumNumberOfLines = _numberOfLines;
  textContainer.size = (CGSize){widthMode == YGMeasureModeUndefined ? CGFLOAT_MAX : width, CGFLOAT_MAX};

  // protective code ↓
  if (isnan(textContainer.size.width)) {
    textContainer.size = (CGSize){CGFLOAT_MAX, CGFLOAT_MAX};
  }
   // protective code ↑

  [layoutManager addTextContainer:textContainer];
  [layoutManager ensureLayoutForTextContainer:textContainer];

  _cachedTextStorageWidth = width;
  _cachedTextStorageWidthMode = widthMode;
  _cachedTextStorage = textStorage;

  return textStorage;
}

@atticoos
Copy link
Contributor

atticoos commented Jan 24, 2018

Interesting. I've observed a leak in this specific function as well and attempted to mitigate with a patch.

I've observed that any time the font width changes (which brings us here), a Malloc 8.5Kib would be retained. It sounds related to what you experienced.

I attempted to unlink the NSLayoutManager and NSTextContainer to see if they somehow had remaining references being held after _cachedTextStorage becomes reassigned. No dice.

Note: we're running the 0.27 version of this file, before Yoga. However, I don't see that as an offender.

// called before _cachedTextStorage reassignment and when invalidation occurrs
-(void)cleanupTextStorageReferences
{
 if (!_cachedTextStorage) {
   return;
 }
 
 NSArray<NSLayoutManager *> *layoutManagers = _cachedTextStorage.layoutManagers;
 
 for (int i = 0; i < layoutManagers.count; i++) {
   
   NSArray<NSTextContainer *> *textContainers = layoutManagers[i].textContainers;
   for (int j = 0; j < textContainers.count; j++) {
     [layoutManagers[i] removeTextContainerAtIndex:j];
   }

   [_cachedTextStorage removeLayoutManager:layoutManagers[i]];
 }
}

In my case I had a clock on the display, and as the numbers ticked every minute, buildTextStorageForWidth:widthMode: would leak due to the characters changing and causing slight width changes. Given that, I assume a reproducible example would just be a Text component with growing content.


I'm going to give your version a shot tomorrow. Thanks for sharing, as I didn't see a infinite loop on my end and may not have came to the same conclusion. Certainly no issues with our queue being busy, unless it only applies to children of the View..

I'd urge you to reopen this issue and update the description to include the protective code that solved your problem. Every user of the library is affected by this nasty one. 8.5KiB is no joke! With some additional 👀, this should be pretty solvable.

@ywz2010
Copy link
Author

ywz2010 commented Jan 26, 2018

@ajwhite Thanks for you advise. But I don't want to reopen this issue now. It was on v0.33.0, and now the latest version is v0.53.0-rc. I have changed job and not focus in React Native now, so I can't check if the buy still exist. If some one say this happen in the latest version, I will reopen it.

@atticoos
Copy link
Contributor

We are going to be running some isolated tests and put it up in an example repository if we reproduce on the latest version. I will move that to a new issue that I'll maintain, and link back to this one and any other related threads.

Thank you for continuing to respond to these issues even thought they no longer affect you. I genuinely appreciate your time.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

6 participants