Bug 98921 – MutationRecord addedNodes/removedNodes should never be null

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

We currently return empty NodeLists for addedNodes/removedNodes in several places where we should return null.

See http://dom.spec.whatwg.org/#concept-node-insert step 7

var div = document.createElement('div');
var observer = new JsMutationObserver(function() {});
observer.observe(div, {
  childList: true
});

var a = document.createTextNode('a');
div.appendChild(a);

var records = observer.takeRecords();
assert(records[0].removedNodes === null);


There is one case of "removedNodes null" in the spec and two cases of "addedNodes null". Searching is your friend.

[Adam Klein]

I think perhaps the spec should simply be updated to match WebKit and Gecko behavior for 'childList'-typed records.  My memory is that the behavior here is intentional to avoid requiring a null-check: consumers of the API can simply walk over addedNodes/removedNodes blindly.

[Erik Arvidsson]

Changing the spec is also acceptable.

[Adam Klein]

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=19463, the spec has been updated. Updating this bug appropriately.

The spec now states that addedNodes/removedNodes should always return non-null NodeLists, which will have items in them if appropriate.

[Erik Arvidsson]

So we are broken for attribute and characterData

[Adam Klein]

Created attachment 168461 [details]
Patch

[Erik Arvidsson]

Doesn't this patch return the same NodeList instance?

var div = document.createElement('div');
var observer = new WebKitMutationObserver(function() {});
observer.observe(div, {attributes: true});
div.setAttribute('a', 'A');
var records = observer.takeRecords();
shouldBeTrue("records[0].addedNodes !== records[0].removedNodes");

[Adam Klein]

(In reply to comment #6)
> Doesn't this patch return the same NodeList instance?
> 
> var div = document.createElement('div');
> var observer = new WebKitMutationObserver(function() {});
> observer.observe(div, {attributes: true});
> div.setAttribute('a', 'A');
> var records = observer.takeRecords();
> shouldBeTrue("records[0].addedNodes !== records[0].removedNodes");

Yes, it does. Does this strike you as particularly problematic? These node lists are immutable, as are the properties on the records. What's the use case for ===/!== comparisons on MutationRecord.addedNodes/removedNodes?

[Erik Arvidsson]

(In reply to comment #7)
> (In reply to comment #6)
> > Doesn't this patch return the same NodeList instance?
> > 
> > var div = document.createElement('div');
> > var observer = new WebKitMutationObserver(function() {});
> > observer.observe(div, {attributes: true});
> > div.setAttribute('a', 'A');
> > var records = observer.takeRecords();
> > shouldBeTrue("records[0].addedNodes !== records[0].removedNodes");
> 
> Yes, it does. Does this strike you as particularly problematic? These node lists are immutable, as are the properties on the records. What's the use case for ===/!== comparisons on MutationRecord.addedNodes/removedNodes?

It strikes me as odd.

Another problem with this is that you are using static which means the instance will be shared between different domains. How does that work? Will the user get permission errors when the try to access this object? Can you add a test to make sure this works as expected?

[Ryosuke Niwa]

I agree with arv here that reusing the same nod list object is odd. I wonder if we can add some binding level optimization where we can lazy-create zero-length NodeList efficiently...

[Adam Klein]

*** Bug 102825 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Comment on attachment 168461 [details]
Patch

I don’t think we should be using the same object. The behavior doesn’t match that of Firefox or the specification.

[Adam Klein]

Will pick this up again and do something more normal.

[Adam Klein]

Created attachment 178260 [details]
Patch

[Adam Klein]

New patch creates new nodelists for each record, ptal.

[Ryosuke Niwa]

Comment on attachment 178260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review

> Source/WebCore/dom/MutationRecord.cpp:74
> +class EmptyNodeList : public NodeList {
>  public:

Do we really need to add this class?

> Source/WebCore/dom/MutationRecord.cpp:78
> +            nodeList = adoptRef(new EmptyNodeList);

Why can’t we just instantiate StaticNodeList here? We’re creating these objects lazily anyway.

> Source/WebCore/dom/MutationRecord.cpp:110
> +class AttributesRecord : public EmptyChildListRecord {
> +public:

It’s odd that AttributesRecord inherits from EmptyChildListRecord since AttributesRecords is NOT a EmptyChildListRecord.

> Source/WebCore/dom/MutationRecord.cpp:127
> +class CharacterDataRecord : public EmptyChildListRecord {

Ditto.

[Adam Klein]

Comment on attachment 178260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review

>> Source/WebCore/dom/MutationRecord.cpp:74
>>  public:
> 
> Do we really need to add this class?

What's the downside? It's about 5 lines of code (the lazy initialization code I'd need anyway).

>> Source/WebCore/dom/MutationRecord.cpp:78
>> +            nodeList = adoptRef(new EmptyNodeList);
> 
> Why can’t we just instantiate StaticNodeList here? We’re creating these objects lazily anyway.

See above, I'm not clear on why StaticNodeList is better for this. NodeList is already fully virtualized, so the only extra cost is the size of the code for EmptyNodeList (which I'd bet is pretty darn small).

>> Source/WebCore/dom/MutationRecord.cpp:110
>> +public:
> 
> It’s odd that AttributesRecord inherits from EmptyChildListRecord since AttributesRecords is NOT a EmptyChildListRecord.

Maybe I named it poorly? AttributesRecord and CharacterDataRecord are both records-with-empty-childlists.  What else would you say AttributesRecord and CharacterDataRecord have in common?

[Ryosuke Niwa]

Comment on attachment 178260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review

>>> Source/WebCore/dom/MutationRecord.cpp:74
>>>  public:
>> 
>> Do we really need to add this class?
> 
> What's the downside? It's about 5 lines of code (the lazy initialization code I'd need anyway).

We shouldn’t be adding code unless it’s necessary. With that argument, we’ll end up adding all sorts of 5-line code, and soon, we’ll double the number of lines in our codebsae like we did in the past 3 years.
I can be convinced that this is a good idea if there is a data that supports adding this class benefit the real world use case.

>>> Source/WebCore/dom/MutationRecord.cpp:110
>>> +public:
>> 
>> It’s odd that AttributesRecord inherits from EmptyChildListRecord since AttributesRecords is NOT a EmptyChildListRecord.
> 
> Maybe I named it poorly? AttributesRecord and CharacterDataRecord are both records-with-empty-childlists.  What else would you say AttributesRecord and CharacterDataRecord have in common?

How about renaming it to RecordWithEmptyChildList?

[Adam Klein]

Created attachment 178276 [details]
Patch

[Adam Klein]

Comment on attachment 178260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review

>>>> Source/WebCore/dom/MutationRecord.cpp:74
>>>>  public:
>>> 
>>> Do we really need to add this class?
>> 
>> What's the downside? It's about 5 lines of code (the lazy initialization code I'd need anyway).
> 
> We shouldn’t be adding code unless it’s necessary. With that argument, we’ll end up adding all sorts of 5-line code, and soon, we’ll double the number of lines in our codebsae like we did in the past 3 years.
> I can be convinced that this is a good idea if there is a data that supports adding this class benefit the real world use case.

I certainly don't have any data for this, just trying to reduce the size of these objects which are effectively just sentinels by sizeof(Vector). The lazy-initialization may or may not help us if the pattern for code that uses these APIs is to always query addedNodes.length and removedNodes.length. I still think this code was unlikely to cause any real maintenance burden or add to the bloat of the codebase.

But I've replaced with StaticNodeList as requested.

[Ryosuke Niwa]

Comment on attachment 178276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178276&action=review

> Source/WebCore/dom/MutationRecord.cpp:92
> +            Vector<RefPtr<Node> > emptyVector;
> +            nodeList = StaticNodeList::adopt(emptyVector);

You would add StaticNodeList::create() to avoid swap.

[Adam Klein]

Created attachment 178286 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178286 [details]
Patch for landing

Clearing flags on attachment: 178286

Committed r136996: <http://trac.webkit.org/changeset/136996>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 178286 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=178286&action=review

> Source/WebCore/dom/StaticNodeList.h:-43
> -        // Adopts the contents of the nodes vector.

Why remove this comment?

[Adam Klein]

(In reply to comment #24)
> (From update of attachment 178286 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178286&action=review
> 
> > Source/WebCore/dom/StaticNodeList.h:-43
> > -        // Adopts the contents of the nodes vector.
> 
> Why remove this comment?

I discussed this on IRC with rniwa, sorry it didn't end up on the bug: my thought was that now both the name ("adopt") and implementation (using swap()) are together, the comment seems redundant. rniwa agreed. If you think the comment is still useful I'd be happy to add it back.

[Ojan Vafai]

Comment on attachment 178260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review

>>>>> Source/WebCore/dom/MutationRecord.cpp:74
>>>>>  public:
>>>> 
>>>> Do we really need to add this class?
>>> 
>>> What's the downside? It's about 5 lines of code (the lazy initialization code I'd need anyway).
>> 
>> We shouldn’t be adding code unless it’s necessary. With that argument, we’ll end up adding all sorts of 5-line code, and soon, we’ll double the number of lines in our codebsae like we did in the past 3 years.
>> I can be convinced that this is a good idea if there is a data that supports adding this class benefit the real world use case.
> 
> I certainly don't have any data for this, just trying to reduce the size of these objects which are effectively just sentinels by sizeof(Vector). The lazy-initialization may or may not help us if the pattern for code that uses these APIs is to always query addedNodes.length and removedNodes.length. I still think this code was unlikely to cause any real maintenance burden or add to the bloat of the codebase.
> 
> But I've replaced with StaticNodeList as requested.

For the record, I think the StaticNodeList approach that got committed is inferior in every way to the EmptyNodeList approach here. I don't think we need to justify every change with data. The EmptyNodeList approach is clearly avoiding significant memory churn. It wouldn't be hard to come up with an example page where it's problematic.

A few lines of extra code here has effectively 0 cost.

[Ryosuke Niwa]

(In reply to comment #26)
> (From update of attachment 178260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178260&action=review
>
> For the record, I think the StaticNodeList approach that got committed is inferior in every way to the EmptyNodeList approach here. I don't think we need to justify every change with data. The EmptyNodeList approach is clearly avoiding significant memory churn. It wouldn't be hard to come up with an example page where it's problematic.

I disagree. The biggest problem is nobody would know to use EmptyNodeList when they're looking to use StaticNodeList. It's much better to optimize StaticNodeList::createEmpty not to require memory allocation because then people working on any client of StaticNodeList are likely to discover this optimization.