exokitxr / exokit

Native VR/AR/XR engine for JavaScript 🦖
MIT License
997 stars 117 forks source link

Fails to load janusweb #950

Open madjin opened 5 years ago

madjin commented 5 years ago

Describe the bug What happened, and what did you expect to happen? Run node --experimental-worker . https://web.janusvr.com in terminal and it halts at a certain point after TypeError: window.customElements.upgrade is not a function

To Reproduce Steps or commands ran to reproduce the behavior:

  1. Go to 'terminal (MINGW64)'
  2. Type 'node --experimental-worker . https://web.janusvr.com'
  3. Scroll down to 'bottom'
  4. See error

Additional context Add any other context about the problem here.

System information:

Screenshots If applicable, add screenshots to help explain your problem. output log: https://pastebin.com/REqVrbp6

avaer commented 5 years ago

Thanks, looks like we need to implement https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/upgrade.

The place to implement it: https://github.com/exokitxr/exokit/blob/d38467d6575b0d2d03c46e8f4e5df890c23238bb/src/Window.js#L147

jbaicoianu commented 5 years ago

Hmm, JanusWeb does include a polyfill which should be setting up the window.customElements object if it's not defined by the environment. I'm not sure if the polyfill works when running under nodejs though.

This is the polyfill library we're using, if it helps for debugging https://github.com/WebReflection/document-register-element

avaer commented 5 years ago

Yeah, I've run into sites using that before but it had major timing problems in A-Frame. Exokit does implement most of that polyfill natively itself -- we're just missing this particular function.

jbaicoianu commented 5 years ago

We're not using it quite as extensively as A-Frame does - in A-Frame, all entities are custom elements, whereas we only use custom elements to define a number of custom viewer tags, eg, <janus-viewer> <janus-viewer-video360 src="...">, etc. I know they had some performance issues with keeping the DOM in sync every time the 3d engine changed attributes, but we're not using custom elements in the same way.

avaer commented 5 years ago

The problem seems to be that window.customElements is hijacked and rewritten by the site to not support .upgrade, despite the fact that we nominally support it.

I'm guessing the polyfill is being confused, kicking in, and downgrading our functionality, then throwing.

avaer commented 5 years ago

This bug is because the document-register-element polyfill decides it can't be kicked in.

The way it checks for this is it defines a custom HTMLAnchorElement, and then it forces a construction of that element via:

e = function() {};
e.prototype = Object.create(HTMLAnchorElement.prototype);
new e();

and then it checks if it's been customized. Exokit doesn't handle this weird case, so attempts to use Exokit's implementation are abandoned and instead the polyfill uses its nonfunctional version, which later causes a throw in Janus Web.

avaer commented 5 years ago

We are now at the point of bootstrapping Janus's Web Workers, post asset loading.

avaer commented 5 years ago

With https://github.com/exokitxr/exokit/pull/960, we are bypassing native workers functionality.

However, now I'm hitting weird Proxy implementation errors coming from Janus:

Janus room parse error: TypeError: 'getOwnPropertyDescriptor' on proxy: trap returned descriptor for property 'playing' that is incompatible with the existing property in the proxy target
    at Function.keys (<anonymous>)
    at getKeys (internal/util/inspect.js:386:21)
    at formatRaw (internal/util/inspect.js:614:12)
    at formatValue (internal/util/inspect.js:545:10)
    at formatProperty (internal/util/inspect.js:1126:11)
    at formatRaw (internal/util/inspect.js:760:11)
    at formatValue (internal/util/inspect.js:545:10)
    at formatProperty (internal/util/inspect.js:1126:11)
    at formatRaw (internal/util/inspect.js:760:11)
    at formatValue (internal/util/inspect.js:545:10)
    at formatProperty (internal/util/inspect.js:1126:11)
    at formatRaw (internal/util/inspect.js:760:11)
    at formatValue (internal/util/inspect.js:545:10)
    at inspect (internal/util/inspect.js:194:10)
    at Object.formatWithOptions (util.js:180:18)
    at Console.(anonymous function) (console.js:199:15)
    at Console.log (console.js:210:31)
    at elation.janusweb.multiplayermanager.registerRoom (file://C:\Users\avaer\Documents\GitHub\exokit2\janusweb.js:105252:9)
    at Object.fireEvent (file://C:\Users\avaer\Documents\GitHub\exokit2\janusweb.js:3106:22)

I've traced it down to being caused by engine.things.sound:

Object.defineProperty(this, 'playing', { get: function() { if (this.audio) return this.audio.isPlaying; return false; } });

It should be reproable on https://github.com/exokitxr/exokit/pull/960 with:

node --experimental-worker . https://web.janusvr.com

@jbaicoianu do you have any idea why this would be errant?

jbaicoianu commented 5 years ago

@modulesio looks like that exception is thrown during normal use, so it isn't exokit-specific. I'm doing a pass to clean up some unhandled exceptions now, so I'll see what I can do to fix that one while I'm at it.

Is Exokit bailing on the first unhandled exception?

avaer commented 5 years ago

Thanks the explanation.

Is Exokit bailing on the first unhandled exception?

It depends, but it might be following node default behavior here in which case it could grind things to a halt. Worth checking that with a try-catch.