Call to destroy() with invalid argument causes error in runtime

[FlyingDR]

Describe the bug

I've started receiving reports from Bugsnag for my application:

TypeError: Cannot read properties of null (reading 'removeChild')

Application is built using SvelteKit, but the issue is in Svelte.

The issue seems to happen immediately after navigation between application's pages. goto() with replaceState: true is used. From my understanding (which I can't prove) at this moment Svelte cleans up DOM from the previous page's nodes in order to start rendering the new page.

Stack traces from the reports clearly points to the detach() function:

svelte/src/runtime/internal/dom.ts

Lines 200 to 202 in ed078e3

	export function detach(node: Node) {
	node.parentNode.removeChild(node);
	}

From the code of the function and the text of the error, it really looks like in some cases some wrong value is being passed to the destroy(). While the root cause of the issue is unknown to me - it seems quite obvious that the error in this particular method can be fixed by simple check that node.parentNode is actually available:

export function detach(node: Node) {
	if (node.parentNode) {
		node.parentNode.removeChild(node);
	}
}

This check was already available in this method in the past as part of #6204, but was reverted by #6290.

The issue was also reported in #6313, but was closed.

Reproduction

I was unable to reproduce the issue by myself yet. The information given above came from Bugsnag reports and my own analysis of Svelte sources.

Logs

Here is the relevant part of the stacktrace:

node_modules/svelte/internal/index.mjs:373:20 At
    }
}

function detach(node) {
    node.parentNode.removeChild(node);
}

function destroy_each(iterations, detaching) {
    for (let i = 0; i < iterations.length; i += 1) {

src/routes/s/[id]/index.svelte:174:129 Object.d
        <Row class="gx-2">
            <Col xs="12" sm="auto" class="mt-1">
                <Button color="secondary" outline=true size="sm" on:click={modals.reasons.toggle}>Why do we asking for it?</Button>
            </Col>
            {#if subscription.status === 'new'}
                <Col xs="12" sm="auto" class="mt-1">

node_modules/sveltestrap/src/Button.svelte:62:45 Object.d
    bind:this={inner}
    on:click
    {value}
    aria-label={ariaLabel || defaultAriaLabel}
    {style}
  >
    <slot>

node_modules/sveltestrap/src/Button.svelte:37:9 Object.d
  $: defaultAriaLabel = close ? 'Close' : null;
</script>

{#if href}
  <a
    {...$$restProps}
    class={classes}

node_modules/svelte/internal/index.mjs:1849:35 Lt
    const $$ = component.$$;
    if ($$.fragment !== null) {
        run_all($$.on_destroy);
        $$.fragment && $$.fragment.d(detaching);
        // TODO null out other refs, including component.$$ (but need to
        // preserve final state?)
        $$.on_destroy = $$.fragment = null;

System Info

For now, I have reports about 10 incidents, all of them from Chrome on Android. Chrome versions vary from 80 to 104.

Svelte v3.49.0 is used in the application.

Severity

annoyance

[brunnerh]

@Conduitry
Reproduction:

<script>
	import Child from './Child.svelte';
	let checked = true;
</script>

<label>
	<input type=checkbox bind:checked />
	Show child
</label>

{#if checked}<Child />{/if}

<!-- Child.svelte -->
<script>
	import { onMount } from 'svelte';
	let node;
	onMount(() => node.remove());
</script>

Child component

<div bind:this={node}>
	Detached node
</div>

REPL

Uncheck the checkbox to cause the child component to be destroyed.
The child has a detached element which will cause the error.

Related or duplicate issues:

• Cannot properly destroy component after changing slotted node #5879
• Error on detach when Node created by subcomponent removed using dom api #3178

[brunnerh]

Aside from checking node.parentNode, it might also be possible to just use remove()?
(I do not know what the API support standards are for the Svelte runtime.)

export function detach(node: ChildNode) {
	node.remove();
}

Most if not all relevant APIs should return ChildNode instances and remove() is safe to be called on already detached nodes.

[brunnerh]

Just checked out the code and noticed that the parentNode check is now back via #6910

[dummdidumm]

Closing as there's no reproduction that isn't a "I removed this manually which I shouldn't have", and #6910 should silence the symptom.

[brunnerh]

@dummdidumm
Rendering content with Svelte and then processing/integrating it with other libraries should be a valid use case that I ran into when content was paginated by a data table library, i.e. some table rows had been detached which caused this error.

[FlyingDR]

@dummdidumm As an issue author, I can confirm that in my case there was no third-party library involved.

In my case, the issue occurs during page navigation in SvelteKit application. While it seems to not cause any significant issues in my case - the fact of its existence is here and the code itself has no guarantee that it will work properly in runtime since it has two levels of chaining.

It is also possible to avoid the issue by using optional chaining (which already has quite a good support level):

export function detach(node: Node) { 
 	node?.parentNode?.removeChild(node); 
} 

To be true I can't understand the reasons for not including this trivial update which may improve code stability. There may be some performance reasons of course, but I'm not sure if there will be any measurable impact.