New issue
Advanced search Search tips

Issue 647693 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

IDL dictionary members of interface type are treated as nullable, should throw TypeError

Project Member Reported by foolip@chromium.org, Sep 16 2016

Issue description

Example: new BlobEvent('foo', { data: null })

The problem is in dictionary_v8.cpp, where null is explicitly exempted from throwing TypeError when conversion fails.
 

Comment 1 by foolip@chromium.org, Sep 16 2016

Blocking: 646837
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f8965f2d8ef4ac323cc4654769e853174267f6f

commit 7f8965f2d8ef4ac323cc4654769e853174267f6f
Author: foolip <foolip@chromium.org>
Date: Sat Sep 17 00:24:40 2016

Enable TypeError for dictionary members of non-nullable interface type

In dictionary_v8.cpp, the extra null check was wrong or redudant
depending on nullabilty:

 * If the member isn't nullable, then passing null should throw
   TypeError.

 * If the member is nullable, then a null check is already emitted
   earlier, and so that part of the condition is always true.

The affected dictionaries were identified by looking at the resulting
changes in the generated bindings. Any affected member was made nullable
so that this change in itself is not observable, but the right behavior
is achieved if those changes are individually reverted.

A few cases that were not given TODOs:

 * MessageEventInit, because the existing TODO is appropriate.

 * MediaStreamEventInit, because it has no spec and MediaStreamEvent's
   corresponding attribute is already nullable.

 * SpeechRecognitionEventInit, because it has no spec.
   SpeechRecognitionEvent's attributes were made nullable because they
   can now in fact be null.

A number of spec issues were discovered, linked with the TODOs.

BUG= 647693 

Review-Url: https://codereview.chromium.org/2342393002
Cr-Commit-Position: refs/heads/master@{#419344}

[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/bindings/templates/dictionary_v8.cpp
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/core/dom/TouchInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/core/events/MessageEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/encryptedmedia/MediaKeyMessageEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/gamepad/GamepadEvent.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/gamepad/GamepadEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/mediastream/MediaStreamEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/presentation/PresentationConnectionAvailableEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/sensor/SensorReadingEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/serviceworkers/FetchEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchResponse.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/speech/SpeechRecognitionEvent.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/speech/SpeechRecognitionEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/vr/VRDisplayEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/webmidi/MIDIConnectionEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/webmidi/MIDIMessageEventInit.idl
[modify] https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f/third_party/WebKit/Source/modules/webusb/USBConnectionEventInit.idl

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 17 2016

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2582f835f9351586c831e87b0cf69318cf6c971e

commit 2582f835f9351586c831e87b0cf69318cf6c971e
Author: rtoy <rtoy@chromium.org>
Date: Tue Sep 20 18:52:42 2016

AudioBufferSourceOptions.buffer should be nullable.

It's valid to set the buffer to null in the constructor.

Also added a test for ConvolverOptions which has the same issue, which
was updated in https://codereview.chromium.org/2352463002/

Spec issue: https://github.com/WebAudio/web-audio-api/issues/980

BUG= 648283 ,  626449 ,  647693 
TEST=constructor/audiobuffersource.html, constructor/convolver.html

Review-Url: https://codereview.chromium.org/2348373002
Cr-Commit-Position: refs/heads/master@{#419830}

[modify] https://crrev.com/2582f835f9351586c831e87b0cf69318cf6c971e/third_party/WebKit/LayoutTests/webaudio/constructor/audiobuffersource.html
[modify] https://crrev.com/2582f835f9351586c831e87b0cf69318cf6c971e/third_party/WebKit/LayoutTests/webaudio/constructor/convolver.html
[modify] https://crrev.com/2582f835f9351586c831e87b0cf69318cf6c971e/third_party/WebKit/Source/modules/webaudio/AudioBufferSourceOptions.idl

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb539825927adfa6f082056d63a9a0c47be8a3b4

commit bb539825927adfa6f082056d63a9a0c47be8a3b4
Author: foolip <foolip@chromium.org>
Date: Thu Sep 22 08:54:09 2016

Make required dictionary members non-nullable again

These were changed to non-nullable to preserve existing behavior.
Because they are required, the risk here is that developers have noticed
they were required but successfully found and used the null loophole. At
the slightest sign of trouble with any of these, it should be reverted
and perhaps the specs should be changed to allow null.

BlobEvent test will be imported from:
https://github.com/w3c/web-platform-tests/pull/3749

PresentationConnectionAvailableEvent is untested:
 https://crbug.com/648922 

BUG= 647693 

Review-Url: https://codereview.chromium.org/2347473005
Cr-Commit-Position: refs/heads/master@{#420285}

[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/notifications/resources/serviceworker-notification-event.js
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html
[add] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-event.html
[add] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-event-worker.js
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/interfaces-worker.js
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/notifications/NotificationEventInit.idl
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/presentation/PresentationConnectionAvailableEventInit.idl
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/serviceworkers/FetchEventInit.idl
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchEventInit.idl
[modify] https://crrev.com/bb539825927adfa6f082056d63a9a0c47be8a3b4/third_party/WebKit/Source/modules/serviceworkers/ForeignFetchResponse.idl

Comment 6 by foolip@chromium.org, Sep 22 2016

Blocking: -646837
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 22 2016

Comment 8 by foolip@chromium.org, Sep 30 2016

Status: Fixed (was: Started)
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/738daed0f742e6bc5f35b0fa895990144dd37010

commit 738daed0f742e6bc5f35b0fa895990144dd37010
Author: foolip <foolip@chromium.org>
Date: Fri Sep 30 09:32:08 2016

Add more use counters for document.createTouch

Safari has 7 required arguments for document.createTouch and treats the
first two arguments as nullable. The hoped-for outcome is that Blink
can also have 7 required arguments, but that at least the target argument
is non-nullable. If not, Touch.prototype.target must be made nullable too.

BUG= 647693 
R=rbyers@chromium.org

Review-Url: https://codereview.chromium.org/2352333002
Cr-Commit-Position: refs/heads/master@{#422071}

[modify] https://crrev.com/738daed0f742e6bc5f35b0fa895990144dd37010/third_party/WebKit/Source/bindings/core/v8/custom/V8DocumentCustom.cpp
[modify] https://crrev.com/738daed0f742e6bc5f35b0fa895990144dd37010/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/738daed0f742e6bc5f35b0fa895990144dd37010/third_party/WebKit/Source/core/dom/Document.idl
[modify] https://crrev.com/738daed0f742e6bc5f35b0fa895990144dd37010/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/738daed0f742e6bc5f35b0fa895990144dd37010/tools/metrics/histograms/histograms.xml

Sign in to add a comment