Bug 20131 - [MutationObservers] Unclear whether unique record instances are required in 5.3.2 "Queuing a mutation record"
[MutationObservers] Unclear whether unique record instances are required in 5...
Status: CLOSED FIXED
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC Windows NT
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-28 19:49 UTC by Travis Leithead [MSFT]
Modified: 2012-12-03 17:04 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Travis Leithead [MSFT] 2012-11-28 19:49:25 UTC
Given the updated algorithm in 5.3.2 (Queuing a mutation record), if I have four unique observers, each observing a single DOM Text node, and I cause a mutation to that text node, then should I expect to see four unique record objects (one in each of the observer's queues), or one unique record object that is referenced from each of the four queues?

I suspect that the intent of the spec is that four unique records are created and each unique record object resides in each queue, thus:
var recList1 = ob1.takeRecords();
var recList2 = ob2.takeRecords();
var recList3 = ob3.takeRecords();
var recList4 = ob4.takeRecords();

assert((recList1[0] !== recList2[0]) &&
       (recList2[0] !== recList3[0]) &&
       (recList3[0] !== recList4[0]) &&
       (recList4[0] !== recList1[0]) &&
       (recList1[0] !== recList3[0]) &&
       (recList2[0] !== recList4[0]));

This is the behavior I see in Firefox.

The spec is ambiguous in step 7.2 where it says to append the record to the observer's record queue. A literal reading of this (to me) indicates that there is only one single record object and it will belong to each of the record queues it is appended to--but this is contrary to the behavior seen in Firefox. To align with Firefox, step 7 might have a step 0 which reads, let rec be a copy of record.

If, in fact the spec is meant to only have a single instance of the record object reside in each of the queues, then there's still an ambiguity with regard to how oldValue is populated. The spec's algorithm would populate oldValue on the record and all observers would get the old value on the record if any one of them wanted it--is that the desired behavior? No browser currently implements it that way. Chrome comes close. Given the four observers case above, if two of them (2 & 4) request the old value, then Chrome creates 2 unique instances of the record and gives observers 1 & 3 one instance (without the oldValue) and 2 & 4 the other instance so that:
assert(recList1[0] === recList3[0])
assert(recList2[0] === recList4[0])
Comment 1 Anne 2012-11-28 20:19:13 UTC
So to me it seems much better to have a single instance that is shared as that creates less objects overall. If you did not ask for oldValue and it is there anyway, is that a big problem?
Comment 2 Olli Pettay 2012-11-28 20:20:38 UTC
Different mutation observers (I'm not talking about possible transient observers) shouldn't share mutation records
Comment 3 Olli Pettay 2012-11-28 20:28:02 UTC
...because different instances of MutationObserver may observe
different things.
One script library might observe only things X, and another one
things Y. those things shouldn't mix up.
Comment 4 Adam Klein 2012-11-28 20:43:04 UTC
WebKit shares records wherever possible to reduce the memory overhead of the feature. Given that all attributes are readonly, this doesn't seem very problematic (the worst that can happen is that the records become a side-channel for expando properties; Events already have this behavior).

As Travis rightly points out, the existing queueing algorithm would need one more loop to describe WebKit's behavior, since we at most create two records for a given mutation (one with an oldValue and one without).
Comment 5 Olli Pettay 2012-11-28 20:47:07 UTC
It would be odd if script library A observing mutations without
oldValue would get oldValue. That would be even error prone:
scripts might rely on the data to not be there if they are not observing it.
So, records should be per observer.

If implementations want to share the internal data, that is ofc ok, but better
to not expose that to JS.
Comment 6 Anne 2012-11-28 20:50:30 UTC
Adam is not suggesting exposing oldValue to observers that did not ask for it though. (The specification has that, and I don't think it's a big deal personally, but that can be changed.)
Comment 7 Olli Pettay 2012-11-28 20:53:42 UTC
Well, that is even more strange.
In same cases observers might share records, in some case not.

Consistent APIs, please ;)
Comment 8 Adam Klein 2012-11-28 21:27:09 UTC
(In reply to comment #7)
> Well, that is even more strange.
> In same cases observers might share records, in some case not.
> 
> Consistent APIs, please ;)

I don't think developers are likely to care much about the identity of records, but I do think they care about the memory footprint of the browsers they work in.

For what it's worth, the old queueing algorithm specified WebKit's behavior with regard to identity (see "record" and "recordWithOldValue"):
http://www.w3.org/TR/2012/WD-dom-20120405/#concept-mo-queue-attributes

(not surprising, since I think I wrote that text).
Comment 9 Olli Pettay 2012-11-28 21:42:13 UTC
(In reply to comment #8)
> I don't think developers are likely to care much about the identity of
> records, but I do think they care about the memory footprint of the browsers
> they work in.

I do care about API consistency. That is what is exposed to the web.
Also, I believe memory usage won't be a problem in this case.
Mutation records should be short living objects and created
only within one microtask, so by far the most common case is to have just
few mutation records queued.
And, implementations are free share the record instances behind the
scenes. Just create different JS wrapper for each observer. That can be done in a
quite cheap way, but is very much implementation detail.
Comment 10 Rafael Weinstein 2012-11-28 21:48:53 UTC
I'm not clear what you think is inconsistent about different observers sharing mutation records.

Can you explain a bit more? I.e. which thing is inconsistent with which other thing(s)?
Comment 11 Olli Pettay 2012-11-28 21:53:46 UTC
My inconsistency argument came from comment 6.
if A isn't expecting oldValue and B is and there are two
records, one with oldValue, one without.
The one without oldValue would be shared, the other one wouldn't.
Comment 12 Rafael Weinstein 2012-11-28 22:08:28 UTC
In your example, neither are shared between A and B. A sees |record| and B sees |recordWithOldValue|.

But in general, both are shared. All observers who requested oldValue see the same |recordWithOldValue| and all observers who did not see the same |record|.
Comment 13 Olli Pettay 2012-11-28 22:13:51 UTC
(In reply to comment #12)
> In your example, neither are shared between A and B. A sees |record| and B
> sees |recordWithOldValue|.
Why? Both observers should get 2 records. One record is with oldValue, one without. But A should never see record with oldValue, just similar record which
has empty old value.
Comment 14 Rafael Weinstein 2012-11-28 22:35:26 UTC
I don't think so, but maybe we're talking at cross purposes. To make it more concrete:

<script>
var MutationObserver = MutationObserver || WebKitMutationObserver;
var div = document.createElement('div');
div.setAttribute('foo', 'foo');

function makeHandler(name) {
  return function handler(recs) {
    console.log('observer ' + name);
    console.log('count: ' + recs.length);
    console.log('oldValue: ' + recs[0].oldValue);
    if (recs[0].expando) {
      console.log('exando found');
    } else {
      recs[0].expando = 1;
      console.log('expando not found, and written');
    }
  };
}

var A = new MutationObserver(makeHandler('A'));
A.observe(div, { attributes: true, attributeOldValue: true });

var B = new MutationObserver(makeHandler('B'));
B.observe(div, { attributes: true });

var C = new MutationObserver(makeHandler('C'));
C.observe(div, { attributes: true, attributeOldValue: true });

var D = new MutationObserver(makeHandler('D'));
D.observe(div, { attributes: true });

div.setAttribute('foo', 'bar');
</script>

In WebKit this outputs:

observer A ---------
count: 1
oldValue: foo
expando not found, and written
observer B ---------
count: 1
oldValue: null
expando not found, and written
observer C ---------
count: 1
oldValue: foo
exando found
observer D ---------
count: 1
oldValue: null
exando found 

In Gecko this outputs:

observer A ---------
count: 1
oldValue: foo
expando not found, and written
observer B ---------
count: 1
oldValue: null
expando not found, and written
observer C ---------
count: 1
oldValue: foo
exando not found, and written
observer D ---------
count: 1
oldValue: null
exando not found, and written
Comment 15 Olli Pettay 2012-11-28 22:43:42 UTC
That creates only one record (per observer). What I was thinking is more like
div.setAttribute('foo', 'bar');
div.setAttribute('foo', 'bar2');
Comment 16 Olli Pettay 2012-11-28 22:51:50 UTC
...that is without setting attribute before creating observers.
Comment 17 Rafael Weinstein 2012-11-28 23:18:59 UTC
I see. Thank you for clarifying. Yes, in that case in WebKit (and the original spec), the record(withoutOldValue) is delivered to A and B.

So your concern is that developers attempt to rely on records either always or never being shared?
Comment 18 Olli Pettay 2012-11-29 00:10:59 UTC
(In reply to comment #17)
> So your concern is that developers attempt to rely on records either always
> or never being shared?
Right.

And I'd prefer never sharing because that just makes the API more coherent.
Records delivered for observer Foo are never delivered for observer Bar.
That way script libraries can rely on that no one can alter their
records (if they use expandos for example). 

And so far I haven't seen cases where memory usage of records is problematic.
They are (usually) short living tiny objects.
Feel free to prove this wrong with a real world use case :)
Comment 20 Travis Leithead [MSFT] 2012-12-03 17:04:48 UTC
Looks good. Thanks for clarifying!