Bug 157556 - Update Node::appendChild() / replaceChild() / removeChild() / insertBefore() to take references instead of pointers
Summary: Update Node::appendChild() / replaceChild() / removeChild() / insertBefore() ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-10 20:24 PDT by Chris Dumez
Modified: 2016-05-12 09:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (145.38 KB, patch)
2016-05-10 22:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (141.22 KB, patch)
2016-05-10 22:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (141.49 KB, patch)
2016-05-10 23:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (150.39 KB, patch)
2016-05-11 10:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (799.87 KB, application/zip)
2016-05-11 11:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.39 MB, application/zip)
2016-05-11 11:34 PDT, Build Bot
no flags Details
Patch (151.13 KB, patch)
2016-05-11 12:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-05-10 20:24:46 PDT
Update Node::appendChild() / replaceChild() / removeChild() / insertBefore() to references instead of pointers.
Comment 1 Chris Dumez 2016-05-10 22:03:09 PDT
Created attachment 278585 [details]
Patch
Comment 2 Chris Dumez 2016-05-10 22:59:39 PDT
Created attachment 278591 [details]
Patch
Comment 3 Chris Dumez 2016-05-10 23:14:18 PDT
Created attachment 278594 [details]
Patch
Comment 4 Darin Adler 2016-05-11 08:55:37 PDT
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 ""?
Comment 5 Chris Dumez 2016-05-11 09:14:42 PDT
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.
Comment 6 Darin Adler 2016-05-11 09:16:39 PDT
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!
Comment 7 Chris Dumez 2016-05-11 09:18:07 PDT
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.
Comment 8 Chris Dumez 2016-05-11 09:22:18 PDT
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.
Comment 9 Chris Dumez 2016-05-11 09:29:02 PDT
I'd rather not touch the IGNORE_EXCEPTION / ASSERT_NO_EXCEPTION in this patch as this is riskier and the patch is already large.
Comment 10 Chris Dumez 2016-05-11 10:25:19 PDT
Created attachment 278632 [details]
Patch
Comment 11 Build Bot 2016-05-11 11:20:02 PDT
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
Comment 12 Build Bot 2016-05-11 11:20:05 PDT
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
Comment 13 Build Bot 2016-05-11 11:34:52 PDT
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
Comment 14 Build Bot 2016-05-11 11:34:55 PDT
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
Comment 15 Chris Dumez 2016-05-11 12:00:07 PDT
Created attachment 278649 [details]
Patch
Comment 16 Chris Dumez 2016-05-11 12:01:21 PDT
Comment on attachment 278649 [details]
Patch

Clearing flags on attachment: 278649

Committed r200696: <http://trac.webkit.org/changeset/200696>
Comment 17 Chris Dumez 2016-05-11 12:01:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Chris Dumez 2016-05-12 09:41:42 PDT
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()).
Comment 19 Darin Adler 2016-05-12 09:47:00 PDT
(In reply to comment #18)
> Looks like a 2% progression on Dromaeo DOM Core on the Mac bots

!!!