Throw TypeError when addEventListener or removeEventListener are called without enough arguments

Throw TypeError when addEventListener or removeEventListener are called without enough arguments

** Changes web-facing behavior **

This removes legacy behavior, per request of Tab:
it fixes some current compatibility problems which *are* causing pain,
but potentially breaks some old content.

Current behavior (not throwing a TypeError) was for compatibility in 2012,
though unspecified cause:
WebKit bug specifically special-casing this in JavaScriptCore bindings is:
[JSC] Regression: addEventListener() and removeEventListener() raise an exception on missing args
https://bugs.webkit.org/show_bug.cgi?id=85928

WebKit/Safari also has legacy behavior (matches current Blink/Chrome):
"For compatibility with legacy content, the EventListener calls are generated without GenerateArgumentsCountCheck."
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm#L2823

However, IE and Firefox *do* throw (correctly), and this discrepancy is causing
compatibility problems:
removeEventListener with only one passed parameter...
http://lists.w3.org/Archives/Public/public-script-coord/2014AprJun/0166.html

Thus this CL aligns with spec, updating tests and removing legacy tests.

R=haraken
BUG=353484
TEST=fast/dom/event-target-arguments.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176746

[Nils Barth (inactive), 2014-06-11 05:49:25]

Removing legacy cruft (our favorite interface, EventListener), per request of
Tab.

[Nils Barth (inactive), 2014-06-11 05:53:08]

Tab, one other question for you:

I've left a FIXME for what to do if |undefined| is passed
for a required argument, like:
document.addEventListener(undefined, listener)

Compatibility-wise, this doesn't throw:
http://lists.w3.org/Archives/Public/public-script-coord/2014AprJun/0166.html
...but there is an (old) argument that it *should* throw:
Dealing with undefined
https://www.w3.org/Bugs/Public/show_bug.cgi?id=23532

Concretely this is in:
https://codereview.chromium.org/329053002/diff/1/LayoutTests/fast/dom/event-t...

Any thoughts on this?

[haraken, 2014-06-11 06:49:25]

LGTM, given the spec and the behavior of IE and Firefox. I'd like to have Tab
double-confirm.

[TabAtkins, 2014-06-11 22:09:32]

On 2014/06/11 06:49:25, haraken wrote:
> LGTM, given the spec and the behavior of IE and Firefox. I'd like to have Tab
> double-confirm.

LGTM

[TabAtkins, 2014-06-11 22:14:58]

On 2014/06/11 05:53:08, Nils Barth wrote:
> Tab, one other question for you:
> 
> I've left a FIXME for what to do if |undefined| is passed
> for a required argument, like:
> document.addEventListener(undefined, listener)
> 
> Compatibility-wise, this doesn't throw:
> http://lists.w3.org/Archives/Public/public-script-coord/2014AprJun/0166.html
> ...but there is an (old) argument that it *should* throw:
> Dealing with undefined
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=23532
> 
> Concretely this is in:
>
https://codereview.chromium.org/329053002/diff/1/LayoutTests/fast/dom/event-t...
> 
> Any thoughts on this?

I think it's a good idea to align with ES's optionality rules, but it's not
important enough to break significant compat.  It might be worthwhile to study
usage, though, to see if there's actually any compat risk of changing, rather
than just assuming that everyone having the same behavior means that behavior is
required.

[Nils Barth (inactive), 2014-06-12 04:10:27]

On 2014/06/11 22:14:58, TabAtkins wrote:
> I think it's a good idea to align with ES's optionality rules, but it's not
> important enough to break significant compat.  It might be worthwhile to study
> usage, though, to see if there's actually any compat risk of changing, rather
> than just assuming that everyone having the same behavior means that behavior
is
> required.

Got it: let's measure first, then remove if looks good.
[DeprecateAs] CL posted at:
[DeprecateAs] if call addEventListener or removeEventListener without enough
arguments
https://codereview.chromium.org/329223004/

For now closing, but posted updated CL for reference.

[Tab Atkins, 2014-06-12 18:28:56]

On 2014/06/12 04:10:27, Nils Barth wrote:
> On 2014/06/11 22:14:58, TabAtkins wrote:
> > I think it's a good idea to align with ES's optionality rules, but it's not
> > important enough to break significant compat.  It might be worthwhile to
study
> > usage, though, to see if there's actually any compat risk of changing,
rather
> > than just assuming that everyone having the same behavior means that
behavior
> is
> > required.
> 
> Got it: let's measure first, then remove if looks good.
> [DeprecateAs] CL posted at:
> [DeprecateAs] if call addEventListener or removeEventListener without enough
> arguments
> https://codereview.chromium.org/329223004/
> 
> For now closing, but posted updated CL for reference.

Note that I think the current change (throwing on missing argument) is fine;
there's compat pain both ways.

I was suggesting measuring usage of passing "undefined" as values, to see if we
can align with ES and treat undefined as missing.

[Nils Barth (inactive), 2014-06-23 09:55:40]

On 2014/06/12 18:28:56, Tab Atkins wrote:
> I was suggesting measuring usage of passing "undefined" as values, to see if
we
> can align with ES and treat undefined as missing.

Oh, got it; that would be part of:
Issue 335871
Treat undefined as missing optional arguments
https://code.google.com/p/chromium/issues/detail?id=335871

We could add use counters for how often |undefined| is explicitly passed in;
I've commented there, and added you to the bug!

[Nils Barth (inactive), 2014-06-23 10:10:14]

Apologies for delay (been busy); rebased and committing.

[Nils Barth (inactive), 2014-06-23 10:10:46]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-06-23 10:11:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nbarth@chromium.org/329053002/80001

[commit-bot: I haz the power, 2014-06-23 11:15:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8255)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12370)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20294)

[commit-bot: I haz the power, 2014-06-23 11:16:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 11:16:26]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12376)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/20303)

[Nils Barth (inactive), 2014-06-23 11:25:31]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-06-23 11:26:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nbarth@chromium.org/329053002/100001

[commit-bot: I haz the power, 2014-06-23 12:31:20]

Change committed as 176746

[f(malita), 2014-06-23 14:59:56]

On 2014/06/23 12:31:20, I haz the power (commit-bot) wrote:
> Change committed as 176746

This broke
browser_tests/ExternallyConnectableMessagingTest.FromIframeWithPermission:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/374...

[f(malita), 2014-06-23 17:07:37]

On 2014/06/23 14:59:56, Florin Malita wrote:
> On 2014/06/23 12:31:20, I haz the power (commit-bot) wrote:
> > Change committed as 176746
> 
> This broke
> browser_tests/ExternallyConnectableMessagingTest.FromIframeWithPermission:
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/374...

Test fix landed: https://codereview.chromium.org/347393005

[tkent, 2014-06-23 23:58:36]

> ** Changes web-facing behavior ** 

Please involve API owners for such changes.


I'm very afraid this will break a lot of mobile web sites.  I recommend to add
UseCounter for addEventListener calls without three arguments.

I thought we had a lot of layout tests with addEventListener calls without the
third argument.
I'm afraid JavaScript code in Chromium also have such addEventListener.

[tkent, 2014-06-24 00:01:52]

On 2014/06/23 23:58:36, tkent wrote:
> > ** Changes web-facing behavior ** 
> 
> Please involve API owners for such changes.
> 
> 
> I'm very afraid this will break a lot of mobile web sites.  I recommend to add
> UseCounter for addEventListener calls without three arguments.
> 
> I thought we had a lot of layout tests with addEventListener calls without the
> third argument.
> I'm afraid JavaScript code in Chromium also have such addEventListener.

Ah, the CL actually made only the second argument mandatory.  So, probably it
won't break a lot of sites.

[Stephen White, 2014-06-24 00:19:35]

On 2014/06/24 00:01:52, tkent wrote:
> On 2014/06/23 23:58:36, tkent wrote:
> > > ** Changes web-facing behavior ** 
> > 
> > Please involve API owners for such changes.
> > 
> > 
> > I'm very afraid this will break a lot of mobile web sites.  I recommend to
add
> > UseCounter for addEventListener calls without three arguments.
> > 
> > I thought we had a lot of layout tests with addEventListener calls without
the
> > third argument.
> > I'm afraid JavaScript code in Chromium also have such addEventListener.
> 
> Ah, the CL actually made only the second argument mandatory.  So, probably it
> won't break a lot of sites.

I think this may also have broken the following tests on ChromeOS:

GalleryBrowserTest.CropImageOnDrive
GalleryBrowserTest.CropImageOnDownloads
GalleryBrowserTestInGuestMode.CropImageOnDownloads

Try bot run:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

Note that there are no Blink canaries with this configuration, so breakage must
be hand-bisected.

I've narrowed it down to r176743-176752, and this seems to be the most likely
suspect in that range.

[Stephen White, 2014-06-24 01:11:16]

On 2014/06/24 00:19:35, Stephen White wrote:
> On 2014/06/24 00:01:52, tkent wrote:
> > On 2014/06/23 23:58:36, tkent wrote:
> > > > ** Changes web-facing behavior ** 
> > > 
> > > Please involve API owners for such changes.
> > > 
> > > 
> > > I'm very afraid this will break a lot of mobile web sites.  I recommend to
> add
> > > UseCounter for addEventListener calls without three arguments.
> > > 
> > > I thought we had a lot of layout tests with addEventListener calls without
> the
> > > third argument.
> > > I'm afraid JavaScript code in Chromium also have such addEventListener.
> > 
> > Ah, the CL actually made only the second argument mandatory.  So, probably
it
> > won't break a lot of sites.
> 
> I think this may also have broken the following tests on ChromeOS:
> 
> GalleryBrowserTest.CropImageOnDrive
> GalleryBrowserTest.CropImageOnDownloads
> GalleryBrowserTestInGuestMode.CropImageOnDownloads
> 
> Try bot run:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...
> 
> Note that there are no Blink canaries with this configuration, so breakage
must
> be hand-bisected.
> 
> I've narrowed it down to r176743-176752, and this seems to be the most likely
> suspect in that range.

Confirmed. Passing at r176745:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

Failing at r176746:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

I couldn't find any offending addEventListener or removeEventListener calls in a
quick search of the image gallery code, but I don't know it at all.

I'm going to disable the tests for now, just so we can keep the roll moving.

[haraken, 2014-06-24 01:23:33]

On 2014/06/24 01:11:16, Stephen White wrote:
> On 2014/06/24 00:19:35, Stephen White wrote:
> > On 2014/06/24 00:01:52, tkent wrote:
> > > On 2014/06/23 23:58:36, tkent wrote:
> > > > > ** Changes web-facing behavior ** 
> > > > 
> > > > Please involve API owners for such changes.
> > > > 
> > > > 
> > > > I'm very afraid this will break a lot of mobile web sites.  I recommend
to
> > add
> > > > UseCounter for addEventListener calls without three arguments.
> > > > 
> > > > I thought we had a lot of layout tests with addEventListener calls
without
> > the
> > > > third argument.
> > > > I'm afraid JavaScript code in Chromium also have such addEventListener.
> > > 
> > > Ah, the CL actually made only the second argument mandatory.  So, probably
> it
> > > won't break a lot of sites.
> > 
> > I think this may also have broken the following tests on ChromeOS:
> > 
> > GalleryBrowserTest.CropImageOnDrive
> > GalleryBrowserTest.CropImageOnDownloads
> > GalleryBrowserTestInGuestMode.CropImageOnDownloads
> > 
> > Try bot run:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...
> > 
> > Note that there are no Blink canaries with this configuration, so breakage
> must
> > be hand-bisected.
> > 
> > I've narrowed it down to r176743-176752, and this seems to be the most
likely
> > suspect in that range.
> 
> Confirmed. Passing at r176745:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...
> 
> Failing at r176746:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...
> 
> I couldn't find any offending addEventListener or removeEventListener calls in
a
> quick search of the image gallery code, but I don't know it at all.
> 
> I'm going to disable the tests for now, just so we can keep the roll moving.

Probably it makes sense to revert this CL and start adding [DeprecatedAs] with
https://codereview.chromium.org/329223004/

[Nils Barth (inactive), 2014-06-24 01:49:08]

A revert of this CL has been created in
https://codereview.chromium.org/331323012/ by nbarth@chromium.org.

The reason for reverting is: Broke Chrome OS tests:
GalleryBrowserTest.CropImageOnDrive
GalleryBrowserTest.CropImageOnDownloads
GalleryBrowserTestInGuestMode.CropImageOnDownloads

https://code.google.com/p/chromium/issues/detail?id=388071

Will add a use counter instead..

[Nils Barth (inactive), 2014-06-24 04:42:17]

Hi Kent,
Sorry about that; I had assumed this was an ok change,
but you're right, this needs API owner approval, and,
since it's an old interface, probably should have a use counter first.

I've thus reverted and will instead commit the deprecation CL:
[DeprecateAs] if call addEventListener or removeEventListener without enough
arguments
https://codereview.chromium.org/329223004/

After we've got some data, we can see if it can be removed.

Since this is just making the first two arguments required
(if these are omitted it's a nop),
it shouldn't break any correct code, but as we've seen from test failures,
it will break some WebKit/Blink-specific incorrect code.
(Just a question of how much there is.)

On 2014/06/24 00:01:52, tkent wrote:
> On 2014/06/23 23:58:36, tkent wrote:
> > > ** Changes web-facing behavior ** 
> > 
> > Please involve API owners for such changes.
> > 
> > 
> > I'm very afraid this will break a lot of mobile web sites.  I recommend to
add
> > UseCounter for addEventListener calls without three arguments.
> > 
> > I thought we had a lot of layout tests with addEventListener calls without
the
> > third argument.
> > I'm afraid JavaScript code in Chromium also have such addEventListener.
> 
> Ah, the CL actually made only the second argument mandatory.  So, probably it
> won't break a lot of sites.

[Nils Barth (inactive), 2014-06-24 07:12:34]

To summarize:
* Reverted.
* Will deprecate instead, in https://codereview.chromium.org/329223004/
* This revealed 2 bugs, both now fixed:
- Missing removeEventListener arg in:
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/assertions.js
https://codereview.chromium.org/347393005/

- Incorrect use of removeEventListener in Chrome OS Gallery.app:
https://code.google.com/p/chromium/issues/detail?id=388071
https://codereview.chromium.org/354543002

FWIW, the two failures were both due to the same error:
in removeEventListener,
omitting the first argument (|DOMString type|),
so the call silently failed, not removing the EL.

I suspect that this is a common failure type,
since failing to *add* an EL is probably pretty visible,
but failing to *remove* one probably causes subtler bugs.