Bug 180019

Summary: [WebIDL] Remove the need to specify [MayThrowException]
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, calvaris, cdumez, changseok, cmarcelo, darin, dino, don.olmstead, drousso, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hta, japhet, jer.noble, jiewen_tan, joepeck, jsbell, kangil.han, kondapallykalyan, macpherson, menard, mifenton, pdr, philipj, sabouhallawa, schenney, sergio, shvaikalesh, tommyw, toyoshim, webkit-bug-importer, yutak
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 216429, 216441, 216442    
Bug Blocks: 177453    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Just the code changes (no IDL or test result changes)
none
Patch
none
Patch none

Description Sam Weinig 2017-11-26 11:48:08 PST
[WebIDL] Remove the need to specify [MayThrowException] for operations
Comment 1 Sam Weinig 2017-11-26 11:54:19 PST Comment hidden (obsolete)
Comment 2 EWS Watchlist 2017-11-26 11:56:51 PST Comment hidden (obsolete)
Comment 3 Sam Weinig 2017-11-26 13:43:53 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2017-11-26 13:46:08 PST Comment hidden (obsolete)
Comment 5 Sam Weinig 2017-11-26 16:51:05 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2017-11-26 16:58:52 PST Comment hidden (obsolete)
Comment 7 Sam Weinig 2017-11-26 17:08:32 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2017-11-26 17:10:04 PST Comment hidden (obsolete)
Comment 9 Alexey Shvayka 2021-03-16 16:18:04 PDT
*** Bug 177453 has been marked as a duplicate of this bug. ***
Comment 10 Sam Weinig 2021-03-21 08:56:25 PDT
I have a change to remove all the remaining uses of [MayThrowException], so am going to broaden this a bit.
Comment 11 Sam Weinig 2021-03-21 09:45:54 PDT Comment hidden (obsolete)
Comment 12 Sam Weinig 2021-03-21 09:47:32 PDT
It's a big one, but it finally does away with [MayThrowException].
Comment 13 Sam Weinig 2021-03-21 09:47:55 PDT
And a huge amount of it is just test changes.
Comment 14 Sam Weinig 2021-03-21 09:51:06 PDT
Created attachment 423830 [details]
Just the code changes (no IDL or test result changes)
Comment 15 Sam Weinig 2021-03-21 09:53:16 PDT
Added a version with just the code changes to make it a bit easier to look at.
Comment 16 Sam Weinig 2021-03-22 09:24:40 PDT
Created attachment 423898 [details]
Patch
Comment 17 Chris Dumez 2021-03-22 11:47:01 PDT
Comment on attachment 423898 [details]
Patch

r=me if the bots are happy. Very nice.
Comment 18 Darin Adler 2021-03-22 11:48:40 PDT
Comment on attachment 423898 [details]
Patch

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

Reading the patch now, spotted a couple errors, but still reading through the rest

> Source/WebCore/ChangeLog:14
> +        compile. To work around this, toJS<>() can now take a lambda as it's value, and

it's -> its

> Source/WebCore/ChangeLog:93
> +        required as we actually look at the return type via deduction and need it match.

need it match -> need it to match
Comment 19 Darin Adler 2021-03-22 11:51:20 PDT
Comment on attachment 423898 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMExceptionHandling.h:101
>          return;

Why not take out the return if we are using else?
Comment 20 Darin Adler 2021-03-22 11:55:40 PDT
Comment on attachment 423898 [details]
Patch

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

Not that we needed to, but if we wanted a smaller patch could have done "allow but ignore MayThrowException" and then did the "remove and forbid MayThrowException" in a follow up.

> Source/WebCore/dom/Element.h:212
> +    // Bindings variants of the above two functions that have the correct return type.

Not 100% sure we need the comment. The names seem self-explanatory and while it’s hard to notice the only difference is the return type, the repercussions of not noticing are very small.

> Source/WebCore/dom/Element.h:214
> +    inline void removeAttributeForBindings(const AtomString& qualifiedName) { removeAttribute(qualifiedName); }
> +    inline void removeAttributeNSForBindings(const AtomString& namespaceURI, const AtomString& localName) { removeAttributeNS(namespaceURI, localName); }

The "inline" here can be omitted and has no effect.
Comment 21 Darin Adler 2021-03-22 11:55:52 PDT
OK, done with my review pass.
Comment 22 Sam Weinig 2021-03-22 16:14:32 PDT
Created attachment 423956 [details]
Patch
Comment 23 Sam Weinig 2021-03-22 16:15:04 PDT
*** Bug 177453 has been marked as a duplicate of this bug. ***
Comment 24 EWS 2021-03-22 17:49:07 PDT
Committed r274832: <https://commits.webkit.org/r274832>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423956 [details].
Comment 25 Radar WebKit Bug Importer 2021-03-22 17:50:19 PDT
<rdar://problem/75717440>
Comment 26 Don Olmstead 2021-03-23 13:46:13 PDT
After this patch I see the following error in a non-unified build.

WebCore/DerivedSources/JSWebGLLoseContext.cpp:146:57: error: use of undeclared identifier 'IDLUndefined'
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
                                                        ^
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:71: warning: expression result unused [-Wunused-value]
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
                                                                      ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders\JavaScriptCore/ExceptionScope.h:113:16: note: expanded from macro 'RELEASE_AND_RETURN'
        return expression__; \
               ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:93: warning: expression result unused [-Wunused-value]
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
                                                                                            ^~~~~~~~~~
JavaScriptCore/PrivateHeaders\JavaScriptCore/ExceptionScope.h:113:16: note: expanded from macro 'RELEASE_AND_RETURN'
        return expression__; \
               ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:161:57: error: use of undeclared identifier 'IDLUndefined'
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
Comment 27 Darin Adler 2021-03-23 13:52:59 PDT
(In reply to Don Olmstead from comment #26)
> After this patch I see the following error in a non-unified build.

The fix for this is to add the necessary includes. It looks like the AddToIncludesForIDLType function is missing code for the case of "undefined". I think it might be something like this:

    if ($type->name eq "undefined") {
        AddToIncludes("IDLTypes.h", $includesRef, $conditional);
        return;
    }
Comment 28 Darin Adler 2021-03-23 15:17:11 PDT
Can someone who is doing a non-unified build (Don?) try the above addition to CodeGeneratorJS.pm with my suggestion above to see if it works?
Comment 29 Don Olmstead 2021-03-23 15:41:41 PDT
(In reply to Darin Adler from comment #28)
> Can someone who is doing a non-unified build (Don?) try the above addition
> to CodeGeneratorJS.pm with my suggestion above to see if it works?

(In reply to Darin Adler from comment #27)
> (In reply to Don Olmstead from comment #26)
> > After this patch I see the following error in a non-unified build.
> 
> The fix for this is to add the necessary includes. It looks like the
> AddToIncludesForIDLType function is missing code for the case of
> "undefined". I think it might be something like this:
> 
>     if ($type->name eq "undefined") {
>         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
>         return;
>     }

I can give that a shot. Might not get to it until tomorrow though.

Thanks for the hint! Always appreciated!
Comment 30 Chris Dumez 2021-03-23 15:53:52 PDT
(In reply to Don Olmstead from comment #29)
> (In reply to Darin Adler from comment #28)
> > Can someone who is doing a non-unified build (Don?) try the above addition
> > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> 
> (In reply to Darin Adler from comment #27)
> > (In reply to Don Olmstead from comment #26)
> > > After this patch I see the following error in a non-unified build.
> > 
> > The fix for this is to add the necessary includes. It looks like the
> > AddToIncludesForIDLType function is missing code for the case of
> > "undefined". I think it might be something like this:
> > 
> >     if ($type->name eq "undefined") {
> >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> >         return;
> >     }
> 
> I can give that a shot. Might not get to it until tomorrow though.
> 
> Thanks for the hint! Always appreciated!

Follow-up build fix landed in <https://commits.webkit.org/r274906>.
Comment 31 Sam Weinig 2021-03-23 15:55:38 PDT
(In reply to Chris Dumez from comment #30)
> (In reply to Don Olmstead from comment #29)
> > (In reply to Darin Adler from comment #28)
> > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > 
> > (In reply to Darin Adler from comment #27)
> > > (In reply to Don Olmstead from comment #26)
> > > > After this patch I see the following error in a non-unified build.
> > > 
> > > The fix for this is to add the necessary includes. It looks like the
> > > AddToIncludesForIDLType function is missing code for the case of
> > > "undefined". I think it might be something like this:
> > > 
> > >     if ($type->name eq "undefined") {
> > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > >         return;
> > >     }
> > 
> > I can give that a shot. Might not get to it until tomorrow though.
> > 
> > Thanks for the hint! Always appreciated!
> 
> Follow-up build fix landed in <https://commits.webkit.org/r274906>.

Did that cause no bindings test changes?
Comment 32 Chris Dumez 2021-03-23 16:10:21 PDT
(In reply to Sam Weinig from comment #31)
> (In reply to Chris Dumez from comment #30)
> > (In reply to Don Olmstead from comment #29)
> > > (In reply to Darin Adler from comment #28)
> > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > 
> > > (In reply to Darin Adler from comment #27)
> > > > (In reply to Don Olmstead from comment #26)
> > > > > After this patch I see the following error in a non-unified build.
> > > > 
> > > > The fix for this is to add the necessary includes. It looks like the
> > > > AddToIncludesForIDLType function is missing code for the case of
> > > > "undefined". I think it might be something like this:
> > > > 
> > > >     if ($type->name eq "undefined") {
> > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > >         return;
> > > >     }
> > > 
> > > I can give that a shot. Might not get to it until tomorrow though.
> > > 
> > > Thanks for the hint! Always appreciated!
> > 
> > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> 
> Did that cause no bindings test changes?

It did, sorry. I updated them at <https://commits.webkit.org/r274909>.

BTW, I don't know if I am going crazy or if something is wrong with my machine but generating the JS bindings seems to be crazy slow. I wonder if we regressed speed somewhat recently.
Comment 33 Sam Weinig 2021-03-23 16:21:27 PDT
(In reply to Chris Dumez from comment #32)
> (In reply to Sam Weinig from comment #31)
> > (In reply to Chris Dumez from comment #30)
> > > (In reply to Don Olmstead from comment #29)
> > > > (In reply to Darin Adler from comment #28)
> > > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > > 
> > > > (In reply to Darin Adler from comment #27)
> > > > > (In reply to Don Olmstead from comment #26)
> > > > > > After this patch I see the following error in a non-unified build.
> > > > > 
> > > > > The fix for this is to add the necessary includes. It looks like the
> > > > > AddToIncludesForIDLType function is missing code for the case of
> > > > > "undefined". I think it might be something like this:
> > > > > 
> > > > >     if ($type->name eq "undefined") {
> > > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > > >         return;
> > > > >     }
> > > > 
> > > > I can give that a shot. Might not get to it until tomorrow though.
> > > > 
> > > > Thanks for the hint! Always appreciated!
> > > 
> > > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> > 
> > Did that cause no bindings test changes?
> 
> It did, sorry. I updated them at <https://commits.webkit.org/r274909>.
> 
> BTW, I don't know if I am going crazy or if something is wrong with my
> machine but generating the JS bindings seems to be crazy slow. I wonder if
> we regressed speed somewhat recently.

Can you quantify it anyway? As one data point, I haven't seen any bugs from ap about a compile time slowdown (which I usually do when I regress it).
Comment 34 Chris Dumez 2021-03-23 16:24:21 PDT
(In reply to Chris Dumez from comment #32)
> (In reply to Sam Weinig from comment #31)
> > (In reply to Chris Dumez from comment #30)
> > > (In reply to Don Olmstead from comment #29)
> > > > (In reply to Darin Adler from comment #28)
> > > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > > 
> > > > (In reply to Darin Adler from comment #27)
> > > > > (In reply to Don Olmstead from comment #26)
> > > > > > After this patch I see the following error in a non-unified build.
> > > > > 
> > > > > The fix for this is to add the necessary includes. It looks like the
> > > > > AddToIncludesForIDLType function is missing code for the case of
> > > > > "undefined". I think it might be something like this:
> > > > > 
> > > > >     if ($type->name eq "undefined") {
> > > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > > >         return;
> > > > >     }
> > > > 
> > > > I can give that a shot. Might not get to it until tomorrow though.
> > > > 
> > > > Thanks for the hint! Always appreciated!
> > > 
> > > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> > 
> > Did that cause no bindings test changes?
> 
> It did, sorry. I updated them at <https://commits.webkit.org/r274909>.
> 
> BTW, I don't know if I am going crazy or if something is wrong with my
> machine but generating the JS bindings seems to be crazy slow. I wonder if
> we regressed speed somewhat recently.

r274832:
real	1m9.199s
user	0m29.724s
sys	0m38.310s

r273000:
real	1m6.759s
user	0m29.013s
sys	0m36.827s

Seems slow to regenerate bindings tests results on an iMac Pro but at least it's not a recent regression :)
Comment 35 Don Olmstead 2021-03-24 12:56:02 PDT
(In reply to Chris Dumez from comment #30)
> (In reply to Don Olmstead from comment #29)
> > (In reply to Darin Adler from comment #28)
> > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > 
> > (In reply to Darin Adler from comment #27)
> > > (In reply to Don Olmstead from comment #26)
> > > > After this patch I see the following error in a non-unified build.
> > > 
> > > The fix for this is to add the necessary includes. It looks like the
> > > AddToIncludesForIDLType function is missing code for the case of
> > > "undefined". I think it might be something like this:
> > > 
> > >     if ($type->name eq "undefined") {
> > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > >         return;
> > >     }
> > 
> > I can give that a shot. Might not get to it until tomorrow though.
> > 
> > Thanks for the hint! Always appreciated!
> 
> Follow-up build fix landed in <https://commits.webkit.org/r274906>.

So after the change I was still getting the error.

If I manually add #include "JSDOMConvertBase.h" it compiles fine.
Comment 36 Chris Dumez 2021-03-24 12:58:58 PDT
(In reply to Don Olmstead from comment #35)
> (In reply to Chris Dumez from comment #30)
> > (In reply to Don Olmstead from comment #29)
> > > (In reply to Darin Adler from comment #28)
> > > > Can someone who is doing a non-unified build (Don?) try the above addition
> > > > to CodeGeneratorJS.pm with my suggestion above to see if it works?
> > > 
> > > (In reply to Darin Adler from comment #27)
> > > > (In reply to Don Olmstead from comment #26)
> > > > > After this patch I see the following error in a non-unified build.
> > > > 
> > > > The fix for this is to add the necessary includes. It looks like the
> > > > AddToIncludesForIDLType function is missing code for the case of
> > > > "undefined". I think it might be something like this:
> > > > 
> > > >     if ($type->name eq "undefined") {
> > > >         AddToIncludes("IDLTypes.h", $includesRef, $conditional);
> > > >         return;
> > > >     }
> > > 
> > > I can give that a shot. Might not get to it until tomorrow though.
> > > 
> > > Thanks for the hint! Always appreciated!
> > 
> > Follow-up build fix landed in <https://commits.webkit.org/r274906>.
> 
> So after the change I was still getting the error.

"the" error or a different error.

The error was:
> use of undeclared identifier 'IDLUndefined'

My patch included the right error for that type.

JSDOMConvertBase.h may be needed too, not for IDLUndefined.


> 
> If I manually add #include "JSDOMConvertBase.h" it compiles fine.
Comment 37 Darin Adler 2021-03-24 13:03:52 PDT
(In reply to Chris Dumez from comment #36)
> (In reply to Don Olmstead from comment #35)
> > So after the change I was still getting the error.
> 
> "the" error or a different error.

Yes, can we see the error?
Comment 38 Don Olmstead 2021-03-24 13:06:59 PDT
(In reply to Darin Adler from comment #37)
> (In reply to Chris Dumez from comment #36)
> > (In reply to Don Olmstead from comment #35)
> > > So after the change I was still getting the error.
> > 
> > "the" error or a different error.
> 
> Yes, can we see the error?

Yea sorry I misspoke. Should've been `an` not `the`. It was just on the same line but now on toJS<IDLUndefined>.

WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:57: error: 'IDLUndefined' does not refer to a value
    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
                                                        ^
../../Source/WebCore/bindings\IDLTypes.h:96:8: note: declared here
struct IDLUndefined : IDLType<void> { };
Comment 39 Darin Adler 2021-03-24 13:48:37 PDT
Given that detail, I agree: we should use JSDOMConvertBase.h in AddToIncludesForIDLType instead of IDLTypes.h.