facebookarchive / react-360

Create amazing 360 and VR content using React
https://facebook.github.io/react-360
Other
8.73k stars 1.23k forks source link

Changing Pano source crashes iOS #268

Closed ctoLarsson closed 7 years ago

ctoLarsson commented 7 years ago

Description

Changing pano source crashes both Safari and Chrome on iOS after a few times.

Seems it happens when going back to a previously loaded source. Do you by any chance miss checking if the browser garbage collector has snatched a previously loaded object before trying to render it? (Just a guess.)

It's simple to reproduce. Running 1.4.0.

Actual behavior

iOS Chrome: Aw, Snap! iOS Safari: A problem occured with this web page, so it has reloaded.

Reproduction

init new project cd static_asset eq2cm chess-world.jpg 64 c1cm eq2cm chess-world.jpg 2048 c2cm

Then just flipping between these two Pano sources 8-12 times crashes the viewer.

index.vr.js:

import React from 'react';
import {
  AppRegistry,
  asset,
  Pano,
  Text,
  View,
  VrButton,
} from 'react-vr';

const cm1 = [ asset('c1cm0.png'), asset('c1cm1.png'), asset('c1cm2.png'), asset('c1cm3.png'), asset('c1cm4.png'), asset('c1cm5.png') ];
const cm2 = [ asset('c2cm0.png'), asset('c2cm1.png'), asset('c2cm2.png'), asset('c2cm3.png'), asset('c2cm4.png'), asset('c2cm5.png') ];

export default class crashtest extends React.Component {
  constructor (props) {
    super(props);
    this.state =  {
      src: cm1,
      toggle: false,
    };
    this.toggleSrc = this.toggleSrc.bind(this);
  }

  toggleSrc(){
    if(this.state.toggle) {
      this.setState({src: cm1});
    } else {
      this.setState({src: cm2});
    }
    this.setState({toggle: !this.state.toggle});
  }

  render() {
    return (
      <View>
        <Pano source={this.state.src}></Pano>
        <VrButton onClick={()=>this.toggleSrc()}>
          <Text style={{
            backgroundColor: '#777879', fontSize: 0.4, fontWeight: "400",
            layoutOrigin: [0.5, 0.5], textAlign: 'center', textAlignVertical: 'center',
            transform: [{translate: [0, 0, -3]}],
          }}>
            Click me 15 times on iOS Chrome or Safari
          </Text>
        </VrButton>
      </View>
    );
  }
};

AppRegistry.registerComponent('crashtest', () => crashtest);

Solution

Haven't found any work-around. Happens sooner or later depending on total file sizes. Launch blocking.

Additional Information

mikearmstrong001 commented 7 years ago

Thanks for the report and the example, sounds like a memory limit is being hit

ctoLarsson commented 7 years ago

Thanks for looking into it. Some more info for you:

If keep loading new image resources, ReactVR doesn't crash - or at least I didn't experience it - only when going back to a previously viewed image resource, like the crash example above shows.

Speculating: It could be that after a few iterations, the browser garbage collector collects some resource which you still reference somewhere in the ReactVR core, so when going back to a previously viewed image component, React VR thinks is still there, but in fact the browser has cleared it, and the crash hits.

First I thought it was limited to cubemaps because it happens faster with cm, but it happens with the identical equirect format image also, just takes longer. Maybe because more files with cm, so statistically triggers the issue quicker (guessing).

The above example is trivial but this issue hits in more complex and hard-to-predict scenarios in our real app. Only blocking issue. Everything else is working fine now with 1.4.0.

(Many other javascript apps consume 10x more memory than the above example, so shouldn't hit any hard limits.)

Hope you can find the bug. Let me know if I can help in any way. Thanks!

ctoLarsson commented 7 years ago

Your recent tour example also crashes in same way after about 15 clicks on an iPhone, both in dev and in production mode. For clarity, ran the example with the released 1.4.0, not by building react-vr from source.

screenshot

mikearmstrong001 commented 7 years ago

@amberroy would you mind having a look at this given reference to the tour sample

ctoLarsson commented 7 years ago

Please note that the code I posted above (50 lines) is the entire code needed to consistently crash React VR in about 30 seconds. I stripped it down to aid your trouble shooting. I've only tested on the released 1.4.0. Thanks!

rupesh-ks commented 7 years ago

Hi guys, i am also working on a React vr app and i am experiencing the same issue. Apparently the app crashes on iPhone 6 and lower. (tested it with a iPhone 6 and iPhone 5s). iPhone 7 works just fine (also tested). It seems that in overal React vr is performing better on Android device (tested with OnePlus One, OnePlus 5, Nexus 5X). Removing video's/images from the app has no use, it's still not performing as it should..

ctoLarsson commented 7 years ago

Suggest delete the spam-bot posts. The last genuine post seems to be the one from rupesh-ks Too important issue to drown under spam :-)

MartinSherburn commented 7 years ago

I fixed a memory leak in the Pano component the other day. Please pull commit 8323673 and see if it fixes the problem for you.

ctoLarsson commented 7 years ago

YES WORKS!!!!!

Doesn't crash with the above replication code any more. Well done!

I will keep testing some more complex scenarios today. Was the same memory leak anywhere else, like in the VideoPano?

With this problem fixed, my last launch blocking issue is solved. Stay tuned :-)

MartinSherburn commented 7 years ago

Glad to hear it @andelar, I believe it was only affecting the Pano and not VideoPano. I'm going to close the issue for now. Thanks.

rupesh-ks commented 7 years ago

Hi Martin, Unfortunately it does not seem to work for me. I am using the new Pano.js, but i get an error that the following: Prefetch.isCached it not a function and get a black screen. I updated the Pano.js from \node_modules\react-vr\Libraries\Pano. Did i miss something? I hope you can help me out, it's probable something small.. Thanks

MartinSherburn commented 7 years ago

If you just updated that single file (Pano.js) then that's almost certainly the problem. You will need to get a consistent set of files, i.e. you will need to download the entire repository and use all of the contents. If you mix and match different versions these kinds of errors are inevitable.

rupesh-ks commented 7 years ago

Hi Martin, maybe a silly question but how do i download the entire repo and update my local files with the new ones. And which repo do i need to download, is that the following? react-vr/ReactVR/js/. Can i use the react-vr/package.json file to update everything? Thanks in advance!

MartinSherburn commented 7 years ago

Have a look at this issue: https://github.com/facebook/react-vr/pull/256 Instructions on how to do this are in progress.