cloneNode on image element is not cloning some properties (e.g., width and height)

[viniciusd]

Basic info:

• Node.js version: v8.11.1
• jsdom version: v12.0.0

Minimal reproduction case

const { JSDOM } = require("jsdom");

const options = {
  resources: 'usable'
};
const dom = new JSDOM('', options);

const img = new dom.window.Image();
// Note it requires canvas support (canvas or canvas-prebuilt)
img.onload = function() {
    const newImg = this.cloneNode(true);
    console.log(this.width, this.height);
    console.log(newImg.width, newImg.height);
}
img.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

Running this code outputs:

16 16
0 0

How does similar code behave in browsers?

Tested on Google Chrome 67.0.3396.99 and 69.0.3497.81.
Also tested on Firefox Nightly, all presenting the same behaviour.

16 16
16 16

[viniciusd]

Took the image data from #1816.
I haven't debugged it throughout, it might relate to #1142. I intend to debug it further this week.

[viniciusd]

I noticed it successfully copies either width or height if I set it explicitly:

img.width = 1;
img.src = '...';

It makes sense given the way cloneNode works: it creates an Image object and then copies all attributes that were explicitly set to the new node.

I traced this issue to height -> naturalHeight. It is worth noticing the following comment in the height getter:

// Just like on browsers, if no width / height is defined, we fall back on the
// dimensions of the internal image data.

Which falls back to the naturalHeight getter, which returns this._image.height. However, this._image.height is 0.
It is also worth noting that this._image is not inspectable in the browser (dev tools), it triggers an error in node(Js). It might relate to Automattic/node-canvas#855
For now, I believe that it is:

1. either a problem in the loading, which does not set values properly
2. or a problem in the cloneNode function, which does not copy the inner image data (this._image)

I am more inclined to (2). I understand image support is not a traditional usage of jsdom and it might have those edge cases (if I may call it so) waiting to be discovered.

[viniciusd]

It does seem to be a problem with setting the source of a new element equals to the one used previously, which would make me believe it is due to some cache (either intended or unintended).
However, it only happens if I am using cloneNode, but it does not seem to have any special call. I also tested using two different image objects with the same source, could not reproduce the issue like that.

Using cloneNode:

const { JSDOM } = require("jsdom");
const options = {
  resources: 'usable'
};
const dom = new JSDOM('', options);

const impl = require('./node_modules/jsdom/lib/jsdom/living/generated/utils.js').implSymbol;

const img = new dom.window.Image();
img.onload = function() {
    const newImg = this.cloneNode(true);
    console.log(this[impl]._image);
    console.log(newImg[impl]._image);
}
img.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

Outputs:
[Image:16x16 complete]
[Image]

Using two different image objects with the same src:

const { JSDOM } = require("jsdom");
const options = {
  resources: 'usable'
};
const dom = new JSDOM('', options);

const impl = require('./node_modules/jsdom/lib/jsdom/living/generated/utils.js').implSymbol;

const img = new dom.window.Image();
const img2 = new dom.window.Image();
img.onload = function() {
    console.log(this[impl]._image);
    img2.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';
}
img2.onload = function() {
    console.log(this[impl]._image);
}
img.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

Outputs:
[Image:16x16 complete]
[Image:16x16 complete]

Note: It happens disregards the Canvas backend used (canvas or canvas-prebuilt).

[viniciusd]

Found it: This issue happens because cloneNode is not fully synchronous for images, given their load that is asynchronous.
The ResourceLoader is promise-based and promises will not run their then until the call stack is empty. HTMLImageElementImpl's _attrModified only sets the _image.source value to the data from the request in a callback that is triggered on promises' resolution.

Running the following code proved it:

const { JSDOM } = require("jsdom");
const options = {
  resources: 'usable'
};
const dom = new JSDOM('', options);

const impl = require('./node_modules/jsdom/lib/jsdom/living/generated/utils.js').implSymbol;

const img = new dom.window.Image();
img.onload = function() {
    console.log(this[impl]._image);
    const newImg = this.cloneNode(true);
    newImg.onload = () => console.log(newImg[impl]._image);
    setTimeout(() => console.log(newImg[impl]._image), 100);

}
img.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

Which outputs:
[Image:16x16 complete]
[Image:16x16 complete]
[Image:16x16 complete]

Proving cloneNode works fully synchronous on Chrome is not hard:

const img = new window.Image();
// Note it requires canvas support (canvas or canvas-prebuilt)
img.onload = function() {
    const newImg = this.cloneNode(true);
    console.log(this.width, this.height);
    console.log(newImg.width, newImg.height);
    while(1);
}
img.src = 'data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7';

Still displays the expected values. Gonna read the specs to check if it is somehow well defined there.

[viniciusd]

As discussed in this Chromium issue, cloneNode should be synchronous. However, they cite the W3C specs and I could not find where the documentation defines it as synchronous.
https://bugs.chromium.org/p/chromium/issues/detail?id=795620