DOM node is not attached to ElRef under some conditions

[wkordalski]

Versions

I can reproduce this bug in Seed 0.9
I cannot reproduce this bug in Seed 0.8

In short:

1. In view we return something like div![el_ref(&some_elref), div!["Some text"]]
2. After running view, inside update some_elref.get().is_none() is true.

This means that DOM node is not attached to ElRef under some conditions.

Example: https://github.com/wkordalski/seed-elref-bug

Run it with:

yarn
yarn run webpack serve

Open localhost:8888
Open Developer tools (esp. console).
Click "Add measurements" few times (not every time this bug is visible)

Exploading assertion should be visible in the console.

...under some conditions

It seems that this bug is related to running async code or websockets.

[flosse]

thanks for reporting!

[wkordalski]

It turns out that switching
from "websocket send-and-receive" as an asynchronous task
to timer-based asynchronous task
also reveals this bug.

As the timer-based example can be easier to analyze, I pushed timer-based version to repository on branch with-timers

[flosse]

@wkordalski thanks for the example in the repo. I can reproduce the described behavior.
Since i was not much involved in the changes in the v0.9 release, it is hard for me to guess what the cause could be.
@MartinKavik or @fosskers do you have a guess?

[MartinKavik]

The example is too big for me to quickly orient in it by myself and imagine all possible cases, so only some general ideas:

1. ElRef integration to Seed rendering system was a bit painful and error-prone if I remember correctly. I wouldn't be too much surprised if there is a bug / unhandled edge case.
2. I'm not sure if it's possible to accidentally call some_elref.get() during the rendering when the diff algorithm is just removing / reattaching new DOM elements - just an idea because async & websockets have been mentioned.
3. I had some similar ElRef-related problems in the past but there were my mistakes in all cases (forgotten skip(), wrong assumption that DOM elements have been already rendered, etc.)

Sorry I can't help more, gl 🙂

[wkordalski]

The example is too big for me to quickly orient in it by myself and imagine all possible cases

Sorry. I've reduced example size from around 5k LOC to 250 LOC (counting Rust only), and I feel it can be hard to reduce more.

1. ElRef integration to Seed rendering system was a bit painful and error-prone if I remember correctly. I wouldn't be too much surprised if there is a bug / unhandled edge case.

I won't be surprised too.

2. I'm not sure if it's possible to accidentally call some_elref.get() during the rendering when the diff algorithm is just removing / reattaching new DOM elements - just an idea because async & websockets have been mentioned.

As I know, whole Seed app and timers run on a single thread and the async tasks are scheduled using non-preemptive scheduler (run setInterval(handler, 1000); while (true) {} and see that handler is never called). Thus some_elref.get() inside update should not be executed during DOM update.

What is more, replacing assert_eq!(...) with if ... { seed::log!(...) } shows that ElRef never gets attached and the node is being rendered. So this cannot be the case that "we asked ElRef too early", but rather "we created the ElRef wrongly".

3. I had some similar ElRef-related problems in the past but there were my mistakes in all cases (forgotten skip(), wrong assumption that DOM elements have been already rendered, etc.)

This might be. Especially that the documentation of ElRef is concise and misses the conditions the programmer should hold to But why the problem does not appear in Seed 0.8?

---

Generally thank you very much for your hints and ideas!
(And for your work that resulted in Seed 0.8, which is usable in our use-case).
I don't even expect that you or someone else fix this problem as our usage of Seed is quite nonstandard.

I hope that this report will help somebody who meet a similar problem or while fixing another bug.
Currently we can stay with Seed 0.8 and maybe fix this bug in the future when money allow us to do so :)

[wkordalski]

Ok... At least I did git bisect and it turns out that that commit 5a4e91b introduces this bug.

5a4e91b36e4fa055d7f9e441171b12a3c889dfa5 is the first bad commit
commit 5a4e91b36e4fa055d7f9e441171b12a3c889dfa5
Author: glennsl <[email protected]>
Date:   Sun Oct 3 23:32:34 2021 +0200

    refactor(virtual_dom): simplify/deduplicate wiring related to DOM insertion

 src/browser/dom/virtual_dom_bridge.rs | 77 +++++++++++++++++++++--------------
 src/virtual_dom/patch.rs              | 23 +----------
 2 files changed, 48 insertions(+), 52 deletions(-)