addEventListener/removeEventListener incorrectly handle a primitive second argument
Reported by
bzbar...@mit.edu,
Dec 23 2016
|
|||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0 Steps to reproduce the problem: 1. Load testcase. What is the expected behavior? Testcase says "PASS". What went wrong? Testcase says "FAIL". Did this work before? No Does this work in other browsers? No This works correctly in Firefox, but Safari seems to be buggy in the same way Chrome is. Chrome version: 57.0.2950.4 (Official Build) dev (64-bit) Channel: n/a OS Version: OS X 10.12 Flash Version: Per spec, that second argument is an EventTarget? and EventTarget is a callback interface. So any argument other than an object, null, or undefined, should throw an exception.
,
Dec 23 2016
The tests do pass in a WebKit nightly, so WebKit has apparently fixed this bug since Safari 10,
,
Dec 24 2016
Correction: per spec the second argument (the callback function) is EventListener, not EventTarget. Other than that, the bug report highlights a valid problem.
,
Dec 24 2016
,
Dec 24 2016
Er, yes, a typo on my part. EventListener? is it.
,
Dec 27 2016
Trying to get my issues resolved ASAP please
,
Dec 27 2016
Bashi, could you take a look at this? The test passes false as the *second* argument (ie the EventListener?) and contends this should fail. Seems reasonable. Here's the spec for EventTarget: https://dom.spec.whatwg.org/#interface-eventtarget [Exposed=(Window,Worker)] interface EventTarget { void addEventListener(DOMString type, EventListener? callback, optional (AddEventListenerOptions or boolean) options); void removeEventListener(DOMString type, EventListener? callback, optional (EventListenerOptions or boolean) options); ... Our metadata about the EventTarget interface is here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/EventTarget.idl?q=addEventListener+%5C.idl$&sq=package:chromium&l=28 Here's the code we generate for that: https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8EventTarget.cpp?sq=package:chromium&rcl=1482797967&l=109 It looks like V8EventListenerHelper, etc. are somehow converting 'false' to an EventListener, which shouldn't be possible since it's not an object?
,
Dec 27 2016
Thanks for the investigation, Dominic. I'll try to fix this.
,
Jan 2 2017
It's fairly likely that this affects some other implementation as well, so adding a test that `addEventListener('type', false)` throws an exception to https://github.com/w3c/web-platform-tests/blob/master/dom/events/EventTarget-addEventListener.html would be great.
,
Jan 5 2017
I uploaded a fix (https://codereview.chromium.org/2598353002/) but it seems that a lot of layout tests use add/removeEventListener in wrong ways. It may imply that the CL would break existing webapps/extensions even it's a bug fix. Not sure it's safe to submit the CL (with fixing tests). Maybe send a PSA to blink-dev@ first?
,
Jan 5 2017
I think we have to allow x.onclick = 0 not throwing an exception. Can you verify that works in other browsers?
,
Jan 5 2017
That works in other browsers _and_ per spec. That's because the type of x.onclick isn't "EventListener?". It's "EventHandler", which is a typedef for "EventHandlerNonNull?", which is defined like so: [TreatNonObjectAsNull] callback EventHandlerNonNull = any (Event event); So any primitive assigned to x.onclick is treated as null per spec. None of that has anything to do with add/removeEventListener. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bzbar...@mit.edu
, Dec 23 2016