[WebIDL] Support callbacks with arbitrary return types
Created attachment 310745 [details] For the bots
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.
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
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
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
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
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
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
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
Created attachment 310756 [details] Patch
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.
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
Created attachment 310758 [details] Patch
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.
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
Ignore the bindings bot. The patch is not applying correctly because our tools can't handle moves / copies correctly still.
Created attachment 310762 [details] Patch
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.
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
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
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.
Created attachment 310777 [details] Patch
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.
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.
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.
Created attachment 310780 [details] Patch for review
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.
Created attachment 310787 [details] Patch with NodeIterator/TreeWalker change as well
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.
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?
Created attachment 310793 [details] Patch with NodeIterator/TreeWalker change as well
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.
Created attachment 310811 [details] Patch with NodeIterator/TreeWalker change as well
Created attachment 310815 [details] Patch with NodeIterator/TreeWalker change as well
I think the complete patch is good to go now.
Comment on attachment 310815 [details] Patch with NodeIterator/TreeWalker change as well OMG this is great :)
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>
All reviewed patches have been landed. Closing bug.