Last Comment Bug 742636 - Keep observed node alive during microtask if there are transient observers
: Keep observed node alive during microtask if there are transient observers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-04 23:50 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-08-07 09:07 PDT (History)
1 user (show)
bugs: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.95 KB, patch)
2012-04-05 00:16 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jonas: review+
Details | Diff | Review
better comment (2.03 KB, patch)
2012-04-10 11:59 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-04 23:50:07 PDT

    
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-05 00:16:26 PDT
Created attachment 612466 [details] [diff] [review]
patch

Transient observers are deleted after the microtask, so kungfudeathgrip will
be cleared at that point.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-05 04:36:28 PDT
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.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-09 15:39:45 PDT
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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-09 22:21:40 PDT
(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.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-10 09:27:16 PDT
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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-10 09:32:14 PDT
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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-10 10:16:13 PDT
So, something like:

<html>
<body><div></div></body>
<script>

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 });
document.body.parentNode.removeChild(document.body);
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()

</script>
</html>
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-10 11:52:55 PDT
Comment on attachment 612466 [details] [diff] [review]
patch

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.
Comment 9 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-10 11:55:09 PDT
I explicitly wanted to use name kungfuDeathGrip so that it is clear why the otherwise not used
variable is there.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-10 11:59:55 PDT
Created attachment 613701 [details] [diff] [review]
better comment
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-10 12:20:23 PDT
https://hg.mozilla.org/mozilla-central/rev/77f0e7d882dd


I'll try to write a testcase for this once the patch for takeRecords has landed.

Note You need to log in before you can comment on or make changes to this bug.