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 invocation order #713
Comments
What's an active mutation observer list? Mutation observers with at least one record? Firefox also passes this test... |
Yes, see the bottom of jsdom/jsdom#2398 (comment) |
Does this mean that if the @pmdartus willing to add a test for that? @smaug---- do you agree that fixing this would require changing step 2 of https://dom.spec.whatwg.org/#notify-mutation-observers to only include mutation observers whose record queue is not empty in notifyList? |
Ping @pmdartus @smaug---- for @annevk's questions :) |
Hey, for what this is worth, I do believe the current design in the spec is incorrect. Indeed, if the context stores a list of all MutationObservers, this list can either be holding strong references to the MOs or weak references to the MOs. Both approaches are not satisfactory.
According to me, the correct design here is not to have a list of all MOs in the context, but only have a list of MOs which currently do have at least one pending notification. When a record is pushed into a MO, the MO is appended to the "active MO" list with a strong reference. When notifications to MOs are about to be sent, the list of active MOs is temporarily copied then emptied, and notifications are sent to the MOs that are in the list at the time. I do believe this is what Blink and Webkit implement, and I am pretty sure this is how this is implemented in Edge as well, from the last time I looked at the code. |
This is not true, because the nodes have references to their corresponding MOs. (Those references get cleared once notifications are delivered.) Blink's design is similar to my suggestion in #720, but they make the node -> MO references weak, instead of the global list. That may contribute to the differences discussed in the OP, perhaps. |
Sorry @annevk for the late response.
I am not clear what you mean by a promise callback and what behavior you want to test. But yes I would be willing to help write additional WPT. =) |
You are still wrong about this @domenic. I think it's time you start listening to what I am saying... you realize I am also a browser implementer right? I wouldn't start talking about this if I wasn't entirely sure of what I am saying... So, yes, the nodes have a strong reference to their MO but this reference list is not cleared when a notification is dispatched; quite the opposite this is the list that is used to find out which notifications to dispatch in the first place, it is only ever cleared when an MO is disconnect()ed. See the spec:
if this list was cleared, it could not be used to create the notifications in the first place. The list that is cleared after each notification is the "active MOs list" which holds the reference to the MOs until their notifications have been processed (that list does not exist in the spec, but does exist in Blink; instead the spec has a list of all MOs, see later why this distinction is important). Now, to address your point that the nodes have a strong ref to the MOs, let's consider this practical case:
Okay, not let's say node
At this point, This is wrong, because the MO should get to process the notification. For instance, it could process the notification, and reinsert the node in the document, keeping itself alive. Also whether the MO would receive the notification or not would leak whether GC has happened or not. However, this doesn't happen in Blink because there is the The model in the spec is incorrect because it doesn't have an Please reread this until you get it, seriously. |
@FremyCompany please adhere to https://whatwg.org/code-of-conduct. Assuming malice is not the way to go. @pmdartus I was thinking that if the |
@annevk I am sorry, I wrote that on the heat of the moment. It's been a little rough in the last few months as you can imagine. I would appreciate it however if someone could read my explanation of what is currently implemented in all browsers and acknowledge it :-/ |
@FremyCompany I think you're correct that the specification leaks memory. And I'd accept a PR that addresses it or a new issue for the backlog. However, I'll note that we haven't formalized strong/weak references and typically assume infinite resources, and the problem I'd like to keep this issue focused on are the web observable differences pointed out in OP. |
@annevk If you replace the global static list of MOs by the dynamic list of active MOs, the test issue mentioned above disappeared because you will be iterating over active MOs that have a pending notification which is strictly equivalent to looping through all MOs (current spec) but filter out those who had no pending notification at the start of the loop (currently proposed edit). Basically those two issues are one and the same according to my understanding, which is why I commented here. |
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.
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.
Based on: jsdom/jsdom#2398 (comment)
While implementing MutationObserver in jsdom, I wasn't able to make the following test pass while following the spec: https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/slotchange-event.html#L540-L594
The test run the following steps:
id
attribute and another one ontitle
) and a slot with aslotchange
event handler.id
attribute and the append a child in the light dom of the host. We then enqueue a microtask to notify the listeners. At this point theid
attribute mutation observer contains 1 mutation record, and the signal slot list contains the slot.id
mutation observer is executed and in it's callback it updates title attribute and append a new element the light dom of the host. By updating thetitle
attribute a new mutation record is enqueued but this time for thetitle
mutation observer.title
mutation observer is invoked because it has a mutation record. And the test throws here since it expects that theslotchange
event is invoked first.Chrome and Webkit don't run into the same issue since they keep track of an active mutation observer list instead of iterating over all the mutation observers.
The text was updated successfully, but these errors were encountered: