SecurityPolicyViolationEvent.constructor do not throw any exception as expected when eventInitDict param do not include all the members required

• Component: Bindings
• Status: RESOLVED FIXED

[[e-mail]]

Steps to reproduce:

(1) Open a tab and navigate to any URL;
(2) Run the following code in the Console of Devtools:
```
new SecurityPolicyViolationEvent('', {});
```
(3) Then this code would be evaluated successfully without throwing any exception.

Actual results:

This code is evaluated successfully without throwing any exception.

Expected results:

As https://docs.w3cub.com/dom/securitypolicyviolationevent/securitypolicyviolationevent says, "eventInitDict is a dictionary object containing information about the properties of the SecurityPolicyViolationEvent to be constructed. This can include the following properties, but bear in mind that if you do include an eventInitDict, certain properties must be included (marked below with required, like disposition)."
That means the code above should throw an exception since the required members are undefined, just like what the Chrome would throw: Uncaught TypeError: Failed to construct 'SecurityPolicyViolationEvent': required member disposition is undefined.

[Sam Sneddon [:gsnedders]]

This is likely a more general bindings bug.

https://heycam.github.io/webidl/#es-dictionary is the relevant spec here.

[Chris Dumez]

(In reply to Sam Sneddon [:gsnedders] from comment #1)
> This is likely a more general bindings bug.
> 
> https://heycam.github.io/webidl/#es-dictionary is the relevant spec here.

Not a bindings bug. Our IDL simply doesn't match the spec here. Dictionary members are not marked as required when they should be.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> (In reply to Sam Sneddon [:gsnedders] from comment #1)
> > This is likely a more general bindings bug.
> > 
> > https://heycam.github.io/webidl/#es-dictionary is the relevant spec here.
> 
> Not a bindings bug. Our IDL simply doesn't match the spec here. Dictionary
> members are not marked as required when they should be.

I am taking a quick stab at it to see if I can align with Blink at least. Aligning exactly with the spec would carry compatibility risks as it does not match our current behavior or Blink's.

[Chris Dumez]

Created attachment 429659 [details]
Patch

[Chris Dumez]

Created attachment 429660 [details]
Patch

[Chris Dumez]

Created attachment 429662 [details]
Patch

[Chris Dumez]

Created attachment 429666 [details]
Patch

[Darin Adler]

Comment on attachment 429666 [details]
Patch

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

> Source/WebCore/dom/SecurityPolicyViolationEvent.h:38
> +        Init() { }

Why is this needed? I would not expect it to have any effect.

[Chris Dumez]

(In reply to Darin Adler from comment #8)
> Comment on attachment 429666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429666&action=review
> 
> > Source/WebCore/dom/SecurityPolicyViolationEvent.h:38
> > +        Init() { }
> 
> Why is this needed? I would not expect it to have any effect.

Well, I did not expect it either!

I had to do this to work around the compiler being silly. I think the compiler is having trouble generating the default constructor (`Init() = default;` also fails):

```
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/Modules/encryptedmedia/CDM.cpp:33:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.h:57:
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:55:105: error: default member initializer for 'disposition' needed within definition of enclosing class 'SecurityPolicyViolationEvent' outside of member functions
    static Ref<SecurityPolicyViolationEvent> create(const AtomString& type, const Init& initializer = { }, IsTrusted isTrusted = IsTrusted::No)
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:46:21: note: default member initializer declared here
        Disposition disposition { Disposition::Enforce };
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:55:89: error: unused parameter 'initializer' [-Werror,-Wunused-parameter]
    static Ref<SecurityPolicyViolationEvent> create(const AtomString& type, const Init& initializer = { }, IsTrusted isTrusted = IsTrusted::No)

```

[Darin Adler]

Comment on attachment 429666 [details]
Patch

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

> Source/WebCore/dom/SecurityPolicyViolationEvent.h:35
> +    enum class Disposition : uint8_t { Enforce, Report };

Could use bool instead of uint8_t here.

>>> Source/WebCore/dom/SecurityPolicyViolationEvent.h:38
>>> +        Init() { }
>> 
>> Why is this needed? I would not expect it to have any effect.
> 
> Well, I did not expect it either!
> 
> I had to do this to work around the compiler being silly. I think the compiler is having trouble generating the default constructor (`Init() = default;` also fails):
> 
> ```
> In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/Modules/encryptedmedia/CDM.cpp:33:
> In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/Document.h:57:
> /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:55:105: error: default member initializer for 'disposition' needed within definition of enclosing class 'SecurityPolicyViolationEvent' outside of member functions
>     static Ref<SecurityPolicyViolationEvent> create(const AtomString& type, const Init& initializer = { }, IsTrusted isTrusted = IsTrusted::No)
> /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:46:21: note: default member initializer declared here
>         Disposition disposition { Disposition::Enforce };
> /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/SecurityPolicyViolationEvent.h:55:89: error: unused parameter 'initializer' [-Werror,-Wunused-parameter]
>     static Ref<SecurityPolicyViolationEvent> create(const AtomString& type, const Init& initializer = { }, IsTrusted isTrusted = IsTrusted::No)
> 
> ```

I am having a lot of trouble understanding this error message. If we understood the complaint, I bet we could fix it!

[Chris Dumez]

Created attachment 429673 [details]
Patch

[EWS]

Committed r278042 (238134@main): <https://commits.webkit.org/238134@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429673 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78473411>

[Chris Dumez]

I have filed: https://github.com/w3c/webappsec-csp/issues/498