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

Widget got deallocated #992

Open
KaitlynEthylia opened this issue Dec 5, 2023 · 7 comments · May be fixed by #1189
Open

Widget got deallocated #992

KaitlynEthylia opened this issue Dec 5, 2023 · 7 comments · May be fixed by #1189

Comments

@KaitlynEthylia
Copy link

Since starting using eww, I've always had the error message "Couldn't upgrade reference, widget got deallocated" filling the log, but I never bothered with it because it didn't stop it working and it didn't cause any problems, but after having my laptop running for an extending period of time, I noticed sddm's wayland-session log (eww is launched via hyprland running eww daemon on start)
taking up all of my disk space

What's causing this error message and is there any way I can get rid of it

@3l0w
Copy link

3l0w commented Dec 8, 2023

Related to #750 ?

@rtoijala
Copy link

rtoijala commented Feb 6, 2024

I have the same issue. I have managed to find a simple reproducer:

(defpoll var :interval "1s" :initial "[0]" "echo '[0]'")

(defwindow foo
  :monitor 0
  (box
    (for dummy in var
        var)))

After the first second, this will print the following:

 2024-02-06T20:50:30.252Z ERROR eww::error_handling_ctx > Error while updating UI after state change

Caused by:
    Couldn't upgrade reference, widget got deallocated

After the second second, it will print the same error twice. After the third second, three times, and so on.

@rtoijala
Copy link

rtoijala commented Feb 7, 2024

Looks like for me, commit 0cccd9d fixes the problem. It's from 2022 but there has been no new release since then.

@w-lfchen
Copy link
Contributor

w-lfchen commented Sep 2, 2024

seems to be fixed since there has been a release. can this issue be closed now?

@rtoijala
Copy link

rtoijala commented Sep 2, 2024

Good that you bumped this issue, I some time ago found a new reproducer that I never got around to posting here.

(defpoll var
  :interval "1s"
  :initial '[0]'
  'echo [0]')

(defwidget widg [v]
  (box
    (for dummy in {v}
      (box
        (children)))))

(defwindow broken
  :monitor 0
  (widg :v {var}
      {var[0]}
  ))

(defwindow works
  :monitor 0
  (box
    (for dummy in {var}
      (box
        {var}))))

The script defines a variable var that is updated at one second, always with the value [0]. This is just for demonstrating the problem by forcing rerendering of the widgets.

Then the script defines a widget widg that takes as a parameter v that will always be [0]. The children element is used inside a for, always creating one copy of the children in each updat cycle. widg does not directly depend on the value of v in any other way.

When the window broken includes a widg, passing along the var as v but also depending on var in widg's children, the following is logged each time var is updated:

 2024-09-02T16:02:55.131Z ERROR eww::error_handling_ctx > Error while updating UI after state change

Caused by:
    Couldn't upgrade reference, widget got deallocated

For comparison, the window works that directly does the same without the indirection via widg does not log the error message.

I had some notes on my analysis at the time, but I managed to mislay them while moving in the time between. From what I recall, I think the root cause is related to the fact that the children (i.e. the text element that prints var[0]) are kind-of owned both by the for in widg, and directly by the window broken. When the for updates, it removes the old children, replacing them with new instances. Soon afterwards, in the same update cycle, broken tries to update the text var[0] (which depends on a variable from that scope), but finds it gone, removed by the for. The listener installed by broken's scope was not removed. The tracking of ownership / scopes / listeners is not handled well in the case of for and children together. I recall ending up with the conclusion that it is necessary to track which listeners are created for each widget so that they can be removed when the widget is removed. But as said, my notes are gone, so please treat that as a vague hint that may well be wrong.

@w-lfchen
Copy link
Contributor

w-lfchen commented Sep 2, 2024

update: i think i found what might be causing this (very unsure):
in build_children_special_widget, the scope passed to the children seems to be never updated, unlike the one the widget operates in.
is this intentional?
i'm really confused about the purpose of the CustomWidgetInvocation::scope field, that one seems to be unnecessary and it is never updated

that's a very detailed report nonetheless, thank you!
i'll see whether i can look into this but i'm still rather inexperienced, so please don't expect much

if i find something, i'll ping you so that you can try breaking it again if that's fine ^^
s my findings so far:
i've found that in the broken example, with each poll, another listener is added.
obviously, this is not the case for the working example, so you are correct.

this todo is referring to this exact issue.

i'm currently evaluating whether self.evaluate_simplexpr_in_scope, as called in notify_value_changed could be of interest. it errors in the broken example, but not in the working one

here's a solution that fixes the symptoms, not the cause:

scope
    .listeners
    .entry(required_var.clone())
    .and_modify(|ls| ls.retain(|l| Rc::strong_count(l) > 1))
    .or_default()
    .push(listener.clone());

instead of this line
this shouldn't leak memory, but it's also obviously not ideal as the issue persists, we're just dealing with the symptoms, and there's a bit of a performance cost

however, it might be worth adding this while the root cause is unknown

here's a smaller reproducible example:

(defwidget widg2 []
  (box
    (for dummy in var
      (box
        (children)))))

(defwindow broken2
  :monitor 0
  (widg2
    var
  ))

@w-lfchen w-lfchen linked a pull request Sep 3, 2024 that will close this issue
3 tasks
@w-lfchen
Copy link
Contributor

w-lfchen commented Sep 3, 2024

i've opened #1189.

since this should be tested very well, i'd appreciate it a lot if you (the person reading this) could help out and report there.

actively trying to break things is encouraged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants