Bug 163016 – Next step on moving to modern way to return DOM exceptions

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Next step on moving to modern way to return DOM exceptions

[Darin Adler]

It’s great to get rid of the messy situation where you have to come up with both a return value and an exception value. I’ve got a patch working well and I am writing the release notes now and putting final touches on it and will probably post it for review tonight.

I will want a little feedback on the stylistic aspects of the patch such as:

- Does the code in functions that can return an exception look good enough?

- I chose the name "toJS" for the function that takes an ExceptionOr<void> and throws the exception if present. Should I use the name throw or throwException instead? Or maybe setDOMException. Unlike the existing setDOMException, though, the argument type makes it unambiguous what type of exception we are talking about.

- I chose to use roughly the same names for functions that take ExceptionOr<T> and either convert a return type or throw an exception as for the functions that convert the return type, so the names are toJS, toJSNewlyCreated, toJSNumber, etc. Are those names OK?

And I am not yet sure if this is going to be efficient enough to replace the old technique for super-performance-critical DOM functions.

[Darin Adler]

Created attachment 290881 [details]
Patch

[Build Bot]

Comment on attachment 290881 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
imported/w3c/web-platform-tests/XMLHttpRequest/responsetype.html

[Build Bot]

Created attachment 290884 [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 290881 [details]
Patch

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
imported/w3c/web-platform-tests/XMLHttpRequest/responsetype.html

[Build Bot]

Created attachment 290885 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Darin Adler]

Created attachment 290886 [details]
Patch

[Darin Adler]

Test failures fixed, EWS all green, so ready for review.

[Darin Adler]

Someone should review this so I can move on to the next step of this exception work.

[Saam Barati]

Comment on attachment 290886 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:380
> +void toJS(JSC::ExecState&, JSC::ThrowScope&, ExceptionOr<void>&&);

I personally like some of the other names you suggested for this function in your previous comment. Another suggestion: "propagateException" or some variant of that. I'll continue to review the patch when I'm in front of my computer and not on my phone unless somebody beats me to it.

[Ryosuke Niwa]

Comment on attachment 290886 [details]
Patch

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

> Source/WebCore/dom/TreeScope.h:-92
> -    DOMSelection* getSelection() const;
> -

getSelection() was here because the shadow DOM spec says we need to have selection in each shadow root:
https://w3c.github.io/webcomponents/spec/shadow/#extensions-to-the-documentorshadowroot-mixin
but it's fine to move it here for now until I can come back and fix it for real since a whole bunch of things are broken with selection in shadow trees.

> Source/WebCore/page/DOMSelection.cpp:-205
>      if (!m_frame)
> -        return;

I don't think we can want to start throwing INVALID_STATE_ERR when m_frame is nullptr.

> Source/WebCore/page/DOMSelection.cpp:-220
>      if (!m_frame)
> -        return;

Ditto about throwing INVALID_STATE_ERR when m_frame is nullptr.

> Source/WebCore/page/DOMSelection.cpp:271
> +    if (!m_frame || offset > (node.offsetInCharacters() ? caretMaxOffset(node) : node.countChildNodes()))
> +        return Exception { INDEX_SIZE_ERR };

Ditto about throwing when m_frame is null.

> Source/WebCore/page/DOMSelection.cpp:273
> +        return { }; // FIXME: Why no exception in this case?

This is a spec'ed behavior:
http://w3c.github.io/selection-api/#widl-Selection-extend-void-Node-node-unsigned-long-offset

> Source/WebCore/page/DOMSelection.cpp:-333
> -    if (!m_frame)
> -        return nullptr;

I don't think we can get rid of m_frame check here.

> Source/WebCore/xml/XMLHttpRequest.cpp:509
> -void XMLHttpRequest::send(ExceptionCode& ec)
> +ExceptionOr<void> XMLHttpRequest::send()

Should we just add a default value to the version which takes a String instead of having this function?

[youenn fablet]

Comment on attachment 290886 [details]
Patch

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

> Source/WebCore/ChangeLog:89
> +        out a way to do it for functions without return values as well as functions with.

That is sad... do we really need to abandon it for attribute getters though?

[Darin Adler]

Comment on attachment 290886 [details]
Patch

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

>> Source/WebCore/ChangeLog:89
>> +        out a way to do it for functions without return values as well as functions with.
> 
> That is sad... do we really need to abandon it for attribute getters though?

Had a long chat with Chris Dumez about this. After much consideration the two of us think it is too confusing to have it be different for getters and functions with return values and for setters and functions without return values; but we could still debate that point.

However, Anders Carlsson is working on a solution, and if he succeeds we may return to not needing an extended attribute in the future. If so, we will then simply delete the extended attributes from all the IDL files.

[Darin Adler]

Comment on attachment 290886 [details]
Patch

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

Thanks, everyone, for the helpful review and comments.

>> Source/WebCore/bindings/js/JSDOMBinding.h:380
>> +void toJS(JSC::ExecState&, JSC::ThrowScope&, ExceptionOr<void>&&);
> 
> I personally like some of the other names you suggested for this function in your previous comment. Another suggestion: "propagateException" or some variant of that. I'll continue to review the patch when I'm in front of my computer and not on my phone unless somebody beats me to it.

I will use the name propagateException.

Also still interested in good ideas for names of the other functions below.

>> Source/WebCore/dom/TreeScope.h:-92
>> -
> 
> getSelection() was here because the shadow DOM spec says we need to have selection in each shadow root:
> https://w3c.github.io/webcomponents/spec/shadow/#extensions-to-the-documentorshadowroot-mixin
> but it's fine to move it here for now until I can come back and fix it for real since a whole bunch of things are broken with selection in shadow trees.

OK, thanks for explaining. I understand now that eventually you will be putting a getSelection function here. The current version of DOMSelection is not ready for this; we should not change the owner of DOMSelection without also changing how its back pointer works. It is tricky to have the pointer to the DOMSelection be in the TreeScope, but the back pointer from the DOMSelection be a pointer to the Frame.

>> Source/WebCore/page/DOMSelection.cpp:-205
>> -        return;
> 
> I don't think we can want to start throwing INVALID_STATE_ERR when m_frame is nullptr.

Sure, will fix. Simple and safe to keep the old behavior and not just change as an unstated side effect of this refactor, but I am concerned about functions that silently do nothing only in an unusual case like this without any mention of this in the specification, and some day we may indeed want to make a change here.

>> Source/WebCore/page/DOMSelection.cpp:-220
>> -        return;
> 
> Ditto about throwing INVALID_STATE_ERR when m_frame is nullptr.

OK, same resolution as above.

>> Source/WebCore/page/DOMSelection.cpp:271
>> +        return Exception { INDEX_SIZE_ERR };
> 
> Ditto about throwing when m_frame is null.

OK, same resolution as above.

>> Source/WebCore/page/DOMSelection.cpp:273
>> +        return { }; // FIXME: Why no exception in this case?
> 
> This is a spec'ed behavior:
> http://w3c.github.io/selection-api/#widl-Selection-extend-void-Node-node-unsigned-long-offset

Will remove the FIXME.

>> Source/WebCore/page/DOMSelection.cpp:-333
>> -        return nullptr;
> 
> I don't think we can get rid of m_frame check here.

The problem here is that getRangeAt is defined as returning a Range, not Range?, not nullable so returning null is unique behavior that can’t happen otherwise. If we want to preserve this behavior of returning null when the frame is null, we’ll have to change the IDL to say Range? and have the return type here be ExceptionOr<RefPtr<Range>>. I’d prefer not to change the interface of this function just because of this behavior. I think we can afford to change the behavior to raise an exception instead.

I think that once the frame is gone, having selection act as if nothing is selected is good behavior that should be compatible; keep in mind that rangeCount will return 0 in this case, consistent with raising INDEX_SIZE_ERR when this is called. In time we may want to consider further how the null m_frame case actually arises and what we want to do. Perhaps we want to support selection once the frame is gone, rather than having the selection object go strangely dead in that case? Certainly nothing in the cross platform specification talks about selection functions mysteriously doing nothing when code continues to run after the transition to a new webpage takes place or a browser window is closed. In time I think we might in fact want this to be specified for interoperability.

>> Source/WebCore/xml/XMLHttpRequest.cpp:509
>> +ExceptionOr<void> XMLHttpRequest::send()
> 
> Should we just add a default value to the version which takes a String instead of having this function?

Either way seems OK to me. I do see that getting rid of ExceptionCode& makes that possible. Since you suggested the change, I guess I will do it even though I think it’s a tie.

[Darin Adler]

Committed r206960: <http://trac.webkit.org/changeset/206960>

[Chris Dumez]

Comment on attachment 290886 [details]
Patch

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

>>> Source/WebCore/page/DOMSelection.cpp:-333
>>> -        return nullptr;
>> 
>> I don't think we can get rid of m_frame check here.
> 
> The problem here is that getRangeAt is defined as returning a Range, not Range?, not nullable so returning null is unique behavior that can’t happen otherwise. If we want to preserve this behavior of returning null when the frame is null, we’ll have to change the IDL to say Range? and have the return type here be ExceptionOr<RefPtr<Range>>. I’d prefer not to change the interface of this function just because of this behavior. I think we can afford to change the behavior to raise an exception instead.
> 
> I think that once the frame is gone, having selection act as if nothing is selected is good behavior that should be compatible; keep in mind that rangeCount will return 0 in this case, consistent with raising INDEX_SIZE_ERR when this is called. In time we may want to consider further how the null m_frame case actually arises and what we want to do. Perhaps we want to support selection once the frame is gone, rather than having the selection object go strangely dead in that case? Certainly nothing in the cross platform specification talks about selection functions mysteriously doing nothing when code continues to run after the transition to a new webpage takes place or a browser window is closed. In time I think we might in fact want this to be specified for interoperability.

This may have caused <rdar://problem/29931223>.

[Chris Dumez]

Comment on attachment 290886 [details]
Patch

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

> Source/WebCore/page/DOMSelection.cpp:292
> +    return m_frame->selection().selection().firstRange().releaseNonNull();

Or rather this part here. See https://bugs.webkit.org/show_bug.cgi?id=171925.