Bug 172407 - [WebIDL] Support callbacks with arbitrary return types
Summary: [WebIDL] Support callbacks with arbitrary return types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-19 18:45 PDT by Sam Weinig
Modified: 2017-05-22 12:27 PDT (History)
5 users (show)

See Also:


Attachments
For the bots (98.75 KB, patch)
2017-05-19 18:48 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.19 MB, application/zip)
2017-05-19 19:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.67 MB, application/zip)
2017-05-19 20:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (927.13 KB, application/zip)
2017-05-19 20:17 PDT, Build Bot
no flags Details
Patch (112.96 KB, patch)
2017-05-19 20:45 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (112.76 KB, patch)
2017-05-19 21:21 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (113.32 KB, patch)
2017-05-19 22:32 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.74 MB, application/zip)
2017-05-20 00:07 PDT, Build Bot
no flags Details
Patch (110.47 KB, patch)
2017-05-20 11:39 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch for review (110.52 KB, patch)
2017-05-20 12:42 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch with NodeIterator/TreeWalker change as well (141.80 KB, patch)
2017-05-20 19:40 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch with NodeIterator/TreeWalker change as well (142.01 KB, patch)
2017-05-20 22:07 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch with NodeIterator/TreeWalker change as well (150.23 KB, patch)
2017-05-21 11:04 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch with NodeIterator/TreeWalker change as well (150.23 KB, patch)
2017-05-21 11:38 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-05-19 18:45:16 PDT
[WebIDL] Support callbacks with arbitrary return types
Comment 1 Sam Weinig 2017-05-19 18:48:32 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-05-19 18:50:33 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2017-05-19 18:51:13 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-05-19 19:45:39 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-05-19 19:45:40 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-05-19 20:15:41 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-05-19 20:15:42 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-05-19 20:17:51 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-05-19 20:17:53 PDT Comment hidden (obsolete)
Comment 10 Sam Weinig 2017-05-19 20:45:21 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-05-19 20:47:30 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-05-19 20:48:20 PDT Comment hidden (obsolete)
Comment 13 Sam Weinig 2017-05-19 21:21:33 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-05-19 21:23:56 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-05-19 21:25:17 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2017-05-19 21:49:28 PDT Comment hidden (obsolete)
Comment 17 Sam Weinig 2017-05-19 22:32:16 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-05-19 22:33:33 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-05-20 00:07:45 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2017-05-20 00:07:46 PDT Comment hidden (obsolete)
Comment 21 Sam Weinig 2017-05-20 10:12:47 PDT
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.
Comment 22 Sam Weinig 2017-05-20 11:39:41 PDT Comment hidden (obsolete)
Comment 23 Sam Weinig 2017-05-20 11:40:52 PDT
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.
Comment 24 Build Bot 2017-05-20 11:42:36 PDT Comment hidden (obsolete)
Comment 25 Darin Adler 2017-05-20 11:54:53 PDT
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.
Comment 26 Sam Weinig 2017-05-20 12:42:04 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2017-05-20 12:44:25 PDT Comment hidden (obsolete)
Comment 28 Sam Weinig 2017-05-20 19:40:06 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2017-05-20 19:41:38 PDT Comment hidden (obsolete)
Comment 30 Sam Weinig 2017-05-20 19:50:08 PDT
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?
Comment 31 Sam Weinig 2017-05-20 22:07:41 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2017-05-20 22:10:17 PDT Comment hidden (obsolete)
Comment 33 Sam Weinig 2017-05-21 11:04:39 PDT Comment hidden (obsolete)
Comment 34 Sam Weinig 2017-05-21 11:38:49 PDT
Created attachment 310815 [details]
Patch with NodeIterator/TreeWalker change as well
Comment 35 Sam Weinig 2017-05-21 18:26:53 PDT
I think the complete patch is good to go now.
Comment 36 Chris Dumez 2017-05-22 11:59:07 PDT
Comment on attachment 310815 [details]
Patch with NodeIterator/TreeWalker change as well

OMG this is great :)
Comment 37 WebKit Commit Bot 2017-05-22 12:27:48 PDT
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>
Comment 38 WebKit Commit Bot 2017-05-22 12:27:50 PDT
All reviewed patches have been landed.  Closing bug.