shouldNotUpdate throws an uncaught exception: vnode._state is undefined

[barneycarroll]

When shouldNotUpdate attempts to query typeof vnode._state.onbeforeupdate, vnode._state evaluates to undefined and throws.

It's unclear when vnode._state is set to this value - querying _state on the vnode in lifecycle methods always reveals an object.

I've tried to find the instances where _state is assigned to in Mithril source but in putting breakpoints at the moment state is assigned to _state, I can't find an instance where state is undefined before the exception occurs.

Steps to Reproduce (for bugs)

https://barneycarroll.github.io/mle/

1. Use the + button in the Node element twice.
2. Observe the error: Uncaught TypeError: Cannot read property 'onbeforeupdate' of undefined (Chrome) or TypeError: vnode._state is undefine (Firefox)

Your Environment

• Version used: 1.1.6
• Browser Name and version: Firefox 57.0.2 (64-bit), Chrome 63.0.3239.132 (Official Build) (64-bit)
• Operating System and version (desktop or mobile): Windows 10 Pro version 1709 build 16299.125
• Link to your project: https://github.com/barneycarroll/mle

[dontwork]

I believe I had this issue when I had a stream.map inside oninit. That stream.map had an m.redraw call which was what caused the issue. Isaiah then got me to do promise.resolve.then(m.redraw). Don't know if it's relevant but thought I'd add it here

[barneycarroll]

The app does play tricks with the redraw loop (part of the app is meant to draw in response to the draw loop of the other 😬) so something along those lines isn't beyond the realms of possibility. But — aren't the failure cases for lifecycle / redraw triggers either noop or infinite loop? I don't understand how things could get any more complicated than that from a Mithril internals perspective.

[pygy]

@barneycarroll Are you mutating a vnode.children list somewhere in the hooks?

[barneycarroll]

@pygy I'm relying on a state property of children… 🤔 ? It's exclusively within the Node component - I don't believe I've mis-referenced them. https://github.com/barneycarroll/mle/blob/master/Node.js#L21

I renamed the reference to avoid confusion, the problem persists…

[dead-claudia]

@barneycarroll vnode.state.children should have no effect.

By any chance, does this repro with next? (link) I suspect it probably would, but given that the node in question is keyed and that @pygy has done quite a bit of work in area recently, things might have changed.

Also, you block every m.redraw call with a requestAnimationFrame, which should alleviate all concerns about recursive rendering. (The m.render algorithm isn't re-entrant, but your method is even safe with v2's m.redraw.sync().)

---

@pygy From digging through the debugger, it's erroring on this node specifically, just not finding the component's onbeforeupdate. No clue what could possibly be clearing vnode._state, though. (The only place in v1 where we explicitly clear it is here, but that code path is correctly never hit.)

Found a lead, though, for v1 at least: vnode.instance is receiving an empty _state here (for obvious reasons), but on the second iteration, vnode.instance is set to the node itself somehow.

@barneycarroll Talk about obscure bugs...this is a torture cell... 😟 😄

[dead-claudia]

As for so far, I feel like I've not even scratched the surface at this point.

[dead-claudia]

vnode._state no longer exists, so closing.

[dead-claudia]

I'm re-opening this to see if it repros against v2 with vnode.state.

[dead-claudia]

@barneycarroll If you update your utility to mithril@next, does this still reproduce?

[charlag]

@isiahmeadows hi!
Does it makes sense for us to update? Is it stable for the production?

[barneycarroll]

Still reproducible with v2.

[dead-claudia]

@barneycarroll Could you come up with an automated repro that doesn't require individual clicks? Something that only requires m.render so it's a little easier to trace?

[pygy]

got it.

When shouldNotUpdate is true, we should also copy the rest of the state of the old vnode (children, instance, dom) on the new one. Otherwise on the next cycle, the old vnode is unbaked, and thus unfit for diff.

[dead-claudia]

@pygy That might do it. I for some dumb reason thought we kept the previous subtree, or I would've caught it and fixed it years ago. 😄

Can't wait until I can review your eventual PR for this. 🙂 👍

[pygy]

Actually, we already copy dom, domsize and instance in shouldNotUpdate(). We should add text and children for completeness. The old attrs should also be copied.

tentative test cases

[pygy]

Another option would be to drop support for onbeforeupdate as an attribute. I'm not very comfortable with messing with the attrs and children since they are user-provided...

Edit: at least, stop the "skip diff" part. Not sure where that hook would otherwise be useful though...

[barneycarroll]

@pygy we could loose onbeforeupdate without feature loss if we implement (before, after)

[pygy]

@barneycarroll Good point. That approach means that we keep "skip diff if old === current" though.

[dead-claudia]

@pygy If we drop onbeforeupdate, I'd rather us use a "don't update" sentinel we just take to mean "replace me with the previous subtree before sending it to any hooks, and don't call any hooks on it".

Alternatively, we could just have people return vnode.instance from view and add view as a magic attribute alternative to vnode.children. We'd just elevate vnode.instance to public status, documenting it as a readonly property holding the last rendered children of that DOM node/fragment/component instance. Personally, I like this method better, since it's easier to explain and still reasonably easy to implement.

But in either case, I'd rather not encourage people to memoize vnode instances on their own. Mithril's allowed to as it owns them and controls them, but users are not since they give their vnodes to Mithril - they don't just lend them like they do in React and similar. (Unlike JS or even TS, Rust provides types to verify this, but because this is in JS, we have to rely on conventions for it.)

[dead-claudia]

But in either case, this really needs fixed in v1 and v2.

[charlag]

What other type enforcement other than "readonly instance" on the vnode given to view we need?

[dead-claudia]

@charlag None, really. I was just referencing why we discourage vnode memoization compared to why React allows (and encourages it) in terms of Rust's lifetimes and borrowing.

[dead-claudia]

Note to self for later: find time to get the fix implemented and landed in the main branch. Not an explicit v2 blocker, but it's something I want to get in ASAP.

[dead-claudia]

Reduced the issue to three variants, one missed in the initial investigation: 1 2 3

When you go to view it, open the browser console. Flems just unhelpfully shows "Script error". The first one results in an error like that of this bug. The second results in a DOM node going mysteriously missing. The third prints a wrong attribute value. All three have a nearly identical root cause.

Stepping through each variant, it's not as simple as @pygy's initial suggested fix made it seem, but his follow-up is almost there. What's happening is that somehow the old vnodes that Mithril has are getting crossed up with data that's never used. So what I'm getting is this:

• Previous attributes need retained for proper attribute diffing.
• Previous children + text need retained for proper children diffing.

It may have seemed like vnode.state wasn't being set appropriately, but it's already been set prior to even entering that invocation, so that's not the issue.

Fix finally incoming!