Correct MutationObserver ownership story #1160
Conversation
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.
|
@rniwa @smaug---- @domenic @shaseley @FremyCompany @pmdartus I would appreciate your review! |
|
LGTM. This matches what Blink's doing with the pending mutation observers. |
FremyCompany
left a comment
FremyCompany
left a comment
There was a problem hiding this comment.
I will admit I had no recollection of our previous discussion, but after reviewing your change, I think it makes sense. That way, only pending MutationObserver instances are held strongly by the document. All the others get automatically collected when all the elements they are attached to leave the document.
PS: This review will show as a comment as I do not appear to have the rights to mark the PR "Approved" directly, but I think this can likely go ahead.
|
Thanks! I will merge this somewhere next week assuming Bikeshed issues have been resolved and there's no further feedback. |
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.
Preview | Diff