Bug 234568

Summary: [WebIDL] document.body.onerror setter should setup a five-parameter listener
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: BindingsAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: ahmad.saleem792, cdumez, changseok, clopez, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, kondapallykalyan, webkit-bug-importer, youennf
Priority: P2 Keywords: BrowserCompat, InRadar, WPTImpact
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://github.com/web-platform-tests/wpt/pull/32164
Attachments:
Description Flags
Patch
none
Patch none

Alexey Shvayka
Reported 2021-12-21 10:37:38 PST
[WebIDL] document.body.onerror setter should setup a five-parameter listener
Attachments
Patch (20.96 KB, patch)
2021-12-21 10:41 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (25.26 KB, patch)
2021-12-21 16:21 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Shvayka
Comment 1 2021-12-21 10:41:54 PST
Created attachment 447733 [details] Patch
EWS Watchlist
Comment 2 2021-12-21 10:43:27 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Alexey Shvayka
Comment 3 2021-12-21 10:44:11 PST
Hey Darin, this change follows-up your FIXME added in https://bugs.webkit.org/show_bug.cgi?id=142282. I would appreciate a feedback!
Darin Adler
Comment 4 2021-12-21 11:50:15 PST
Comment on attachment 447733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447733&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5437 > + my $isOnErrorEventHandler = $attribute->name eq "onerror" && ($interface->type->name eq "DOMWindow" || $interface->type->name eq "WorkerGlobalScope" || $attribute->extendedAttributes->{WindowEventHandler}); I still would like to do this by using OnErrorEventHandler and OnBeforeUnloadEventHandler in the IDL files rather than hard-coding the attribute and class names here. This change looks great to me, but I think we should do it by adding an understanding of the names OnErrorEventHandler and OnBeforeUnloadEventHandler. There are only four places in the entire code generator where the name EventHandler appears, so that is the maximum number of call sites we’d have to change to use a shared function that understands these three different EventHandler names that exist, or special case all type names with that end with an EventHandler suffix instead of comparing the whole type name. Then, I would still suggest we keep the rest of the code you wrote the same; this one line of code would be different. > Source/WebCore/dom/GlobalEventHandlers.idl:55 > + attribute EventHandler onerror; // OnErrorEventHandler semantics is added in GenerateAttributeSetterBodyDefinition We should write OnErrorEventHandler here in the IDL file and fix the code generator to understand what it means. Is there a good reason not to do that? If it’s really helpful to not write OnErrorEventHandler as the type, we could consider an extended attribute named [OnErrorEventHandler] instead, but that seems unnecessary. One of the ways you can tell this is a good idea is that we end up without having to write a comment explaining ourselves! > Source/WebCore/page/WindowEventHandlers.idl:31 > + [WindowEventHandler] attribute EventHandler onbeforeunload; // OnBeforeUnloadEventHandler semantics is added in JSEventListener::handleEvent() Ditto. > Source/WebCore/workers/WorkerGlobalScope.idl:38 > + attribute EventHandler onerror; // OnErrorEventHandler semantics is added in GenerateAttributeSetterBodyDefinition Ditto.
Alexey Shvayka
Comment 5 2021-12-21 12:41:31 PST
(In reply to Darin Adler from comment #4) > We should write OnErrorEventHandler here in the IDL file and fix the code > generator to understand what it means. Is there a good reason not to do > that? I would say it's not practical to do that considering the necessary changes to functions like ProcessInterfaceSupplementalDependencies. Let's consider the changes for suggested approach: 1. Add attribute `OnErrorEventHandler onerror` to the DOMWindow.idl. We can't just amend the type of `onerror` in GlobalEventHandlers (like the spec does) because other receivers other than Window / Worker / <body> / <frameset> should setup single-parameter listener. This is fine though. 2. Teach code generator to support type names ending with EventHandler suffix => also neat and fine. Now we get a compilation error because there are two `onerror` accessors on DOMWindow. 3. We now need to teach ProcessInterfaceSupplementalDependencies to ignore an `ErrorHandler onerror` attribute from supplemental interface if there is a `OnErrorEventHandler onerror`. That won't be neat and I would rather prefer a hard-coded type name check, especially since it's nicely matches the spec, then somewhat weird merging. Same effort would be needed for using an [OnErrorEventHandler] extended attribute rather than a return type.
Darin Adler
Comment 6 2021-12-21 13:59:49 PST
How do the WebIDL specification and the other specifications deal with this conflict between onerror in GlobalEventhandlers and in DOMWindow with two different types?
Alexey Shvayka
Comment 7 2021-12-21 14:19:44 PST
(In reply to Darin Adler from comment #6) > How do the WebIDL specification and the other specifications deal with this > conflict between onerror in GlobalEventhandlers and in DOMWindow with two > different types? `onerror` in GlobalEventHandlers has a OnErrorEventHandler type, expecting a five-parameter function, which is pretty confusing given that a regular listener is invoked for most receivers. WebIDL has no mechanism of validating / rejecting a user land function by signature, so that OnErrorEventHandler accepts a regular listener. However, when any event handler is called, if it's an ErrorEvent with "error" type fired on Window / Worker (step 3 of https://html.spec.whatwg.org/#the-event-handler-processing-algorithm), it's called with 5 arguments instead of 1. Unlike the spec, WebKit attempts to do less work in runtime, hence the JSErrorHandler and the additional code generation efforts.
Darin Adler
Comment 8 2021-12-21 14:41:19 PST
Given this history, I suggest we specify OnErrorEventHandler in both places, do the 1-argument version for efficiency by default and use an extended attribute on the interfaces that want the full 5-argument form. This won’t be hard to do. If you prefer to have me make that change I will be happy to. I think it’s even better to do it that way than the way you’ve done it here.
Alexey Shvayka
Comment 9 2021-12-21 14:52:30 PST
(In reply to Darin Adler from comment #8) > Given this history, I suggest we specify OnErrorEventHandler in both places, > do the 1-argument version for efficiency by default and use an extended > attribute on the interfaces that want the full 5-argument form. Hmm, I don't see how that saves us from implementing the weird attribute merging. Unless we put an extended attribute on the Window / Worker / <body> / <frameset> interfaces itself, which doesn't seem better than hardcoding their names. I'm happy do the change myself! === As for OnBeforeUnloadEventHandler, there is no conflict of signatures for it. It's handled in JSEventListener::handleEvent() only for convenience. We can put a OnBeforeUnloadEventHandler type to the IDL, and make a subclass of JSEventListener, but is there a practical benefit to such a change?
Darin Adler
Comment 10 2021-12-21 14:54:00 PST
(In reply to Alexey Shvayka from comment #9) > Unless we put an extended attribute on the Window / Worker / <body> > / <frameset> interfaces itself, which doesn't seem better than hardcoding > their names. Aha, we have found it, this the key point on which we differ. I do think that putting the extended attribute on these 4 interfaces is better than hard-coding their names.
Alexey Shvayka
Comment 11 2021-12-21 15:48:25 PST
(In reply to Darin Adler from comment #10) > Aha, we have found it, this the key point on which we differ. I do think > that putting the extended attribute on these 4 interfaces is better than > hard-coding their names. Seems like I came up with a solution we both will be happy with. Exporting the WPT while it compiles...
Alexey Shvayka
Comment 12 2021-12-21 16:21:33 PST
Created attachment 447762 [details] Patch Introduce IsEventHandlerType helper, remove the hard-coding of interface names, and align IDLs with the spec.
Radar WebKit Bug Importer
Comment 13 2021-12-28 10:38:18 PST
<rdar://problem/86959987>
Alexey Shvayka
Comment 14 2022-01-07 14:01:00 PST
Comment on attachment 447762 [details] Patch Hey Darin, Does the updated patch look to you like something we can land w/o a follow-up? I appreciate all your feedback! View in context: https://bugs.webkit.org/attachment.cgi?id=447762&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5437 > + my $eventListenerType = $attribute->type->name eq "OnErrorEventHandler" && (IsDOMGlobalObject($interface) || $attribute->extendedAttributes->{WindowEventHandler}) ? "JSErrorHandler" : "JSEventListener"; Usage of IsDOMGlobalObject here is forward-compatible & safe because there will be no EventHandler's introduced in new specs, as those are considered legacy, especially the ones of OnErrorEventHandler type. I will make sure to note this in ChangeLog.
Darin Adler
Comment 15 2022-01-07 16:31:42 PST
(In reply to Alexey Shvayka from comment #14) > Does the updated patch look to you like something we can land w/o a > follow-up? Yes. > I appreciate all your feedback! Thanks; I find it fun to try to tame the code generation code a bit and make it simpler over time. Cutting down special cases. And also, relying a bit more on C++ techniques that were not available when it was originally written. Separately, maybe someone should port it to Python at some point.
Ahmad Saleem
Comment 16 2023-05-12 08:24:42 PDT
*** Bug 256484 has been marked as a duplicate of this bug. ***
Alexey Shvayka
Comment 17 2023-05-17 13:43:46 PDT
Pull request: https://github.com/WebKit/WebKit/pull/13983
EWS
Comment 18 2023-05-17 21:28:49 PDT
Committed 264190@main (cf847edd8255): <https://commits.webkit.org/264190@main> Reviewed commits have been landed. Closing PR #13983 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.