Bug 160356

Summary: Enable strict type checking for Window dictionary members
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, dbates, rniwa, sam
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: http://heycam.github.io/webidl/#es-interface
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch none

Description Chris Dumez 2016-07-29 14:17:28 PDT
Enable strict type checking for Window dictionary members. Technically, we should do strict type checking of all wrapper types but this patch focuses on Window because it is common to pass a Window dictionary member to Event constructors.

By strict type checking, I mean that we should throw a TypeError is the value is not null/undefined and does not implement the Window interface:
- http://heycam.github.io/webidl/#es-interface

Firefox and Chrome comply with the specification already.
Comment 1 Chris Dumez 2016-07-29 14:22:52 PDT
Created attachment 284901 [details]
Patch
Comment 2 Build Bot 2016-07-29 16:48:21 PDT
Comment on attachment 284901 [details]
Patch

Attachment 284901 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1775961

New failing tests:
media/track/track-remove-quickly.html
Comment 3 Build Bot 2016-07-29 16:48:25 PDT
Created attachment 284918 [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.5
Comment 4 Chris Dumez 2016-07-29 17:00:56 PDT
The iOS failure does not seem related to my change.
Comment 5 Alexey Proskuryakov 2016-07-30 00:14:19 PDT
Yes, the iOS failure is tracked as bug 160367 (and the test causing the crash is now skipped).
Comment 6 Darin Adler 2016-07-30 16:51:35 PDT
Comment on attachment 284901 [details]
Patch

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

> Source/WebCore/bindings/js/JSDictionary.cpp:184
> +    if (value.isUndefinedOrNull()) {
> +        result = nullptr;
> +        return;
> +    }

Why not merge this with the UNLIKELY(!window) case below instead of checking before calling toWrapped? I assume the undefined and null are not hot cases that are as important to optimize as the window case so it’s nice to save the branch in the normal case.

> Source/WebCore/bindings/js/JSDictionary.cpp:186
> +    if (UNLIKELY(!window)) {

Change this to:

    if (UNLIKELY(!window) && !value.isUndefinedOrNull()) {

and then we don’t need to add the check above.
Comment 7 Chris Dumez 2016-07-30 16:59:11 PDT
Created attachment 284957 [details]
Patch
Comment 8 Chris Dumez 2016-07-30 17:01:10 PDT
(In reply to comment #6)
> Comment on attachment 284901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284901&action=review
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:184
> > +    if (value.isUndefinedOrNull()) {
> > +        result = nullptr;
> > +        return;
> > +    }
> 
> Why not merge this with the UNLIKELY(!window) case below instead of checking
> before calling toWrapped? I assume the undefined and null are not hot cases
> that are as important to optimize as the window case so it’s nice to save
> the branch in the normal case.
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:186
> > +    if (UNLIKELY(!window)) {
> 
> Change this to:
> 
>     if (UNLIKELY(!window) && !value.isUndefinedOrNull()) {
> 
> and then we don’t need to add the check above.

Ok, I agree that in the dictionary case, passing null/undefined is not the common case (people would likely just omit it from the dictionary) and factoring as you suggest is indeed better. I made the change, thanks.
Comment 9 WebKit Commit Bot 2016-07-30 17:30:35 PDT
Comment on attachment 284957 [details]
Patch

Clearing flags on attachment: 284957

Committed r203950: <http://trac.webkit.org/changeset/203950>
Comment 10 WebKit Commit Bot 2016-07-30 17:30:40 PDT
All reviewed patches have been landed.  Closing bug.