usePermission creates memory leaks

[kiroushi]

Describe the bug

Update:

All of the information below is incorrect. The culprit is not useClipboard but usePermission. Refer to #4149 (comment).

Seems like the effect callback in useSupported is preventing the component instances from being disposed and thus keeping all derived state and desdendant instances in memory.

Changing the helper to use a ref instead of a computed solves the issue, but I don't know if @antfu's intention here was to do a sole single mounted evaluation or to keep it fresh against the support (i.e. live permissions change).

export function useSupported(callback: () => unknown) {
  const isMounted = useMounted();

  const isSupported = ref(callback())

  watch(isMounted, () => {
    isSupported.value = callback()
  })

  return isSupported
}

I've added a small repo where the issue can be reproduced by navigating back and forth the component that uses the composable.

Video:

Recording.2024-08-14.105556.mp4

Reproduction

https://github.com/kiroushi/vueuse-clipboard-memory-leak

System Info

System:
    OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
    CPU: (16) x64 AMD Ryzen 7 7800X3D 8-Core Processor
    Memory: 8.31 GB / 15.22 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 18.19.0 - /run/user/1000/fnm_multishells/28791_1723623412408/bin/node
    Yarn: 1.22.22 - /run/user/1000/fnm_multishells/28791_1723623412408/bin/yarn
    npm: 10.2.3 - /run/user/1000/fnm_multishells/28791_1723623412408/bin/npm
    pnpm: 8.15.4 - ~/.local/share/pnpm/pnpm
    bun: 1.1.10 - ~/.bun/bin/bun
  npmPackages:
    @vueuse/core: ^10.11.1 => 10.11.1 
    vue: 2.7.16 => 2.7.16

Used Package Manager

pnpm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a VueUse issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[Onion-L]

I noticed you mentioned the issue about the effect callback in useSupported preventing component instances from being disposed, and I think maybe the computed here can ensure the return value is read-only.

[kiroushi]

@Onion-L I'm not sure that is the intended use; in fact, in the computed body isMounted.value is just referenced so the effect is part of the reactivity chain.

If the intention is to make the ref read only, I think the appropriate thing would be to return readonly(isSupported).

[Onion-L]

@kiroushi Yes, you are right. I ran the test and observed a few failures🤯. I'll see if I can fix them.

[Onion-L]

export function useSupported(callback: () => unknown) {
  const isMounted = useMounted()
  const isSupported = ref(false)

  watch(isMounted, () => {
    isSupported.value = Boolean(callback())
  }, { immediate: true })
  return readonly(isSupported)
}

I tried this, and it works and has passed all the tests. I notice that, for the first time, the isMounted value in computed makes no change, so I think the isMounted value can be ignored for the first time, so maybe we can trigger this effect immediately and watch the isMounted. I'll submit a PR😃

[ferferga]

@kiroushi Why you don't force the GC between runs? The effect may be not trivial to dispose and the GC may take more time to kick automatically (but we can force by doing a manual run).

I haven't tried your repo yet, so I'm just doing wild guesses as of now (I'm a bit busy these days but would like to help with this sometime next week).

CC as well #4154 (comment)

[Onion-L]

@ferferga Thank you for your comment. Yes, you are totally right; I didn't consider the garbage collection, and it will be great if you can take a look at this problem. I think I am a bit too hasty, and I am sorry for this.

[kiroushi]

@ferferga @Onion-L sorry for not providing the GC forcing step. The memory leak is there, definitely.

Recording.2024-08-16.161256.mp4

Edit: I think I've narrowed down the issue to another part of the implementation. At this point, I don't know if the GC is being prevented on useSupported or usePermission.

The pinpoint is definitely around the implementation of the latter in conjunction with the former around this line:

vueuse/packages/core/usePermission/index.ts

Line 69 in a9b49f5

const isSupported = useSupported(() => navigator && 'permissions' in navigator)

Changing that to just const isSupported = { value: true } gets rid of the memory leak.

Also in:

vueuse/packages/core/usePermission/index.ts

Line 83 in a9b49f5

if (!isSupported.value)

Removing the reference to isSupported.value in the "singleton promise" fabrication also removes the GC. (I'm not sure why a lingering promise is created there and never awaited).

There's a weird interaction between the computed effect in isMounted, the reference to useSupported there, and the reference to usePermission in the original composable (useClipboard) itself.

I don't have more time to investigate during the weekend but maybe this might shed some light to anyone trying to help with this.

[kiroushi]

Update: Actually I had some more time to investigate today and I think I've traced down the issue, finally.

The issue is not on useClipboard, but usePermission. Any composable derived from the use of it will have memory leaks, and this is because of the fact that the scope effects will never be disposed since the useEventListener is invoked in a closure context:

vueuse/packages/core/usePermission/index.ts

Lines 82 to 96 in a9b49f5

	const query = createSingletonPromise(async () => {
	if (!isSupported.value)
	return
	if (!permissionStatus) {
	try {
	permissionStatus = await navigator!.permissions.query(desc)
	useEventListener(permissionStatus, 'change', onChange)
	onChange()
	}
	catch {
	state.value = 'prompt'
	}
	}
	return permissionStatus
	})

.

Changing useEventListener to trace the scope at the time of the invocation in L88 will get undefined. No effect scope is present at the time of the invocation, and thus, tryOnScopeDispose will never be called, leaving retainers for the whole chain.

I will refactor the usePermission composable to avoid this, or if anyone prefers to take it, I'm good with it.

permission.mp4

[ferferga]

@Onion-L No problem! This is why reviews exist, because others might have noticed things that you haven't! It could have been the opposite: I might have been in the wrong and you already did the investigation I thought you didn't, just forgot to post about it!.

@kiroushi I saw your message yesterday and, after a quick glance at the code, I suspected about it since the beginning, since I also had problems with useEventListener in the past, but I was also wondering why the useSingletonPromise is needed, because I think that's more problematic itself, since useEventListener can be at the root level of the composable and permissionStatus can be a ref and passed to useEventListener directly, since it also accepts refs as parameters, updating the event handler accordingly.

Either case, what's more strange, is that I recreated your reproduction in the SFC Playground and I could not reproduce your problem. That makes me wonder if the leak is caused because you're doing the reproductions in development (+ HMR) and not in production?

Either way, I think the refactor I'm mentioning makes sense, I'll make a branch in my fork too with my proposed changes.

[ferferga]

@kiroushi Made the changes in this branch, please test them so, if they fix the issue, I can make a PR and get this going 🚀.

To test you can either copy the source code to your own project and import from there or build the package (using Codespaces is easiest) and replace the contents from dist in the node_modules of your project.

[kiroushi]

@ferferga thanks for your quick turnaround.

I'm not sure what could be the reason you cannot reproduce it, but certainly the leak is present in both development with HMR env and prod. build.

In any case, your solution seems to be working in an elegant way by getting rid of some of the complexity of the implementation. I am also unsure of the underlying need to use this "singleton promise", but in any case it escapes the purpose of this change.

In your branch, I would only suggest one small change, which is to remove the exogenous reference in onChange and make a direct assignation to state.value after assigning the PermissionStatus:

  useEventListener(permissionStatus, 'change', (e) => {
    state.value = (e.target as PermissionStatus)?.state ?? 'prompt'
  })

  const query = createSingletonPromise(async () => {
    if (!isSupported.value)
      return

    if (!permissionStatus.value) {
      try {
        permissionStatus.value = await navigator!.permissions.query(desc)
        state.value = permissionStatus.value.state
      }
      catch {
        state.value = 'prompt'
      }
    }

    if (controls)
      return toRaw(permissionStatus.value)
  })

Thanks for taking the time to help with this ;)

[ferferga]

@kiroushi I understood why after working in the branch: given further controls are exposed to the user (with the controls option), creating a promise once is necessary so multiple calls will be binded to the same Promise instance and resolved together.

I'll leave your proposed changes as the additional context of the PR so they're discussed in code review since I'm not too convinced of them because:

• I haven't used the Permissions API in depth, but I believe this might introduce a breaking change, since, in the current code, if permissionState.value is nullish, we don't do anything, while in your approach we set it to prompt.
• A bit nitpick, but it introduces the extra type casting, which makes it a bit more verbose
• Falls a bit of the original scope of the PR, which probably means that it will be splitted into a different PR anyway while delaying the shipment of this fix, which in my opinion is more critical to have landed than an small refactor whose only benefit is probably a smaller bundle.

PR opened at #4157

[kiroushi]

@ferferga sounds good. It is just a nitpick as well.

The reasoning behind falling back to prompt is the PermissionsStatus state property doesn't allow a nullish value at any state:

https://developer.mozilla.org/en-US/docs/Web/API/PermissionStatus/state

If we're falling back to prompt on API invocation failure (in the catch clause), then it would need to be the default fallback state as well.

But, as you said, it might be introducing a divergent implementation and might be breaking current uses, so no point on bundling it with the issue at hand.

[ferferga]

@kiroushi I opened #4167 with your proposed refactor (with a little bit of "my sauce" as well 😁), please take a look when you can, I'm waiting specially from your feedback!