BabylonJS / Spector.js

Explore and Troubleshoot your WebGL scenes with ease.
http://spector.babylonjs.com
MIT License
1.3k stars 169 forks source link

Spector uses hooked `getContext` on an unmarked / non-identifiable `<canvas>` which the hook owner can't recognize #275

Closed JannikGM closed 1 year ago

JannikGM commented 1 year ago

There are some situations where Spector.js creates a canvas; here are some examples (other cases might exist):

https://github.com/BabylonJS/Spector.js/blob/35385a5c015dee6fcd232ba253ad98d3d502f5ea/src/backend/states/drawCalls/drawCallTextureInputState.ts#L31-L34

https://github.com/BabylonJS/Spector.js/blob/35385a5c015dee6fcd232ba253ad98d3d502f5ea/src/backend/states/context/visualState.ts#L29-L32

It then calls getContext on these.


That is a problem because our code wraps some third-party libraries to add a listener for webglcontextcreationerror before letting them use the real getContext. To ensure we instrument the correct canvas, we check the class-name of the canvas in question and throw an error if it's not what we'd expect (as the target library might have had internal changes).

However, because Spector also hooks getContext and then calls its own getContext on an unexpected canvas, this breaks our logic (it works as intended, but it's a false-positive). We'd filter out the Spector canvas from our hook explicitly, but it doesn't have any markings / identification.

Ideally, in the browser extension version, Spector would keep a copy of the canvas.prototype.getContext before the app even runs (which is definitely not hooked), so it can avoid these side-effects.

sebavan commented 1 year ago

You should filter out the "2d" contexts in this case ? or you could add an id to those canvases before calling getContext ? recording on load could always suffer from timing.

JannikGM commented 1 year ago

You should filter out the "2d" contexts in this case ?

Yes, not sure why I didn't think of that. I just swapped the check around wether it's a WebGL context and wether it's the expected canvas.

or you could add an id to those canvases before calling getContext ?

Not possible because we debug a library (mapbox in this case) which creates the element internally.


I see this is very niche, so feel free to close this. (I'll keep this open for now, as I could imagine other people running into similar problems).

sebavan commented 1 year ago

let s close it and if somebody else stumble upon the issue let s reopen and find a way :-)