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

[Layout] Navigating back to scene with flexWrap + flexDirection sends CPU to 100% #1378

Closed
rxb opened this issue May 22, 2015 · 30 comments
Closed
Assignees
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@rxb
Copy link
Contributor

rxb commented May 22, 2015

To reproduce:

  • Have a scene using flexWrap: 'wrap' and flexDirection: 'row' (my component: https://gist.github.com/rxb/d37f05d6ad16be1a1afe)
  • Use Navigator to transition away
  • Attempt to transition back -- but the Navigator never transitions back and the CPU goes to 100%
  • If I remove the flexWrap and flexDirection rules, everything works fine and I can transition between those scenes with no problem.

Not sure if related to: #824
I first noticed while using 0.4.4. Still happening after upgrading to 0.5.0-rc

@rxb
Copy link
Contributor Author

rxb commented May 23, 2015

To narrow things down, tried a simpler layout with no ListView or ScrollView and it still happens:
https://gist.github.com/rxb/31068fa0041a2080f5de

@brentvatne
Copy link
Collaborator

@rxb - thanks for bug report and examples, it would be ideal the example includes all code necessary to reproduce the problem - currently no mention of navigator. Additionally, if you can post it on rnplay.org - eg: https://rnplay.org/plays/NVOjJA (feel free to fork this) that would help a lot too and it makes it easy to test it across various versions

@rxb
Copy link
Contributor Author

rxb commented May 24, 2015

Thanks for the help. I put together a better example here that reproduces it:
https://rnplay.org/plays/phXLkA

If you load this example, try switching to the second tab, then back to the first, that's where the hang is.
Then, if you comment out either of the style rules on lines 168 or 169 and re-run the app, transitioning back and forth works again.

@brentvatne
Copy link
Collaborator

Thanks @rxb! That's perfect.

@brentvatne
Copy link
Collaborator

Wanted to find out where this broke - 0.4.2 works, 0.4.3+ is broken.

@lwansbrough
Copy link
Contributor

@brentvatne Best course of action is probably to check any changes made to facebook/css-layout as that was the original source of the issue.

@brentvatne brentvatne changed the title Navigating back to scene with flexWrap + flexDirection sends CPU to 100% [Layout] Navigating back to scene with flexWrap + flexDirection sends CPU to 100% May 29, 2015
@brentvatne
Copy link
Collaborator

@lwansbrough - good point, do you have any time today to try that out?

@lwansbrough
Copy link
Contributor

@brentvatne Yeah I can take a look in a bit

@johanmic
Copy link

johanmic commented Jun 4, 2015

any plans for get this fix in for 0.5?

@bogdantmm92
Copy link

+1

@jsierles
Copy link
Contributor

Also saw this - it seems related to a regression in css-layout as mentioned above. I haven't had time yet to debug the specific issue, but testing with a newer version of Layout.c against master fixes this issue:

https://github.com/facebook/css-layout/fca176109d84a7b583bc091f91109d8c359ac1fc/src/Layout.c

I didn't try css-layout master which changes some core method signatures.

@jsierles
Copy link
Contributor

Also this commit, which went into 0.4.3, could be the source of the issue:

6c812c8

@lwansbrough
Copy link
Contributor

Nice work @jsierles, hopefully we can get this sorted for good.

@johanmic
Copy link

using the workaround in 1644

var Dimensions = require('Dimensions');
var windowSize = Dimensions.get('window');

and then using width: windowSize.width in its parent element seems to be a good enough solution.
if this is the standard going forward, I suggest mentioning the width requirement in the docs.

@yelled3
Copy link
Contributor

yelled3 commented Jun 25, 2015

@brentvatne

Wanted to find out where this broke - 0.4.2 works, 0.4.3+ is broken.

I'm currently using 0.6.0-rc and I encountered the same issue:

  • CPU is up to 100%
  • navigation doesn't work
  • no error message

it was my first time using the Navigatior
after a long debugging session I came across this issue :-)
@jaywalklabs workaround worked although, it was not clear who's the culprit was since I was using nested Navigatiors and the crash happened when try to transition to a 3rd screen, while the real issue was in the initial route.

@jaywalklabs

I suggest mentioning the width requirement in the docs.

couldn't agree more. in bold letters :-)

@jsierles
Copy link
Contributor

@vjeux So for now there's no plans to upgrade css-layout?

@vjeux
Copy link
Contributor

vjeux commented Jun 27, 2015

@jsierles: yes, we need to upgrade css-layout but when we tried last time it broke our internal apps because we relied on inconsistencies with the css spec :(

I don't have any date but it's certainly high on the list of things we need to do

@yelled3
Copy link
Contributor

yelled3 commented Jul 1, 2015

@jaywalklabs I ended up wrapping this hack into a component

const React = require('react-native');
const {
  StyleSheet,
  View
  } = React;

const Dimensions = require('Dimensions');
const windowSize = Dimensions.get('window');

const ScreenContainer = React.createClass({

  propTypes: {
    style: View.propTypes.style,
    children: React.PropTypes.node
  },

  render() {
    return (
      <View style={[styles.container, this.props.style]}>{this.props.children}</View>
    );
  }
});

const styles = StyleSheet.create({
  container: {
    flex: 1,
    width: windowSize.width
  }
});


module.exports = ScreenContainer;

thanks again.

@johanmic
Copy link

johanmic commented Jul 2, 2015

@yelled3 nice one! good work!

@yelled3
Copy link
Contributor

yelled3 commented Jul 2, 2015

@jaywalklabs

@JedWatson
Copy link

I encountered this just now with react-native@0.8.0, burned an hour or two while I figured out that the issue was related to the use of flexWrap on the screen I had previously navigated away from.

The specific behaviour I was seeing:

  • Render a scene in the Navigator component (scene A)
  • push a new scene that uses flexWrap for layout (scene B)
  • push another new scene onto the navigator (scene C). Scene B remains mounted off-screen
  • push another scene onto the navigator (scene D), or popToTop and the cpu hits 100%. Everything stops working.

I'm going to try the workaround wrapper @yelled3 suggests above (thanks!) but am also wondering if it's possible to implement a warning or something when these conditions exist?

Looks like this has been an issue for a while and it's a real showstopper when you encounter it. By pure chance, I already had scene C working with another flow, so I was able to figure out it must have something to do with navigating from A > B > C > D in that order, and I ripped out components from all of them at random until it stopped happening.

Alternatively we should really add a warning to the navigator / layout docs - happy to submit a PR if you like.

@sghiassy
Copy link

sghiassy commented Aug 5, 2015

I created an issue which is similar to this one: #2237. There's a link on the GitHub page to a sample app I created which repos the problem 100%.

One point of interest, is that the CPU pegging I'm seeing is on .push with React Native 0.8.0

@plrthink
Copy link
Contributor

also meet this issue on 0.8.0

@preppypiet
Copy link

I've encountered this issue about 3 separate times now. Most of the situations, the fix mentioned above regarding window size works just fine. Unfortunately I've run into this issue now inside a row-direction parent element (embedded a couple of layers). I can't use the fix/hack for obvious reasons. +1 for fixing :( currently on 0.10.0rc. I should note that in this case, I don't think it has anything to do with navigation. It just happens when using flex direction row + wrap inside a flex direction row parent element somewhere higher up the hierarchy (not immediate ancestor in my case).

@laurilehmijoki
Copy link
Contributor

In my case it seems that the <Navigator> is calling the renderScene function twice when it should call the function only once.

To illustrate the problem, here's I'm seeing the following in my console:

2015-08-18 15:06:17.721 [info][tid:com.facebook.React.JavaScript] 'ROUTE', { name: 'mainRoute' }
2015-08-18 15:06:20.385 [info][tid:com.facebook.React.JavaScript] 'ROUTE', { name: 'mainRoute' }
2015-08-18 15:06:20.388 [info][tid:com.facebook.React.JavaScript] 'ROUTE', { name: 'settingsRoute' }

And here's the code that prints the ROUTE message into the console:

<Navigator
    initialRoute={component.state.appStartedWithoutSiteKey ? {name: 'settingsRoute'} : {name: 'mainRoute'} }
    renderScene={(route, navigator) => {
        console.log('ROUTE', route)
        var routes = {
           'mainRoute': () => mainRoute.renderMainRoute(component.state, {
               navigateToSettings: () => navigator.push({name: 'settingsRoute'})
           }),
           'settingsRoute': () => settingsRoute.renderSettingsRoute(component.state, {
               navigateBack: navigator.getCurrentRoutes().length > 1 ? navigator.pop : undefined,
               navigateToMainView: () =>
                   navigator.getCurrentRoutes().length > 1 ?
                       navigator.pop() :
                       navigator.push({name: 'mainRoute'})
           })
        }
        return routes[route.name]()
    }}
/>

If I replace the style <View style={{flexDirection: 'row', flexWrap: 'wrap', justifyContent: 'space-between'}}> with <View style={{justifyContent: 'space-between'}}> in my mainRoute React element, then the app will not freeze, and the CPU usage will not rise to 100%.

Maybe the problem in my case is somehow caused by the fact that the <Navigator> renders the current route twice.

@sghiassy
Copy link

@laurilehmijoki - I think the problem has to do with the flexDirection and flexWrap. With those attributes, the layout engine goes into an infinite while loop under certain circumstants. The problem, I believe lies in Layout.c - although I haven't had more time to debug it.

@yelled3
Copy link
Contributor

yelled3 commented Aug 25, 2015

@spicyj god's work, my friend

BTW, is there a way to test this so it won't reproduce somehow?

@sophiebits
Copy link
Contributor

facebook/yoga@9001a3d2 has a test already.

sophiebits added a commit that referenced this issue Aug 26, 2015
Summary:
Manual pick of facebook/yoga@9001a3d2 since we're still lagging behind the upstream. Fixes #1378.
@ide
Copy link
Contributor

ide commented Aug 27, 2015

@spicyj's fix has been patched into 0.10 and 0.11-rc.

@yelled3
Copy link
Contributor

yelled3 commented Aug 27, 2015

amazing - thanks @ide @spicyj

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

No branches or pull requests