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

[ThinEngine] Check if document is available before accessing it #12571

Conversation

antoine-gannat
Copy link
Contributor

Description

Hi,

In ThinEngine the getHostDocument method is not react-native "safe", when accessed from a react-native application, we try to access document without checking that it exists first, thus causing the app to crash.

Repro

To repro the issue, all you need is to create a FollowCamera and make it follow a target.
I got this issue to repro by using this code in an Expo environment:

 function initGame(gl: ExpoWebGLRenderingContext) {
  const engine = new Engine(gl, true, {}, false);

  const scene = new Scene(engine);
  // Our built-in 'box' shape. Params: name, size, scene
  const box = MeshBuilder.CreateBox("box", { size: 2 }, scene);

  new FollowCamera("ArcRotateCamera", new Vector3(2, 0, 0), scene, box);
  const light = new HemisphericLight("light1", new Vector3(0, 1, 0), scene);

  // Default intensity is 1. Let's dim the light a small amount
  light.intensity = 0.7;

  scene.registerBeforeRender(() => {
    if (this.updatePan) {
      (scene.activeCamera as any).alpha -= this._translateX / 6000;
      (scene.activeCamera as any).beta -= this._translateY / 6000;
    }
    if (this.updatePinch) {
      (scene.activeCamera as any).radius -= this._scale * 60;
    }

    gl.endFrameEXP();
  });

  engine.runRenderLoop(() => {
    if (scene) {
      scene.render();
    }
  });
}

Cause

What happens here is that at some point, the Effect class (packages/dev/core/src/Materials/effect.ts) will try to access the host document by only checking if IsWindowObjectExist returns true:

const hostDocument = IsWindowObjectExist() ? this._engine.getHostDocument() : null;

This issue is, in this react-native context window will exist but not document, leading to this issue.

Fix

To fix this, I added a check in getHostDocument to make sure that document actually exist before returning it. If it doesn't, I simply return null.

This is my first PR in this repo, please let me know if I forgot anything.

@antoine-gannat antoine-gannat changed the title Check if document is available before accessing it [ThinEngine] Check if document is available before accessing it May 24, 2022
@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

@deltakosh deltakosh merged commit a8a1d9c into BabylonJS:master May 24, 2022
@antoine-gannat antoine-gannat deleted the user/angannat/fix-thinengine-gethostdocument branch May 24, 2022 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants