Bug 160356 - Enable strict type checking for Window dictionary members
Summary: Enable strict type checking for Window dictionary members
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://heycam.github.io/webidl/#es-in...
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2016-07-29 14:17 PDT by Chris Dumez
Modified: 2016-07-30 17:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (44.48 KB, patch)
2016-07-29 14:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1020.61 KB, application/zip)
2016-07-29 16:48 PDT, Build Bot
no flags Details
Patch (44.47 KB, patch)
2016-07-30 16:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.