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

Avoid adding props to global function prototype. Doing so breaks the react-three-fiber reconciler #9207

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

marsy-invests
Copy link

Closes #9206

Change List

  • create-props.ts Changed createPropsPrototypeAndTypes to bail out if the parent class of the passed component class is the global function prototype so that it doesn't get polluted.

There may well be a better and cleaner way to do this, but this fixes the bug I'm experiencing.

Note that I was not able to get tests to run locally or on my fork via Github Actions, but the errors appear unrelated to my changes and seem to be occurring on some workflow runs on the main repo.

P.S. deck.gl is an absolutely amazing piece of software!

…react-three-fiber reconciler and prevents it from detecting when event handlers have changed and need to be updated in the underlying three.js objects.
@marsy-invests marsy-invests changed the title Avoid adding props to global function prototype. Doing so breaks the … Avoid adding props to global function prototype. Doing so breaks the react-three-fiber reconciler Oct 10, 2024
@felixpalmer
Copy link
Collaborator

There do seem to be some relevant failing tests, does yarn test browser not work for you locally?

@marsy-invests
Copy link
Author

marsy-invests commented Oct 10, 2024

yarn test browser does indeed work locally - thanks. I can see there's only one failing test if I roll back my change vs 25 failing tests with my change. I'll look further into them to see if I can understand the issue.

The failing tests I saw on the CI build related to missing image files. If I run yarn test locally, I get

file:///redacted/deck.gl/examples/layer-browser/src/data-samples.js:6
import allPoints from '../data/sf.bike.parking.json' assert { type: 'json' };
                                                     ^^^^^^

SyntaxError: Unexpected identifier 'assert'
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:166:18)
    at callTranslator (node:internal/modules/esm/loader:437:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:443:30)

Node.js v22.9.0
error Command failed with exit code 1.

@marsy-invests
Copy link
Author

Playing with this a bit further, It's starting to look like deck.gl requires attaching props to the global function prototype in order for the layer tests to pass - something that is incompatible with the way the react-three-fiber reconciler works. I need to spend more time familiarising myself with the deck.gl code and what role props play.

@ibgreen
Copy link
Collaborator

ibgreen commented Oct 10, 2024

@marsy-invests Can you pinpoint in the debugger when the function prototype is polluted? Perhaps add a debugger statement to your if statement and see where it breaks and follow the stack trace?

@marsy-invests
Copy link
Author

@ibgreen if I change getPropsPrototype (see https://github.com/visgl/deck.gl/blob/master/modules/core/src/lifecycle/create-props.ts#L82) to throw an error when about to pollute the function prototype as follows:

function getPropsPrototype(componentClass, extensions) {
    // A string that uniquely identifies the extensions involved
    let cacheKey = MergedDefaultPropsCacheKey;
    if (extensions) {
        for (const extension of extensions) {
            const ExtensionClass = extension.constructor;
            if (ExtensionClass) {
                cacheKey += `:${ExtensionClass.extensionName || ExtensionClass.name}`;
            }
        }
    }
    const defaultProps = getOwnProperty(componentClass, cacheKey);
    if (!defaultProps) {
        if (componentClass === Function.prototype) {
            throw Error("About to pollute function prototype", componentClass)
        }
        return (componentClass[cacheKey] = createPropsPrototypeAndTypes(componentClass, extensions || []));
    }
    return defaultProps;
}

I get the following stack trace

ErrorBoundary.tsx:29 Uncaught error: Error: About to pollute function prototype
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34413:13)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createProps (chunk-WVBUUDCH.js?v=94ea3cd1:34385:26)
    at new Component (chunk-WVBUUDCH.js?v=94ea3cd1:34543:18)
    at new Layer (chunk-WVBUUDCH.js?v=94ea3cd1:34918:5) {componentStack: '\n    at Image (http://localhost:5173/src/ui/le…(http://localhost:5173/src/ErrorBoundary.tsx:6:1)'} 

@marsy-invests
Copy link
Author

marsy-invests commented Oct 10, 2024

And if I add console.log("Calling getPropsPrototype for", componentClass) to the start of getPropsPrototype, here's the logs that are created as it walks the prototype chain as the layer gets created.

Calling getPropsPrototype for class extends CompositeLayer {
  static {
    this.layerName = "GeoJsonLayer";
  }
  static {
    this.defaultProps = defaultProps15;
  }
  initializeState() {
    this.state = {
      layerProps: {},
    …
Calling getPropsPrototype for class extends Layer {
  static {
    this.layerName = "CompositeLayer";
  }
  /** `true` if this layer renders other layers */
  get isComposite() {
    return true;
  }
  /** Returns true if all async res…
Calling getPropsPrototype for class extends Component {
  constructor() {
    super(...arguments);
    this.internalState = null;
    this.lifecycle = LIFECYCLE.NO_STATE;
    this.parent = null;
  }
  static {
    this.defaultProps = d…
Calling getPropsPrototype for class {
  static {
    this.componentName = "Component";
  }
  static {
    this.defaultProps = {};
  }
  constructor(...propObjects) {
    this.props = createProps(this, propObjects);
    this.id = this.p…
Calling getPropsPrototype for ƒ () { [native code] }
chunk-NUM45GRF.js?v=586f8a51:16638 Uncaught Error: About to pollute function prototype
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34414:13)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createProps (chunk-SL5OX4HQ.js?v=586f8a51:34385:26)

@marsy-invests
Copy link
Author

Revised the fix. It now passes the test suite. getPropsPrototype now just returns an empty object if the componentClass passed is not actually a Component.

@coveralls
Copy link

Coverage Status

coverage: 91.639% (-0.004%) from 91.643%
when pulling e846166 on marsy-invests:master
into 065816e on visgl:master.

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.

[Bug] Deck.gl breaks the React Three Fiber reconciler due to function prototype pollution
5 participants