Element resize listening via <iframe> breaks on destruction

[tivac]

Describe the bug
We run in an embedded browser that uses an older version of WebKit and have run into a bug in 3.21.0, likely due to #2989 which fixed #2147. When destroying elements that use bind:width, bind:height, etc we get a JS error in the returned unsubscription function from listen().

svelte/src/runtime/internal/dom.ts

Lines 60 to 63 in b336442

	export function listen(node: EventTarget, event: string, handler: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions | EventListenerOptions) {
	node.addEventListener(event, handler, options);
	return () => node.removeEventListener(event, handler, options);
	}

It can't call node.removeEventListener because the iframe.contentWindow used to set up the listener previously has already been set to null by the <iframe> being detached from the DOM.

svelte/src/runtime/internal/dom.ts

Line 283 in b336442

unsubscribe = listen(iframe.contentWindow, 'resize', fn);

Logs

TypeError: e.removeEventListener is not a function.

To Reproduce

{#if show}
<div bind:offsetHeight={height} style="height: 500px;" />
{/if}

<script>
let show = true;
let height;

window.hide = () => show = false;
</script>

Running hide() will trigger the error, but obviously only in older versions of webkit like the one we use

Expected behavior
The element would clean itself up and hide without errors.

Information about your Svelte project:

• Custom embedded WebKit version
• Windows 10
• 3.21.0
• We use rollup

Severity
This blocks us from using 3.21.0, which we need to fix all of the outro issues.

Additional context

There's a relatively small fix which requires shuffling around the detachment/unsubscription order a bit.

1. Move the destruction of the generated <iframe> before the destruction of its parent

svelte/src/compiler/compile/render_dom/wrappers/Element/index.ts

Lines 588 to 590 in b336442

	block.chunks.destroy.push(
	b`${resize_listener}();`
	);

-block.chunks.destroy.push(
+block.chunks.destroy.unshift(
     b`${resize_listener}();`
 );

2. Move the unsubscription of the listener on the <iframe> to before it is detached from the DOM.

svelte/src/runtime/internal/dom.ts

Lines 289 to 292 in b336442

	return () => {
	detach(iframe);
	if (unsubscribe) unsubscribe();
	};

 return () => {
-    detach(iframe);
     if (unsubscribe) unsubscribe();
+    detach(iframe);
 };

Since they originally authored #2989 I'm gonna ping @mrkishi here to see if they've got any ideas on the safety of these proposed changes. I'll start getting a PR together shortly.

[tivac]

The proposed fix isn't complete because I'm seeing the component get destroyed after the DOM that contains it is torn down, so it still errors out trying to detach the event listener from a iframe.contentWindow that is null. I'll need to keep digging to better understand how the d blocks are created for each component and the implications of changing the order there.

[tivac]

Huh. So it seems like svelte tears down the DOM for a component in order from top-to-bottom, including components. It won't add a detach() call for nested elements (makes sense), but it will tear down the containing element before destroying the component it contains.

https://svelte.dev/repl/dde83e1f676443bba7038a5bf0296326?version=3.21.0

d(detaching) {
    if (detaching) detach(a);
	destroy_component(child);
}

That seems wrong and counter-intuitive to me. I'd expect the component to be destroyed before it's containing element like this.

 d(detaching) {
+  	 destroy_component(child);
     if (detaching) detach(a);
-  	 destroy_component(child);
 }

If I swap them around my trivial repros/examples work fine, can anyone think of a reason why that would be a terrible idea? Worth noting that I haven't been able to figure out a clean way to do this in the compiler yet, that'll come next I guess.

[tivac]

I hardcoded our copy of add_resize_listener() to make it so that is_crossorigin() is always true and that codepath works fine and doesn't show the issue since it's subscribing/unsubscribing to window instead of iframe.contentWindow.

@mrkishi is there a reason we need to support two separate paths here? Could we just use the message-passing version in all cases or is there a downside to that flow that I'm unaware of?

[tivac]

svelte/src/runtime/internal/dom.ts

Lines 257 to 293 in ff5f252

	export function add_resize_listener(node: HTMLElement, fn: () => void) {
	const computed_style = getComputedStyle(node);
	const z_index = (parseInt(computed_style.zIndex) || 0) - 1;
	if (computed_style.position === 'static') {
	node.style.position = 'relative';
	}
	const iframe = element('iframe');
	iframe.setAttribute('style',
	`display: block; position: absolute; top: 0; left: 0; width: 100%; height: 100%; ` +
	`overflow: hidden; border: 0; opacity: 0; pointer-events: none; z-index: ${z_index};`
	);
	iframe.setAttribute('aria-hidden', 'true');
	iframe.tabIndex = -1;
	let unsubscribe: () => void;
	if (is_crossorigin()) {
	iframe.src = `data:text/html,<script>onresize=function(){parent.postMessage(0,'*')}</script>`;
	unsubscribe = listen(window, 'message', (event: MessageEvent) => {
	if (event.source === iframe.contentWindow) fn();
	});
	} else {
	iframe.src = 'about:blank';
	iframe.onload = () => {
	unsubscribe = listen(iframe.contentWindow, 'resize', fn);
	};
	}
	append(node, iframe);
	return () => {
	detach(iframe);
	if (unsubscribe) unsubscribe();
	};
	}

Something kinda like this?

 	if (computed_style.position === 'static') {
 		node.style.position = 'relative';
 	}
 
 	const iframe = element('iframe');
+    iframe.src = `data:text/html,<script>onresize=function(){parent.postMessage(0,'*')}</script>`;
 	iframe.setAttribute('style',
 		`display: block; position: absolute; top: 0; left: 0; width: 100%; height: 100%; ` +
 		`overflow: hidden; border: 0; opacity: 0; pointer-events: none; z-index: ${z_index};`
 	);
 	iframe.setAttribute('aria-hidden', 'true');
 	iframe.tabIndex = -1;
 
-	let unsubscribe: () => void;
-
-	if (is_crossorigin()) {
-		iframe.src = `data:text/html,<script>onresize=function(){parent.postMessage(0,'*')}</script>`;
-		unsubscribe = listen(window, 'message', (event: MessageEvent) => {
-			if (event.source === iframe.contentWindow) fn();
-		});
-	} else {
-		iframe.src = 'about:blank';
-		iframe.onload = () => {
-			unsubscribe = listen(iframe.contentWindow, 'resize', fn);
-		};
-	}
-
-	append(node, iframe);
+	const unsubscribe = listen(window, 'message', (event: MessageEvent) => {
+        if (event.source === iframe.contentWindow) fn();
+    });
+	
+    append(node, iframe);
 
 	return () => {
 		detach(iframe);
-		if (unsubscribe) unsubscribe();
+		unsubscribe();
 	};
 }

[mrkishi]

The only reason for the check was to do less stuff when possible. At this point, what about just checking for iframe.contentWindow before unsubscribing? It'd be garbage-collected anyway.

[tivac]

I wasn't able to determine conclusively if that would lead to leaking the event handler. My knowledge of when event handlers get leaked is apparently a bit rusty 😕

If you're confident it's safe I'm totally on-board and can have a PR up shortly!

[tivac]

@@ -11,15 +11,17 @@
 		`display: block; position: absolute; top: 0; left: 0; width: 100%; height: 100%; ` +
 		`overflow: hidden; border: 0; opacity: 0; pointer-events: none; z-index: ${z_index};`
 	);
 	iframe.setAttribute('aria-hidden', 'true');
 	iframe.tabIndex = -1;
 
+	const crossorigin = is_crossorigin();
+
 	let unsubscribe: () => void;
 
-	if (is_crossorigin()) {
+	if (crossorigin) {
 		iframe.src = `data:text/html,<script>onresize=function(){parent.postMessage(0,'*')}</script>`;
 		unsubscribe = listen(window, 'message', (event: MessageEvent) => {
 			if (event.source === iframe.contentWindow) fn();
 		});
 	} else {
 		iframe.src = 'about:blank';
@@ -28,10 +30,15 @@
 		};
 	}
 
 	append(node, iframe);
 
 	return () => {
-		if (unsubscribe) unsubscribe();
+		if(crossorigin) {
+			unsubscribe();
+		} else if(unsubscribe && iframe.contentWindow) {
+			unsubscribe()
+		}
+		
 		detach(iframe);
 	};
 }

Seem reasonable @mrkishi? Dunno if that's gonna run afoul of TS, I've only tried it in the generated index.mjs file so far but it solves my particular issue and doesn't strike me as tremendously scary.

[tivac]

Tried it, passes all the svelte tests so I assume TS is ok with it 😅

[Conduitry]

Your workaround has been released in 3.23.0, thank you!