Bug 149466 – Implement EventListenerOptions argument to addEventListener

• Component: DOM
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

This is a proposal to allow authors to specify that they'll never preventDefault on events:
http://rbyers.github.io/EventListenerOptions/EventListenerOptions.html

allowing the browser to do various optimizations.

[Radar WebKit Bug Importer]

<rdar://problem/22802031>

[Theresa O'Connor]

I would hold off on this (until|if) it lands in the DOM spec.

https://github.com/whatwg/dom/pull/82

[Rick Byers]

FYI, this has now landed in the spec: https://github.com/whatwg/dom/commit/253a21b8e78e37447c47983916a7cf39c4f6a3c5

Explainer here: https://github.com/RByers/EventListenerOptions/blob/gh-pages/explainer.md

We'll probably ship the basic EventListenerOptions API in blink soon, but we've got a lot of work to do to fully implement the performance benefits of the 'passive' option (it's high priority for us though).

[David Tapuska]

Any progress on this? It has been in the DOM spec for a few months now.

FireFox has landed an implementation earlier this month.

[Dean Jackson]

(In reply to comment #4)
> Any progress on this? It has been in the DOM spec for a few months now.
> 
> FireFox has landed an implementation earlier this month.

No progress to report. We'll get to it. Keep bugging us.

[Chris Dumez]

I am working on initial support:
- Bindings support and passing all the options to native code
- Support for the 'once' / 'capture' options

The 'passive' flag will be there on our EventListeners but I am not planning to leverage it in my initial patch. However, Benjamin said he may be interested in updating our touch event handling code to leverage it (for performance).

[David Tapuska]

The main thing that needs to be done for compatibility is to pass the dictionary in as the 3rd arg. I don't know if it makes sense to add the passive field unless you actually support it.  Chromium shipped this way for a few releases. We exposed the dictionary as 3rd argument in 48 I think and then enabled support for the passive field in 51.

[Chris Dumez]

(In reply to comment #7)
> The main thing that needs to be done for compatibility is to pass the
> dictionary in as the 3rd arg. I don't know if it makes sense to add the
> passive field unless you actually support it.  Chromium shipped this way for
> a few releases. We exposed the dictionary as 3rd argument in 48 I think and
> then enabled support for the passive field in 51.

As far as I know, wether the passive field is in the IDL or not is not observable from JS. So I am planning to have the dictionary match the specification. All 3 flags will be passed to our implementation and stored. We anyway will hopefully find good uses for the passive flag in the fairly short term.

[David Tapuska]

Fields are observable due to the resolution of dictionaries to objects. 

See https://github.com/WICG/EventListenerOptions/blob/gh-pages/EventListenerOptions.polyfill.js for how to detect passive is supported

[Chris Dumez]

Created attachment 280515 [details]
WIP patch

[WebKit Commit Bot]

Attachment 280515 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGElement.h:137:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/EventListenerMap.h:58:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 280515 [details]
WIP patch

This is cool.

[Rick Byers]

Thanks for working on this feature!

We're actively encouraging feature detection of the passive option (eg. Via Modernizer.passiveeventlisteners), so sites will go down a different code path depending on whether the dictionary member is present.  It shouldn't often matter, but it's possible preventDefault succeeding on passive listeners could cause compat issues.  So I suggest that if you're going to have the passive member you also should implement it's observable behavior (ignoring calls to preventDefault).  That should be trivial, and the perf benefit can always come later.

[Chris Dumez]

(In reply to comment #13)
> Thanks for working on this feature!
> 
> We're actively encouraging feature detection of the passive option (eg. Via
> Modernizer.passiveeventlisteners), so sites will go down a different code
> path depending on whether the dictionary member is present.  It shouldn't
> often matter, but it's possible preventDefault succeeding on passive
> listeners could cause compat issues.  So I suggest that if you're going to
> have the passive member you also should implement it's observable behavior
> (ignoring calls to preventDefault).  That should be trivial, and the perf
> benefit can always come later.

Sure, ignoring calls to preventDefault() if passive is set should be trivial Island can be done from the start.

[Chris Dumez]

Created attachment 280570 [details]
WIP patch

[WebKit Commit Bot]

Attachment 280570 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/EventListenerMap.h:58:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/svg/SVGElement.h:137:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 280571 [details]
WIP patch

[Build Bot]

Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1450022

New failing tests:
fast/workers/dedicated-worker-lifecycle.html

[Build Bot]

Created attachment 280579 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1450024

New failing tests:
fast/workers/dedicated-worker-lifecycle.html

[Build Bot]

Created attachment 280580 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1450028

New failing tests:
fast/workers/dedicated-worker-lifecycle.html

[Build Bot]

Created attachment 280581 [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.4

[Build Bot]

Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1450029

New failing tests:
fast/workers/dedicated-worker-lifecycle.html

[Build Bot]

Created attachment 280582 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Simon Fraser (smfr)]

Chris Dumez did some of this already.

[Simon Fraser (smfr)]

(In reply to comment #26)
> Chris Dumez did some of this already.

Sorry, I was thinking about scrolling options.

[Chris Dumez]

Created attachment 280604 [details]
WIP Patch

[Chris Dumez]

Created attachment 280615 [details]
WIP Patch

Filed https://github.com/whatwg/dom/issues/265 to ask for clarification about a corner case that came up while writing test cases.

[Chris Dumez]

Created attachment 280624 [details]
Patch

[Chris Dumez]

Created attachment 280626 [details]
Patch

[Chris Dumez]

Created attachment 280631 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280631 [details]
Patch

Clearing flags on attachment: 280631

Committed r201730: <http://trac.webkit.org/changeset/201730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158453

[Ryan Haddad]

*** Bug 158451 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Comment on attachment 280631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280631&action=review

> Source/WebCore/dom/EventTarget.cpp:260
>          RegisteredEventListener& registeredListener = entry[i];

The crashes happened because registeredListener is a reference to a Vector item.
However, when handleEvent() is called below, the JS may unregister event listeners, in which case the vector may get modified and the registeredListener reference gets invalidated.
This used to be fine because registeredListener was not used after calling handleEvent()...

> Source/WebCore/dom/EventTarget.cpp:289
> +        if (registeredListener.isPassive)

... However, I started using registeredListener after calling handleEvent() here...

> Source/WebCore/dom/EventTarget.cpp:292
> +        if (registeredListener.isOnce)

... and here.

This can be fixed by copying the registeredListener instead of using a reference as it is cheap to copy. I'll re-run all the tests in ASAN before re-landing to make sure this was the only issue.

[Chris Dumez]

Created attachment 280662 [details]
Patch

[Chris Dumez]

Comment on attachment 280662 [details]
Patch

Clearing flags on attachment: 280662

Committed r201735: <http://trac.webkit.org/changeset/201735>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

The new test fails on all bots:

@@ -6,9 +6,9 @@
 document.body.addEventListener('test', listenerFunction, { 'once': true })
 PASS listenerCallCount is 0
 document.body.dispatchEvent(new Event('test'))
-PASS listenerCallCount is 1
+FAIL listenerCallCount should be 1. Was 2.
 document.body.dispatchEvent(new Event('test'))
-PASS listenerCallCount is 1
+FAIL listenerCallCount should be 1. Was 2.
 PASS successfullyParsed is true

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158465

[Chris Dumez]

(In reply to comment #41)
> The new test fails on all bots:
> 
> @@ -6,9 +6,9 @@
>  document.body.addEventListener('test', listenerFunction, { 'once': true })
>  PASS listenerCallCount is 0
>  document.body.dispatchEvent(new Event('test'))
> -PASS listenerCallCount is 1
> +FAIL listenerCallCount should be 1. Was 2.
>  document.body.dispatchEvent(new Event('test'))
> -PASS listenerCallCount is 1
> +FAIL listenerCallCount should be 1. Was 2.
>  PASS successfullyParsed is true

Ok, copying the listener is a bad idea as we need to update its ´removed' flag. I will reupload a fixed version this morning.

[Chris Dumez]

Created attachment 280711 [details]
Patch

[Chris Dumez]

Created attachment 280716 [details]
Patch

[Chris Dumez]

Comment on attachment 280716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280716&action=review

Dean, can you please take another look. Here are the changes compared to my previous patch:

> Source/WebCore/dom/EventTarget.cpp:260
> +        RegisteredEventListener registeredListener = entry[i];

Copy the registeredListener instead of keeping a reference because this reference gets invalidated when:
1. we call removeEventListener() below if registeredListener.isOnce is true
2. potentially after calling handleEvent() as the JS can unregister events.

RegisteredEventListener is cheap to copy anyway.

> Source/WebCore/dom/EventTarget.cpp:273
> +        if (registeredListener.isOnce)

No more 'removed' flag on the registeredListener. Instead, we copy the registeredListener above and we remove it from the vector *before* calling handleEvent(). Note that the specification has been updated to do just that:
https://github.com/whatwg/dom/commit/5324200d70eb23276cb58b814416e19c436104f2

[Dean Jackson]

Comment on attachment 280716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280716&action=review

>> Source/WebCore/dom/EventTarget.cpp:260
>> +        RegisteredEventListener registeredListener = entry[i];
> 
> Copy the registeredListener instead of keeping a reference because this reference gets invalidated when:
> 1. we call removeEventListener() below if registeredListener.isOnce is true
> 2. potentially after calling handleEvent() as the JS can unregister events.
> 
> RegisteredEventListener is cheap to copy anyway.

OK.

>> Source/WebCore/dom/EventTarget.cpp:273
>> +        if (registeredListener.isOnce)
> 
> No more 'removed' flag on the registeredListener. Instead, we copy the registeredListener above and we remove it from the vector *before* calling handleEvent(). Note that the specification has been updated to do just that:
> https://github.com/whatwg/dom/commit/5324200d70eb23276cb58b814416e19c436104f2

Nice.

[WebKit Commit Bot]

Comment on attachment 280716 [details]
Patch

Clearing flags on attachment: 280716

Committed r201757: <http://trac.webkit.org/changeset/201757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Rick Byers]

Thanks for importing the WPT and blink tests for this!
FYI I've (finally) landed my WPT test for passive: https://github.com/w3c/web-platform-tests/pull/3119 (sorry I didn't do that sooner), and I'll be deleting our blink LayoutTests tests for the API since they're now redundant with the WPT tests.

We haven't implemented 'once' yet, but I'm happy to review/merge any web-platform-test pull requests for it.

[Chris Dumez]

(In reply to comment #50)
> Thanks for importing the WPT and blink tests for this!
> FYI I've (finally) landed my WPT test for passive:
> https://github.com/w3c/web-platform-tests/pull/3119 (sorry I didn't do that
> sooner), and I'll be deleting our blink LayoutTests tests for the API since
> they're now redundant with the WPT tests.
> 
> We haven't implemented 'once' yet, but I'm happy to review/merge any
> web-platform-test pull requests for it.

Thanks for letting us know. I'll import the new WPT test.