Skip to content

Commit 176aed6

Browse files
Fix a-assets image onload crash from loop-variable closure (#5809)
The image onload handler closed over the shared outer loop variables, so by the time it fired imgEls[i] was undefined (or a stale element already removed by fixUpMediaElement), causing "Cannot read properties of undefined (reading 'getAttribute')". Bind the current img to a local inside the Promise executor.
1 parent dd6709c commit 176aed6

2 files changed

Lines changed: 36 additions & 5 deletions

File tree

src/core/a-assets.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,18 +57,21 @@ class AAssets extends ANode {
5757
for (i = 0; i < imgEls.length; i++) {
5858
imgEl = fixUpMediaElement(imgEls[i]);
5959
loaded.push(new Promise(function (resolve, reject) {
60+
// Bind the current img to a local so onload below does not close
61+
// over the shared outer loop variable.
62+
var el = imgEl;
6063
// Set in cache because we won't be needing to call three.js loader if we have.
6164
// a loaded media element.
62-
if (imgEl.complete) {
63-
THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
65+
if (el.complete) {
66+
THREE.Cache.add('image:' + el.getAttribute('src'), el);
6467
resolve();
6568
return;
6669
}
67-
imgEl.onload = function () {
68-
THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
70+
el.onload = function () {
71+
THREE.Cache.add('image:' + el.getAttribute('src'), el);
6972
resolve();
7073
};
71-
imgEl.onerror = reject;
74+
el.onerror = reject;
7275
}));
7376
}
7477

tests/core/a-assets.test.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,34 @@ suite('a-assets', function () {
8888
document.body.appendChild(sceneEl);
8989
});
9090

91+
test('caches image loaded asynchronously alongside media element', function (done) {
92+
var assetsEl = this.el;
93+
var sceneEl = this.scene;
94+
95+
// Use a cache-busted src so the browser has to actually fetch, making
96+
// imgEl.complete false when the Promise executor runs and forcing the
97+
// onload path to be exercised.
98+
var src = IMG_SRC + '?async-load-test';
99+
THREE.Cache.remove('image:' + src);
100+
101+
var img = document.createElement('img');
102+
img.setAttribute('src', src);
103+
assetsEl.appendChild(img);
104+
105+
// A sibling media element ensures the image onload callback is
106+
// verified alongside a non-trivial asset list.
107+
var audio = document.createElement('audio');
108+
audio.setAttribute('src', '');
109+
assetsEl.appendChild(audio);
110+
111+
sceneEl.addEventListener('loaded', function () {
112+
assert.equal(THREE.Cache.get('image:' + src), img);
113+
done();
114+
});
115+
116+
document.body.appendChild(sceneEl);
117+
});
118+
91119
test('does not wait for media element without preload attribute', function (done) {
92120
var el = this.el;
93121
var scene = this.scene;

0 commit comments

Comments
 (0)