Bug 157556 – Update Node::appendChild() / replaceChild() / removeChild() / insertBefore() to take references instead of pointers

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Update Node::appendChild() / replaceChild() / removeChild() / insertBefore() to references instead of pointers.

[Chris Dumez]

Created attachment 278585 [details]
Patch

[Chris Dumez]

Created attachment 278591 [details]
Patch

[Chris Dumez]

Created attachment 278594 [details]
Patch

[Darin Adler]

Comment on attachment 278594 [details]
Patch

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

I noticed some things while reading the patch, not sure that you need to address most of them though.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:120
> +        setDOMException(&state, TypeError);

This should use throwTypeError instead of setDOMException.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:121
> +        return jsNull();

Peculiar that we use null here instead of using undefined like we do everywhere else. Not that it matters; it’s a bug if the caller looks at this value. But the inconsistency bothers me.

I kind of wish for all these uses there was a function or constant that isn’t specific about what value it returns but it’s just “the value that’s OK to return when you have set an exception”. In debug builds maybe even return a "poison" value that we assert is never used by anyone for anything. And then in release builds we’d just return whatever is most efficient.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:127
> +        return jsNull();

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:141
> +        setDOMException(&state, TypeError);
> +        return jsNull();

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:147
> +        return jsNull();

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:160
> +        setDOMException(&state, TypeError);
> +        return jsNull();

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:169
> +    ASSERT(!ec);

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:179
> +        setDOMException(&state, TypeError);
> +        return jsNull();

Ditto.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:185
> +        return jsNull();

Ditto.

> Source/WebCore/dom/Node.idl:67
> +    [ObjCLegacyUnnamedParameters, Custom, RaisesException] Node insertBefore([CustomReturn] Node newChild, Node? refChild);
> +    [ObjCLegacyUnnamedParameters, Custom, RaisesException] Node replaceChild(Node newChild, [CustomReturn] Node oldChild);
> +    [Custom, RaisesException] Node removeChild([CustomReturn] Node oldChild);
> +    [Custom, RaisesException] Node appendChild([CustomReturn] Node newChild);

What’s the significance of the [CustomReturn] extended attribute? Is that present just so that we can, in the future, stop using [Custom] and still have this do the right thing? Or is it just matching the specification, but using a keyword that our bindings generator ignores because we write these as [Custom] instead?

> Source/WebCore/dom/NodeOrString.cpp:59
> +        nodeToReturn->appendChild(node, ec);
> +        if (ec)
> +            return nullptr;

Pre-existing problem: Somewhere in this function, someone has to set ec to zero at least once if we are going to check the value of ec after calling a function. Otherwise, this dangerously depends on appendChild setting ec to zero when it succeeds, which is not guaranteed by our exception code calling convention. Alternatively, that could change that convention.

> Source/WebCore/dom/Range.cpp:766
> +                clonedAncestor->appendChild(*clonedContainer, ec);

Pre-existing problem: No exit here if this raises an exception. Why is that OK? Should this be passing ASSERT_NO_EXCEPTION instead of ec?

> Source/WebCore/dom/Range.cpp:783
> +                ancestor->removeChild(child, ec);

Pre-existing problem: No exit here if this raises an exception. Why is that OK? Should this be passing ASSERT_NO_EXCEPTION instead of ec?

> Source/WebCore/dom/Range.cpp:787
> +                    clonedContainer->appendChild(child, ec);

Pre-existing problem: No exit here if this raises an exception. Why is that OK? Should this be passing ASSERT_NO_EXCEPTION instead of ec?

> Source/WebCore/dom/Range.cpp:789
> +                    clonedContainer->insertBefore(child, clonedContainer->firstChild(), ec);

Pre-existing problem: No exit here if this raises an exception. Why is that OK? Should this be passing ASSERT_NO_EXCEPTION instead of ec?

Same below in a couple more call sites.

> Source/WebCore/editing/AppendNodeCommand.cpp:54
> +    m_parent->appendChild(m_node, IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION right or should it be ASSERT_NO_EXCEPTION, or what?

Maybe we should have a different internal version of these functions to be used in the ASSERT_NO_EXCEPTION cases?

> Source/WebCore/editing/Editor.cpp:627
> +            element.remove(ec);

Pre-existing issue: Why is it OK to ignore ec here?

> Source/WebCore/editing/Editor.cpp:630
> +        context->selectNodeContents(element, ec);

Pre-existing issue: Why is it OK to ignore ec here?

> Source/WebCore/editing/Editor.cpp:631
> +        element.appendChild(createFragmentFromText(context, text), ec);

Pre-existing issue: Why is it OK to ignore ec here?

> Source/WebCore/editing/Editor.cpp:636
> +                parent->insertBefore(element, siblingAfter.get(), ec);

Pre-existing issue: Why is it OK to ignore ec here?

> Source/WebCore/editing/Editor.cpp:638
> +                parent->appendChild(element, ec);

Pre-existing issue: Why is it OK to ignore ec here?

> Source/WebCore/editing/SplitElementCommand.cpp:67
> +        m_element1->appendChild(child, ec);

Is ignoring the exception the right thing here? Earlier in the function it exits after an exception.

> Source/WebCore/editing/SplitElementCommand.cpp:89
> +        m_element2->insertBefore(child, refChild.get(), IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION correct?

> Source/WebCore/editing/WrapContentsInDummySpanCommand.cpp:47
> +        m_dummySpan->appendChild(child, IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION correct?

> Source/WebCore/editing/WrapContentsInDummySpanCommand.cpp:49
>      m_element->appendChild(*m_dummySpan, IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION correct?

> Source/WebCore/editing/WrapContentsInDummySpanCommand.cpp:69
> +        m_element->appendChild(child, IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION correct?

> Source/WebCore/editing/WrapContentsInDummySpanCommand.cpp:71
>      m_dummySpan->remove(IGNORE_EXCEPTION);

Is IGNORE_EXCEPTION instead of ASSERT_NO_EXCEPTION correct?

> Source/WebCore/html/FTPDirectoryDocument.cpp:113
> +    rowElement->appendChild(typeElement, IGNORE_EXCEPTION);

I’m going to stop commenting on these. There are simply too many of them. No idea why IGNORE_EXCEPTION is the right thing.

> Source/WebCore/html/HTMLOptionElement.cpp:74
> +    auto text = Text::create(document, data.isNull() ? "" : data);

emptyString() instead of ""?

[Chris Dumez]

Comment on attachment 278594 [details]
Patch

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

>> Source/WebCore/bindings/js/JSNodeCustom.cpp:120
>> +        setDOMException(&state, TypeError);
> 
> This should use throwTypeError instead of setDOMException.

Ok, I think I'll use return JSValue::decode(throwArgumentTypeError(*state, 0, "node", "Node", "insertBefore", "Node")); then to match the generated bindings

>> Source/WebCore/dom/Node.idl:67
>> +    [Custom, RaisesException] Node appendChild([CustomReturn] Node newChild);
> 
> What’s the significance of the [CustomReturn] extended attribute? Is that present just so that we can, in the future, stop using [Custom] and still have this do the right thing? Or is it just matching the specification, but using a keyword that our bindings generator ignores because we write these as [Custom] instead?

This is used by ObjC bindings to indicate that this Node parameter should be returned by the operation. Note that the implementation is returning a bool, not a Node.

>> Source/WebCore/dom/NodeOrString.cpp:59
>> +            return nullptr;
> 
> Pre-existing problem: Somewhere in this function, someone has to set ec to zero at least once if we are going to check the value of ec after calling a function. Otherwise, this dangerously depends on appendChild setting ec to zero when it succeeds, which is not guaranteed by our exception code calling convention. Alternatively, that could change that convention.

Ok, I think we could just rely on the boolean returned by appendChild()

>> Source/WebCore/html/HTMLOptionElement.cpp:74
>> +    auto text = Text::create(document, data.isNull() ? "" : data);
> 
> emptyString() instead of ""?

definitely.

[Darin Adler]

Comment on attachment 278594 [details]
Patch

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

>>> Source/WebCore/dom/Node.idl:67
>>> +    [Custom, RaisesException] Node appendChild([CustomReturn] Node newChild);
>> 
>> What’s the significance of the [CustomReturn] extended attribute? Is that present just so that we can, in the future, stop using [Custom] and still have this do the right thing? Or is it just matching the specification, but using a keyword that our bindings generator ignores because we write these as [Custom] instead?
> 
> This is used by ObjC bindings to indicate that this Node parameter should be returned by the operation. Note that the implementation is returning a bool, not a Node.

I was aware of the special behavior, but I was not aware that [CustomReturn] was for Objective-C. Really unclear that [Custom] affects JavaScript but not Objective-C, and that [CustomReturn] affects Objective-C but not JavaScript!

[Chris Dumez]

Comment on attachment 278594 [details]
Patch

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

>>>> Source/WebCore/dom/Node.idl:67
>>>> +    [Custom, RaisesException] Node appendChild([CustomReturn] Node newChild);
>>> 
>>> What’s the significance of the [CustomReturn] extended attribute? Is that present just so that we can, in the future, stop using [Custom] and still have this do the right thing? Or is it just matching the specification, but using a keyword that our bindings generator ignores because we write these as [Custom] instead?
>> 
>> This is used by ObjC bindings to indicate that this Node parameter should be returned by the operation. Note that the implementation is returning a bool, not a Node.
> 
> I was aware of the special behavior, but I was not aware that [CustomReturn] was for Objective-C. Really unclear that [Custom] affects JavaScript but not Objective-C, and that [CustomReturn] affects Objective-C but not JavaScript!

Oh, You're right that [CustomReturn] may be useless if we use [Custom] already. I'll double check. I am not super familiar with ObjC bindings.

[Chris Dumez]

Comment on attachment 278594 [details]
Patch

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

>>>>> Source/WebCore/dom/Node.idl:67
>>>>> +    [Custom, RaisesException] Node appendChild([CustomReturn] Node newChild);
>>>> 
>>>> What’s the significance of the [CustomReturn] extended attribute? Is that present just so that we can, in the future, stop using [Custom] and still have this do the right thing? Or is it just matching the specification, but using a keyword that our bindings generator ignores because we write these as [Custom] instead?
>>> 
>>> This is used by ObjC bindings to indicate that this Node parameter should be returned by the operation. Note that the implementation is returning a bool, not a Node.
>> 
>> I was aware of the special behavior, but I was not aware that [CustomReturn] was for Objective-C. Really unclear that [Custom] affects JavaScript but not Objective-C, and that [CustomReturn] affects Objective-C but not JavaScript!
> 
> Oh, You're right that [CustomReturn] may be useless if we use [Custom] already. I'll double check. I am not super familiar with ObjC bindings.

Well, I have confirmed it does not build without them.

[Chris Dumez]

I'd rather not touch the IGNORE_EXCEPTION / ASSERT_NO_EXCEPTION in this patch as this is riskier and the patch is already large.

[Chris Dumez]

Created attachment 278632 [details]
Patch

[Build Bot]

Comment on attachment 278632 [details]
Patch

Attachment 278632 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1304786

New failing tests:
fast/events/remove-target-with-shadow-in-drag.html

[Build Bot]

Created attachment 278639 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278632 [details]
Patch

Attachment 278632 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1304797

New failing tests:
fast/events/remove-target-with-shadow-in-drag.html

[Build Bot]

Created attachment 278641 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 278649 [details]
Patch

[Chris Dumez]

Comment on attachment 278649 [details]
Patch

Clearing flags on attachment: 278649

Committed r200696: <http://trac.webkit.org/changeset/200696>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Looks like a 2% progression on Dromaeo DOM Core on the Mac bots (mostly due to the Dromaeo DOM Modification subtest which covers removeChild() / appendChild() and insertBefore()).

[Darin Adler]

(In reply to comment #18)
> Looks like a 2% progression on Dromaeo DOM Core on the Mac bots

!!!