Keep observed node alive during microtask if there are transient observers




6 years ago
5 years ago


(Reporter: smaug, Assigned: smaug)


12 Branch
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(2 attachments)

Comment hidden (empty)

Comment 1

6 years ago
Created attachment 612466 [details] [diff] [review]

Transient observers are deleted after the microtask, so kungfudeathgrip will
be cleared at that point.
Attachment #612466 - Flags: review?(jonas)

Comment 2

6 years ago
Another option would be to copy all the states from the original receiver to
transient receivers, but just keeping the node alive is a simpler approach.
I don't understand this. Why should we keep a node alive just because there are transient observers to it? If the only thing keeping the node alive is the transient observer, then the node won't be modified anyway and so there won't be any mutations to record.

If I'm misunderstanding, feel free to explain a scenario where this would be detectable by the page. I note that there is no testcase in the patch which might help with understanding when this would make a difference.

Comment 4

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #3)
> I don't understand this. Why should we keep a node alive just because there
> are transient observers to it?
The current setup is that if the original observed node goes away, all the receivers
will be removed. That is not what we want. The transient receivers should work until the
end of microtask.
But why? How does the page see a difference? I.e. what is the downside of keeping the code as it is?

Is it just a matter of code simplicity?

The downside of this patch is that it causes us to keep nodes alive longer. But it's obviously not a whole lot longer since it's just to the end of the current microtask.

Comment 6

5 years ago
Currently, if node is deleted, so are all the transient observers, so MutationObserver doesn't
observe all the possible mutations in the observed set anymore. Keeping node alive
until the end of microtask makes sure we don't lose any records.

Comment 7

5 years ago
So, something like:


function callback(records) {
  // records should contain the changes to the div element's child list.

var div = document.body.firstChild;
var mo = new MutationObserver(callback);
mo.observe(document.body, { childList: true, subtree: true });
div.parentNode.removeChild(div); // div has now a transient observer
mo.takeRecords(); // Clear the records, so that nothing there keeps body element alive
// do something which CC/GC so that the body element, which isn't kept alive by anyone
// is deleted (this is what the bug is about. Someone should keep body alive)
div.appendChild(document.createTextNode("hello world")); // this should end up to the callback()

Comment on attachment 612466 [details] [diff] [review]

Ok, I finally understand. The example helped a lot.

So we're keeping a bit more memory alive here than we need to, but it's not a big deal really. And we can always change things later if we care to.

By the way, you can easily write a testcase for this by calling SpecialPowers.gc();

I'm not super excited about the name mKungFuDeathGrip though. We usually use that for the very specific case of keeping yourself alive for the lifetime of a function call. Here you're keeping more than yourself alive, and you are doing so for longer than a function scope.

Maybe mTargetOwner? I can live with mKungFuDeathGrip if you really prefer it.

But make the comment something like: "While we have transient observers, keep the original mutation observer alive so it doesn't go away and diconnect all its transient receivers."

It's not actually important that the original target node stays alive.
Attachment #612466 - Flags: review?(jonas) → review+

Comment 9

5 years ago
I explicitly wanted to use name kungfuDeathGrip so that it is clear why the otherwise not used
variable is there.

Comment 10

5 years ago
Created attachment 613701 [details] [diff] [review]
better comment

Comment 11

5 years ago

I'll try to write a testcase for this once the patch for takeRecords has landed.
Flags: in-testsuite?


5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED


5 years ago
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.