Bug 172407 – [WebIDL] Support callbacks with arbitrary return types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Support callbacks with arbitrary return types

[Sam Weinig]

Created attachment 310745 [details]
For the bots

[Build Bot]

Attachment 310745 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310745 [details]
For the bots

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

New failing tests:
Please see test output for results

[Build Bot]

Comment on attachment 310745 [details]
For the bots

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

New failing tests:
imported/w3c/web-platform-tests/dom/traversal/NodeIterator.html
http/tests/misc/acid3.html
fast/dom/node-filter-detached-iframe-crash.html
fast/dom/TreeWalker/acceptNode-filter.html
imported/w3c/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html
traversal/exception-forwarding.html
fast/frames/frame-window-as-callback.html
fast/dom/TreeWalker/filter-throw.html

[Build Bot]

Created attachment 310749 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 310745 [details]
For the bots

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

New failing tests:
imported/w3c/web-platform-tests/dom/traversal/NodeIterator.html
http/tests/misc/acid3.html
fast/dom/node-filter-detached-iframe-crash.html
fast/dom/TreeWalker/acceptNode-filter.html
imported/w3c/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html
traversal/exception-forwarding.html
fast/frames/frame-window-as-callback.html
fast/dom/TreeWalker/filter-throw.html

[Build Bot]

Created attachment 310754 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[Build Bot]

Comment on attachment 310745 [details]
For the bots

Attachment 310745 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3780563

New failing tests:
imported/w3c/web-platform-tests/dom/traversal/NodeIterator.html
fast/dom/node-filter-detached-iframe-crash.html
fast/dom/TreeWalker/acceptNode-filter.html
imported/w3c/web-platform-tests/dom/traversal/TreeWalker-acceptNode-filter.html
traversal/exception-forwarding.html
fast/frames/frame-window-as-callback.html
fast/dom/TreeWalker/filter-throw.html
fetch/closing-while-fetching-blob.html

[Build Bot]

Created attachment 310755 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 310756 [details]
Patch

[Build Bot]

Attachment 310756 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310756 [details]
Patch

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

New failing tests:
Please see test output for results

[Sam Weinig]

Created attachment 310758 [details]
Patch

[Build Bot]

Attachment 310758 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310758 [details]
Patch

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

New failing tests:
Please see test output for results

[Sam Weinig]

Ignore the bindings bot. The patch is not applying correctly because our tools can't handle moves / copies correctly still.

[Sam Weinig]

Created attachment 310762 [details]
Patch

[Build Bot]

Attachment 310762 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310762 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/dom/traversal/NodeIterator.html
traversal/exception-forwarding.html
http/tests/misc/acid3.html

[Build Bot]

Created attachment 310767 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[Sam Weinig]

Ok, I figured out the issue. In a test like:

function test(node)
{
    throw "1";
}

var i = document.createNodeIterator(document.documentElement, 0xFFFFFFFF, test, true);
i.nextNode()

The first time JSNodeFilter::acceptNode() is called, we call test(), see the exception and re-throw it. Then, without going back to JS, we have another call to JSNodeFilter::acceptNode(). Since there is still and exception pending, invoking the callback asserts under JSC::ExceptionScope::assertNoException(). 

In the old code, we got lucky, because there was a RETURN_IF_EXCEPTION(scope, NodeFilter::FILTER_REJECT); after the call to convert the passed in Node to JS, but that should never have fired because toJS for Node doesn't throw. However, it was acting as a bail out point.

I think the correct move here would be to have NodeIterator and TreeWalker (the callers of NodeFilter::acceptNode(), bail if an exception is thrown, and not try to call acceptNode again.

Darin, I would be interested in your thoughts on this, as you have worked in this wonderful area of traversal before.

[Sam Weinig]

Created attachment 310777 [details]
Patch

[Sam Weinig]

This version keeps the NodeFilter binding custom, but adds all the infrastructure needed for it. In a follow up, I will update the NodeIterator and TreeWalker to handle propagating the exception correctly.

[Build Bot]

Attachment 310777 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Here’s my take:

1) When DOM code calls JavaScript, we typically absorb the JavaScript exception, clear it from JavaScript execution state, and report it directly to the user through the web inspector. But when JavaScript code calls DOM code, which in turn then calls JavaScript, that behavior isn’t quite right. Instead we want to package up the exception and send it on through to the calling JavaScript. The way NodeFilter does this is "just leave the JavaScript exception behind in the VM state". I think that’s a bit sloppy since it means all NodeFilter callers have to guarantee that someone is going to handle or clear that JavaScript exception.

2) When DOM code calls JavaScript, we typically ignore the JavaScript exception. But that is not the correct behavior for NodeFilter and TreeWalker. We don’t want them to keep iterating the tree if they were called from JavaScript and then called back into JavaScript and have a JavaScript exception left behind in the VM state.

I am pretty sure that we should change acceptNode to return an indication that an exception occurred, rather than requiring that callers inspect the VM to see if an exception is present.

If we want to keep the code almost the same as it currently is, then the return value could simply indicate that an exception occurred. Probably should *not* be a magic value for the unsigned short return value, instead something out of band using a separate return value, or std::expected, or whatever. Callers would need to stop iterating when the exception occurred. The exception itself would be left behind in the VM. This leaves two open questions:

a) How would the bindings for nextNode and previousNode be told that a JavaScript exception might be set as a side effect? Do all bindings already work with that behavior or do we need a custom IDL property to tell the bindings generator to properly handle a possibly-set exception?

b) When calling nextNode or previousNode from something other than the JavaScript bindings, what code will take responsibility for clearing out the the exception from the VM and reporting it to the web inspector if the filter happened to be a JavaScript one? All callers of nextNode and previousNode would have to take on this responsibility unless it’s OK to leave behind an unhandled exception in the VM.

If we want to change code more, then we could change acceptNode to return the exception it encountered instead of re-throwing it. The trick there is to figure out how we wrap a JavaScript exception inside the DOM so that it can be re-thrown. We would pass this exception up from acceptNode to nextNode and previousNode, and they would return this wrapped exception, which would be unwrapped by the nextNode and previousNode bindings.

[Sam Weinig]

Created attachment 310780 [details]
Patch for review

[Build Bot]

Attachment 310780 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 310787 [details]
Patch with NodeIterator/TreeWalker change as well

[Build Bot]

Attachment 310787 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Thanks so much for the detailed response Darin! I fortunately already have (well, in the sense that it is up for review) all javascript callbacks returning a new CallbackResult<> type (loosely modeled on ExceptionOr) that encapsulates the return value or a problem case (right now there are two, enabled to execute at all and exception thrown).

Since that was in place, all that was left was to change NodeIterator/TreeWalker's IDLs to have [MayThrowException] on all the functions that call acceptNode(), add checks after calling acceptNode() in those functions, and return Exception { ExistingExceptionError }. The bindings already know what to do by way of propagateException() having an early return if there is an exception on the stack.

The one piece of the puzzle I don't think I got right was the Objective-C bindings (how ironic?) I asserted (both in the ASSERT sense, and in prose in the ChangeLog) that the DOMNodeIterator and DOMTreeWalker could never have a JSNodeFilter as their filter, so we could ignore the exception part of the ExceptionOr that the WebCore implementation functions now return. But, I'm pretty sure they are WebScriptObjects, so I think you could get a JavaScript land NodeIterator and convert it to a DOMNodeIterator. Honestly, it's been so long I can't remember if that's possible. My point is, maybe instead of ASSERTing I should return nil / throw and objective-c exception?

[Sam Weinig]

Created attachment 310793 [details]
Patch with NodeIterator/TreeWalker change as well

[Build Bot]

Attachment 310793 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorDatabaseAgent.cpp:177:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 310811 [details]
Patch with NodeIterator/TreeWalker change as well

[Sam Weinig]

Created attachment 310815 [details]
Patch with NodeIterator/TreeWalker change as well

[Sam Weinig]

I think the complete patch is good to go now.

[Chris Dumez]

Comment on attachment 310815 [details]
Patch with NodeIterator/TreeWalker change as well

OMG this is great :)

[WebKit Commit Bot]

Comment on attachment 310815 [details]
Patch with NodeIterator/TreeWalker change as well

Clearing flags on attachment: 310815

Committed r217237: <http://trac.webkit.org/changeset/217237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.