$watch can register it's effects on the wrong instance

[tim-janik]

Version

3.0.0

Steps to reproduce

I'm in the process of porting a smallish front-end (10klocs) from Vue2 to Vue-3.0.0.
I have one mixin that installs a watch and uninstalls it like this (abbreviated):

  updated: function () {
      this.$dom_updates.unwatch1?.();
      this.$dom_updates.unwatch1 = null;
      const dom_update_reactive = vm => { ... };
      this.$dom_updates.unwatch1 = this.$watch (dom_update_reactive, this.$forceUpdate);
  },
  beforeUnmount: function () {
    this.$dom_updates.unwatch1?.();
    this.$dom_updates.unwatch1 = null;
  },

Calling unwatch1 always triggers the following:

vue.js:1180 [Vue warn]: Unhandled error during execution of beforeUnmount hook 
  at <BModaldialog key=1 value=false onInput=fn<onInput>  ... > 
  at <BApp>
vue.js:1180 [Vue warn]: Unhandled error during execution of scheduler flush. 
  This is likely a Vue internals bug. Please open an issue at 
  https://new-issue.vuejs.org/?repo=vuejs/vue-next 
  at <BApp>
vue.js:212 Uncaught (in promise) TypeError: Cannot read property 'indexOf' of null
    at remove (vue.js:212)
    at Object.unwatch1 (vue.js:6489)
    at Proxy.beforeUnmount (util.js:737)
    at callWithErrorHandling (vue.js:1296)
    at callWithAsyncErrorHandling (vue.js:1305)
    at Array.hook.__weh.hook.__weh (vue.js:3665)
    at invokeArrayFns (vue.js:275)
    at unmountComponent (vue.js:6157)
    at unmount (vue.js:6071)
    at patchKeyedChildren (vue.js:5892)
remove	@	vue.js:212
(anonymous)	@	vue.js:6489
beforeUnmount	@	util.js:737
callWithErrorHandling	@	vue.js:1296

It looks like Vue's unwatch callback as returned from $watch() cannot remove itself from instance.effects, since vue.js reads:

vue.js:211:const remove = (arr, el) => {
vue.js:212:    const i = arr.indexOf(el);
  ...
vue.js:6489:             remove(instance.effects, runner);`

Since I'm in the middle of a transition, it's impossible for me to deduce a minimal test case.
I hope the trace is useful regardless.

The full (unported) Vue2 based code can be found here, still using beforeDestroy instead of beforeUnmount:

https://github.com/tim-janik/beast/blob/7e743157617f74f842aabbe6efbff2184b0b1109/ebeast/util.js#L696

What is expected?

unwatch() as returned from $watch() can be called without exception.

What is actually happening?

An exception is thrown.

[LinusBorg]

watch effects created with $watch are cleaned up up on unmount automatically so I don't see the need to do that manually. Try leaving that call out.

so my first suspicion was that the effects simply were already cleanup up when your code is called by that's not the case, so something we should probably take a look at.

[posva]

I tried boiling it down but couldn't: https://jsfiddle.net/posva/gtmksj37/

Please open a new issue if you manage to provide a boiled down reproduction

[tim-janik]

watch effects created with $watch are cleaned up up on unmount automatically so I don't see the need to do that manually.
Try leaving that call out.

Are you serious?
How is the caller of unwatch supposed to know if instance.effects has been reset when it has to ensure that the watch callback must not be called anymore?
Consider resetting with instance.effects = [] instead of null if you have multiple sites that try to unregister/cleanup the watch list.

[LinusBorg]

Not sure If you misunderstood my intentions.

1. I'm saying the watch is removed for you.
2. Still it should not throw an error when called again, hence I said (we should still take a look)
3. We could not reproduce the behavior in a clean example. here's another jsfiddle:

https://jsfiddle.net/85geva61/

We can't debug your whole project with our ressources. If you want to make it easier for us to identify a potential issue, try stripping your code down until no other effects are in play to the best of your abilities.

[tim-janik]

While I don't have a miniscule repro yet, I did figure out what the bug is.

Please add the following assertion to vue.js and then try out some non-trivial apps that use $watch, in particular call this.$watch (implemented by function instanceWatch) with an instance that is not currentInstance:

--- ./node_modules/.vite_opt_cache/vue.orig
+++ ./node_modules/.vite_opt_cache/vue.js
@@ -3172,6 +3172,7 @@ function doWatch(source, cb, { immediate, deep, flush, onTrack, onTrigger } = EM
         onTrigger,
         scheduler
     });
+    console.assert (instance == currentInstance);
     recordInstanceBoundEffect(runner);
     // initial run
     if (cb) {

Here's why things break, recordInstanceBoundEffect() will always add the runner to currentInstance.
But remove() will always remove the runner from instance, so if the two mismatch, we may try to remove from an instance that still has a null effects list, which is the exception I originally reported.

The fix is obviously to change the call to recordInstanceBoundEffect() so the runner is added to instance instead.

Now, speculating, the current code tends to add runners to the wrong (current) instance whenever doWatch is called with an explicit instance that is not currentInstance. That behaviour can generally be expected to cause spurious rendering updates (a runner still active on the wrong instance after it should have been stopped), and cause missing updates other times (the runner was stopped too early during unmount of the wrong instance it was attached to).
So once this issue is fixed, it could be wise to recheck other open issues with repros for spurious or missing updates.

Unfortunately I cannot reopen this issue, so please do that for me.

[LinusBorg]

I'll reopen this so we can re-evaluate, thanks so far.

[LinusBorg]

Managed to reproduce it:

https://jsfiddle.net/Linusborg/4w8gjvph/6/

Still looking into the specifics of what happens and wether tim-janik's proposed fix is the right one, but as he explained, it's related to a mismatch of currentInstance and instance, the 4th argument of doWatch

As this.$watch is the only watch implementation making use of that 4th argument, the issue is limited to situations where a component's this.$watch is called while another component is in fact currentInstance. Other watch related APIs are safe, as is this.$watch being used in its "normal" context - within its own component's lifecycle.

Even more limiting - the issue only happens as long as the child itself doesn't have any instance.effects set.

So one way to fix the error being thrown is by checking for the existance of effects here:

https://github.com/vuejs/vue-next/blob/9b34f915ab3211b099f71472e450a39b0d149b57/packages/runtime-core/src/apiWatch.ts#L306

like so:

  instance.effects && remove(instance.effects!, runner)

Another way would be to initialize instance.effects with an empty array, but that could be considered wasteful memory-wise.

Both of these fixes would presuppose that the behaviour of this.$watch adding its effects to the currentInstance instead of the actual instance it belongs to is correct, and I don't think it is.

So then we have to fix recordInstanceBoundEffect as time-jeniks laid out, by passing it the instance

https://github.com/vuejs/vue-next/blob/288c764e5279ccef63e0ef304d4250f5ad935a46/packages/runtime-core/src/component.ts#L759-L763

maybe something like

export function recordInstanceBoundEffect(effect: ReactiveEffect, instance?: ComponentInternalInstance) {
  if (!instance && currentInstance) {
   instance = currentInstance
  }

  if (instance) {
    ;(instance.effects || (instance.effects = [])).push(effect)
  }
}