Bug 98921 - MutationRecord addedNodes/removedNodes should never be null
Summary: MutationRecord addedNodes/removedNodes should never be null
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL: http://dom.spec.whatwg.org/#concept-n...
Keywords: WebExposed
: 102825 (view as bug list)
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2012-10-10 09:53 PDT by Erik Arvidsson
Modified: 2012-12-10 17:18 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.99 KB, patch)
2012-10-12 12:17 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2012-12-07 13:06 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (8.56 KB, patch)
2012-12-07 14:28 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (9.98 KB, patch)
2012-12-07 14:55 PST, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-10-10 09:53:54 PDT
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.
Comment 1 Adam Klein 2012-10-10 14:13:15 PDT
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.
Comment 2 Erik Arvidsson 2012-10-10 14:49:05 PDT
Changing the spec is also acceptable.
Comment 3 Adam Klein 2012-10-11 11:56:48 PDT
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.
Comment 4 Erik Arvidsson 2012-10-12 10:19:15 PDT
So we are broken for attribute and characterData
Comment 5 Adam Klein 2012-10-12 12:17:02 PDT
Created attachment 168461 [details]
Patch
Comment 6 Erik Arvidsson 2012-10-12 12:22:05 PDT
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");
Comment 7 Adam Klein 2012-10-12 12:25:29 PDT
(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?
Comment 8 Erik Arvidsson 2012-10-12 12:29:25 PDT
(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?
Comment 9 Ryosuke Niwa 2012-11-03 18:30:47 PDT
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...
Comment 10 Adam Klein 2012-11-20 10:51:14 PST
*** Bug 102825 has been marked as a duplicate of this bug. ***
Comment 11 Ryosuke Niwa 2012-12-03 13:37:34 PST
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.
Comment 12 Adam Klein 2012-12-07 11:32:12 PST
Will pick this up again and do something more normal.
Comment 13 Adam Klein 2012-12-07 13:06:25 PST
Created attachment 178260 [details]
Patch
Comment 14 Adam Klein 2012-12-07 13:06:42 PST
New patch creates new nodelists for each record, ptal.
Comment 15 Ryosuke Niwa 2012-12-07 14:00:05 PST
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.
Comment 16 Adam Klein 2012-12-07 14:07:38 PST
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?
Comment 17 Ryosuke Niwa 2012-12-07 14:17:21 PST
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?
Comment 18 Adam Klein 2012-12-07 14:28:45 PST
Created attachment 178276 [details]
Patch
Comment 19 Adam Klein 2012-12-07 14:31:20 PST
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.
Comment 20 Ryosuke Niwa 2012-12-07 14:32:50 PST
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.
Comment 21 Adam Klein 2012-12-07 14:55:04 PST
Created attachment 178286 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-12-07 15:44:46 PST
Comment on attachment 178286 [details]
Patch for landing

Clearing flags on attachment: 178286

Committed r136996: <http://trac.webkit.org/changeset/136996>
Comment 23 WebKit Review Bot 2012-12-07 15:44:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Darin Adler 2012-12-08 13:19:23 PST
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?
Comment 25 Adam Klein 2012-12-08 14:22:21 PST
(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.
Comment 26 Ojan Vafai 2012-12-10 17:00:13 PST
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.
Comment 27 Ryosuke Niwa 2012-12-10 17:18:34 PST
(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.