Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOM node is not attached to ElRef under some conditions #685

Open
wkordalski opened this issue Jun 7, 2022 · 6 comments
Open

DOM node is not attached to ElRef under some conditions #685

wkordalski opened this issue Jun 7, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@wkordalski
Copy link

wkordalski commented Jun 7, 2022

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 flosse added the bug Something isn't working label Jun 8, 2022
@flosse
Copy link
Member

flosse commented Jun 8, 2022

thanks for reporting!

@wkordalski
Copy link
Author

wkordalski commented Jun 8, 2022

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
Copy link
Member

flosse commented Jun 19, 2022

@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
Copy link
Member

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
Copy link
Author

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.

  1. 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".

  1. 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
Copy link
Author

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(-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants