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

commit ab4bb3dbcec83cc7220b011b5e313eebc260a7dc
Author: bashi <bashi@chromium.org>
Date: Tue Dec 20 16:19:38 2016

bindings: Don't wrap nullable callback functions in Nullable<T>

The code generator wrongly uses `Nullable<Callback*>` when nullable
callback functions are used like below:

callback C = void();
interface I { void f(C? callback); }

Since C++ impl of callback functions are garbage collected types,
we can just use `Callback*`.

BUG= 675859 

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

[modify] https://crrev.com/ab4bb3dbcec83cc7220b011b5e313eebc260a7dc/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/ab4bb3dbcec83cc7220b011b5e313eebc260a7dc/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp

Thanks bashi \o/

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

commit 1a82b63469d04db61a23921fa95ffa86ec9fd5ed
Author: bashi <bashi@chromium.org>
Date: Mon Dec 26 08:52:26 2016

bindings: Throw a TypeError when a given callback function isn't callable

According to the spec[1], we should throw a TypeError when a given
callback function isn't callable.

[1] https://heycam.github.io/webidl/#es-callback-function

BUG= 675859 

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

[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/1a82b63469d04db61a23921fa95ffa86ec9fd5ed/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp

Does this change web exposed behavior, or did all of those methods verified that the callback is callable in their implementation instead anyway?

It changed web exposed behavior (throwing a TypeError) but that's what the spec requires and I think it's a kind of bug fix.

Sure - but the Chrome 57 beta blog post and ChromeStatus should also announce this - can you, please, create an entry for that?
Throwing may crash applications, so this might even need an intent... Or at least a public service announcement...

I totally understand your concerns, but historically we haven't sent PSAs for this kind of changes because otherwise we will have to send a lot of PSAs as our code generator isn't perfect and contains a lot of bugs that may require changing web exposed behavior to fix :(

I'd like to ask bindings owners for opinions about this. yukishiino, haraken: What do you think? Should we send PSAs every time web expose behavior is changed even if it's a bug fix?

Causing explicit crashes in code that did not crash before is a somewhat more than a small web exposed change (changing ints to longs and vice versa is a small change, making something optional is a small change).

I think we don't need PSA for this change.  My understanding is that PSA is to track much more bigger changes.  If we added this kind of bug fixes to PSA, then we were going to add hundreds of PSA and it would be noisy to track important changes.  I'm fine with not sending PSA.

I don't think we need a PSA for this, though we might want to get LGTM from an API owner for this kind of web-exposed change.

Rick, Philip, can you review (well, in retrospect)?

#9 - if this may completely break code (and it looks like it can), then I would definitely err on the side of caution and noise.
But Rick and Philip may have different opinions.

bashi, I think the nullable callback functions are still incorrect.

Consider adding the following method to CallbackFunctionTest.idl:
```
DOMString testNullableCallback(TestCallback? callback, DOMString message1, DOMString message2);
```

In idl-callback-function-unittest.html, if we have:
```
callbackFunctionTest.testNullableCallback(null, 'foo', 'bar');
```
It will throw a TypeError: "The callback provided as parameter 1 is not a function."

Also, we expect to receive a `nullptr` in the interface implementation when passing `null` as argument.

Can you send another fix?

#11, thanks for asking, but not all web-exposed changes need an Intent or PSA, even if the change causes exceptions to be thrown where they weren't before. I've made many such small changes that skip all process myself.

It's handy if the commit descriptions says which APIs are affected and why it's safe. In this case, I would have written:

Based on changes to the generated bindings, mainly forEach methods for interfaces that have iterable<T> are affected. Testing with NodeList, document.querySelectorAll('body').forEach(null) already throws in Firefox and forEach isn't there in Edge. Combined with the fact that iterable<T> is pretty new, the risk is very low.

setTimeout and setInterval are also affected, but because there is a DOMString overload for each, the generated error handling is actually unreachable.

PhistucK, as the above kind of analysis is time-consuming, it would be good if you could perform it yourself, and ask for clarification only when there's a high chance a mistake has been made.

#14 - I am not paid for this time, you are. ;)

#13: Thanks for the report! Fixing.

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

commit b141b2dbaf241b4b9da254954a008f4ad2340172
Author: bashi <bashi@chromium.org>
Date: Thu Jan 05 06:26:32 2017

bindings: Don't throw a TypeError when 'null' is passed to nullable callback function

'null' and 'undefined' are valid values for nullable callback function parameters.

BUG= 675859 

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

[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/LayoutTests/fast/dom/idl-callback-function-unittest.html
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/AnyCallbackFunctionOptionalAnyArg.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/AnyCallbackFunctionOptionalAnyArg.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/LongCallbackFunction.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/LongCallbackFunction.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/StringSequenceCallbackFunctionLongSequenceArg.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/StringSequenceCallbackFunctionLongSequenceArg.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunction.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunction.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunctionInterfaceArg.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunctionInterfaceArg.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunctionTypedef.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/core/VoidCallbackFunctionTypedef.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/modules/VoidCallbackFunctionModules.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/bindings/tests/results/modules/VoidCallbackFunctionModules.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/core/testing/CallbackFunctionTest.cpp
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/core/testing/CallbackFunctionTest.h
[modify] https://crrev.com/b141b2dbaf241b4b9da254954a008f4ad2340172/third_party/WebKit/Source/core/testing/CallbackFunctionTest.idl

You started fixing this bug over two years ago. Are you still working on it? You can update the status to "archived", "wontfix", or "closed". You can remove yourself as owner and change status to "untriaged", but if this is still a real bug, please do not sit on it.