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

MutationObserver GC section looks bogus #1159

Closed
annevk opened this issue Feb 13, 2023 · 3 comments · Fixed by #1160
Closed

MutationObserver GC section looks bogus #1159

annevk opened this issue Feb 13, 2023 · 3 comments · Fixed by #1160

Comments

@annevk
Copy link
Member

annevk commented Feb 13, 2023

As discovered in #1152 (comment) by @shaseley.

  1. Strong references are implied.
  2. The weak reference is about a concept the specification doesn't acknowledge at all. I vaguely recall that it might have been refactored away.
@annevk
Copy link
Member Author

annevk commented Feb 13, 2023

This originated in https://www.w3.org/Bugs/Public/show_bug.cgi?id=16638.

I think the main problem is the handling around https://dom.spec.whatwg.org/#mutationobserver-node-list. We don't ever seem to empty it currently.

  1. Should that be a list of weak references? I guess so.
  2. Should we explicitly empty it when invoking disconnect()? I guess so.

With that I think we can remove https://dom.spec.whatwg.org/#garbage-collection as that seems largely bogus.

@rniwa @smaug---- thoughts?

@smaug----
Copy link
Collaborator

Is (2) needed? Nodes may disappear from the list at any point. Somehow the node list would need to be able handle weak references and that should be enough, no?

I don't know if https://dom.spec.whatwg.org/#garbage-collection is bogus. Perhaps not very clear though.
But if the node list could just explicitly handle weak references, that should be enough, I think.
Doing also (2) might make the spec easier to understand.

(In Gecko MutationObserver has a strong list of MutationReceivers, which are the things bound to a node to observe some changes. MutationReceivers don't own the relevant node. A MutationReceiver will get notified when the Node is about to be deleted and at that point it asks MutationObserver to drop any references to itself. Calling disconnect() removes all the MutationReceivers from the MutationObserver)

@annevk
Copy link
Member Author

annevk commented Feb 13, 2023

I guess you're right that they'll automatically disappear anyway. I don't have a lot of experience with weak references so I don't know if clearing them proactively like that is bad/who cares/good.

There's also #713 (and #720) about the global list of MutationObserver objects being wrong. #713 (comment) still seems like the correct solution for that.

I'll try to create a PR for this and then get everyone involved to date to review.

annevk added a commit that referenced this issue Feb 13, 2023
The existing design had some leaks and was also different from implementations. In particular, assume you have two mutation observers and a change triggered the first one as well as a slot change. Then if in the reaction to the first one a further change triggers the second one it would run before the slot change per the existing specification, but that didn't match implementations and more importantly didn't allow for garbage collection.

Test: https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/slotchange-event.html#L540-L594.

Fixes #713. Fixes #1159. Closes #720.
annevk added a commit that referenced this issue Feb 20, 2023
The existing design had some leaks and was also different from implementations. In particular, assume you have two mutation observers and a change triggered the first one as well as a slot change. Then if in the reaction to the first one a further change triggers the second one it would run before the slot change per the existing specification, but that didn't match implementations and more importantly didn't allow for garbage collection.

Test: https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/slotchange-event.html#L540-L594.

Fixes #713. Fixes #1159. Closes #720.
@annevk annevk mentioned this issue Mar 3, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants