Event: Increase robustness of an inner native event in leverageNative #5466

mgol · 2024-04-02T15:48:49Z

Summary

In Firefox, alert displayed just before blurring an element dispatches
the native blur event twice which tripped the jQuery logic if a jQuery blur
handler was not attached before the trigger call.

This was because the leverageNative logic part for triggering first checked if
setup was done before (which, for example, is done if a jQuery handler was
registered before for this element+event pair) and - if it was not - added
a dummy handler that just returned true. The leverageNative logic made that
true then saved into private data, replacing the previous saved array. Since
true passed the truthy check, the second native inner handler treated true
as an array, crashing on the slice call.

The same issue could happen if a handler returning true is attached before
triggering. A bare length check would not be enough as the user handler may
return an array-like as well. To remove this potential data shape clash, capture
the inner result in an object with a value property instead of saving it
directly.

Since it's impossible to call alert() in unit tests, simulate the issue by
replacing the addEventListener method on a test button with a version that
calls attached blur handlers twice.

Fixes gh-5459
Ref gh-5236

main @399a78ee9fc5802509df462a2851aef1b60b7fbc
   raw     gz Filename
   +31    +12 dist/jquery.min.js
   +31    +14 dist/jquery.slim.min.js
   +31    +13 dist-module/jquery.module.min.js
   +31    +13 dist-module/jquery.slim.module.min.js

Checklist

New tests have been added to show the fix or feature works
~~If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com~~

mgol · 2024-04-02T21:25:44Z

src/event.js

-				// This technically gets the ordering wrong w.r.t. to `.trigger()` (in which the
-				// bubbling surrogate propagates *after* the non-bubbling base), but that seems
-				// less bad than duplication.


This part of the comment is no longer valid since the spec now mandates what browsers ship, i.e. that the bubbling events are fired after the non-bubbling ones.

mgol · 2024-04-02T21:44:30Z

I'm referring to #5236 here as this PR essentially reverts that one (with added tests). The rationale for simplifying the check in #5236 was valid, it's just that we now have a second case where it's needed.

src/event.js

mgol · 2024-04-12T21:22:22Z

src/event.js

 				} else if ( ( jQuery.event.special[ type ] || {} ).delegateType ) {
 					event.stopPropagation();
 				}

 			// If this is a native event triggered above, everything is now in order
 			// Fire an inner synthetic event with the original arguments
-			} else if ( saved ) {
+			} else if ( saved.length ) {


In some very edge-case'y code, it's possible to have a blur handler returning an array and falling into this check, destructuring the incorrect array. But guarding against that would be way more complex and I doubt many people will hit such an issue.

In Firefox, alert displayed just before blurring an element dispatches the native blur event twice which tripped the jQuery logic if a jQuery blur handler was not attached before the trigger call. This was because the `leverageNative` logic part for triggering first checked if setup was done before (which, for example, is done if a jQuery handler was registered before for this element+event pair) and - if it was not - added a dummy handler that just returned `true`. The `leverageNative` logic made that `true` then saved into private data, replacing the previous `saved` array. Since `true` passed the truthy check, the second native inner handler treated `true` as an array, crashing on the `slice` call. The same issue could happen if a handler returning `true` is attached before triggering. A bare `length` check would not be enough as the user handler may return an array-like as well. To remove this potential data shape clash, capture the inner result in an object with a `value` property instead of saving it directly. Since it's impossible to call `alert()` in unit tests, simulate the issue by replacing the `addEventListener` method on a test button with a version that calls attached blur handlers twice. Fixes jquerygh-5459 Closes jquerygh-5466 Ref jquerygh-5236

mgol · 2024-04-24T17:00:35Z

@timmywil @gibson042 PR updated & ready for another review.

gibson042 · 2024-04-24T18:58:46Z

src/event.js

@@ -522,11 +522,12 @@ function leverageNative( el, type, isSetup ) {
 			if ( ( event.isTrigger & 1 ) && this[ type ] ) {


I think we should explain the relevance of saved up here, rather than by implication scattered throughout the code branches.

Suggested change

if ( ( event.isTrigger & 1 ) && this[ type ] ) {

// This controller function is invoked under multiple circumstances,

// differentiated by the stored value in `saved`:

// 1. For an outer synthetic .trigger()ed event (detected by

// `event.isTrigger & 1` and non-array `saved`), it records arguments

// as an array and fires an [inner] native event to prompt state

// changes that should be observed by registered listeners (such as

// checkbox toggling and focus updating), then clears the stored value.

// 2. For an [inner] native event (detected by `saved` being

// an array), it triggers an inner synthetic event, records the

// result, and preempts propagation to further jQuery listeners.

// 3. For an inner synthetic event (detected by `event.isTrigger & 1` and

// array `saved`), it prevents double-propagation of surrogate events

// but otherwise allows everything to proceed (particularly including

// further listeners).

if ( ( event.isTrigger & 1 ) && this[ type ] ) {

Actually, can we be more specific than "non-array saved" in the first case? What other types might it be?

I added Richard's comment plus a short listing of possible saved data types, PTAL.

src/event.js

gibson042 · 2024-04-24T19:02:20Z

src/event.js

+					// There will always be at least one argument (an event object),
+					// so this array will not be confused with a leftover capture object.


We should experiment with checking isArray(saved) rather than saved.length (as both point replacements in the if conditions and as a dedicated variable like savedIsArgs = Array.isArray(saved)). If there's no size increase, then this commentary would be unnecessary.

Changing saved.length checks to Array.isArray( saved ) results in a size increase:

raw gz Filename +16 +7 dist/jquery.min.js +16 +1 dist/jquery.slim.min.js +16 +4 dist-module/jquery.module.min.js +16 0 dist-module/jquery.slim.module.min.js

src/event.js

In Firefox, alert displayed just before blurring an element dispatches the native blur event twice which tripped the jQuery logic if a jQuery blur handler was not attached before the trigger call. This was because the `leverageNative` logic part for triggering first checked if setup was done before (which, for example, is done if a jQuery handler was registered before for this element+event pair) and - if it was not - added a dummy handler that just returned `true`. The `leverageNative` logic made that `true` then saved into private data, replacing the previous `saved` array. Since `true` passed the truthy check, the second native inner handler treated `true` as an array, crashing on the `slice` call. The same issue could happen if a handler returning `true` is attached before triggering. A bare `length` check would not be enough as the user handler may return an array-like as well. To remove this potential data shape clash, capture the inner result in an object with a `value` property instead of saving it directly. Since it's impossible to call `alert()` in unit tests, simulate the issue by replacing the `addEventListener` method on a test button with a version that calls attached blur handlers twice. Fixes jquerygh-5459 Closes jquerygh-5466 Ref jquerygh-5236

mgol · 2024-04-24T22:36:28Z

main @399a78ee9fc5802509df462a2851aef1b60b7fbc
   raw     gz Filename
   +31    +12 dist/jquery.min.js
   +31    +14 dist/jquery.slim.min.js
   +31    +13 dist-module/jquery.module.min.js
   +31    +13 dist-module/jquery.slim.module.min.js

In Firefox, alert displayed just before blurring an element dispatches the native blur event twice which tripped the jQuery logic if a jQuery blur handler was not attached before the trigger call. This was because the `leverageNative` logic part for triggering first checked if setup was done before (which, for example, is done if a jQuery handler was registered before for this element+event pair) and - if it was not - added a dummy handler that just returned `true`. The `leverageNative` logic made that `true` then saved into private data, replacing the previous `saved` array. Since `true` passed the truthy check, the second native inner handler treated `true` as an array, crashing on the `slice` call. The same issue could happen if a handler returning `true` is attached before triggering. A bare `length` check would not be enough as the user handler may return an array-like as well. To remove this potential data shape clash, capture the inner result in an object with a `value` property instead of saving it directly. Since it's impossible to call `alert()` in unit tests, simulate the issue by replacing the `addEventListener` method on a test button with a version that calls attached blur handlers twice. Fixes jquerygh-5459 Closes jquerygh-5466 Ref jquerygh-5236

…eNative

In Firefox, alert displayed just before blurring an element dispatches the native blur event twice which tripped the jQuery logic if a jQuery blur handler was not attached before the trigger call. This was because the `leverageNative` logic part for triggering first checked if setup was done before (which, for example, is done if a jQuery handler was registered before for this element+event pair) and - if it was not - added a dummy handler that just returned `true`. The `leverageNative` logic made that `true` then saved into private data, replacing the previous `saved` array. Since `true` passed the truthy check, the second native inner handler treated `true` as an array, crashing on the `slice` call. The same issue could happen if a handler returning `true` is attached before triggering. A bare `length` check would not be enough as the user handler may return an array-like as well. To remove this potential data shape clash, capture the inner result in an object with a `value` property instead of saving it directly. Since it's impossible to call `alert()` in unit tests, simulate the issue by replacing the `addEventListener` method on a test button with a version that calls attached blur handlers twice. Fixes gh-5459 Closes gh-5466 Ref gh-5236 (cherry picked from commit 527fb3d)

mgol added Event Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Apr 2, 2024

mgol added this to the 3.7.2 milestone Apr 2, 2024

mgol self-assigned this Apr 2, 2024

mgol mentioned this pull request Apr 2, 2024

Triggering after an alert() in an event handler results in a JS error #5459

Closed

mgol commented Apr 2, 2024

View reviewed changes

mgol commented Apr 3, 2024

View reviewed changes

src/event.js Show resolved Hide resolved

mgol commented Apr 12, 2024

View reviewed changes

mgol force-pushed the alert-blur-gh-5459 branch from efb6e46 to ee787ff Compare April 24, 2024 08:59

mgol requested review from timmywil and gibson042 April 24, 2024 08:59

mgol force-pushed the alert-blur-gh-5459 branch from ee787ff to 53b647c Compare April 24, 2024 16:52

mgol force-pushed the alert-blur-gh-5459 branch from 53b647c to e66d718 Compare April 24, 2024 16:56

gibson042 approved these changes Apr 24, 2024

View reviewed changes

mgol force-pushed the alert-blur-gh-5459 branch from e66d718 to 4b3bb8a Compare April 24, 2024 22:35

timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Apr 25, 2024

mgol added 2 commits April 28, 2024 23:19

fixup! Event: Increase robustness of an inner native event in leverag…

1952220

…eNative

mgol force-pushed the alert-blur-gh-5459 branch from 4b3bb8a to 1952220 Compare April 28, 2024 21:53

mgol requested a review from gibson042 April 28, 2024 21:53

mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Apr 28, 2024

gibson042 approved these changes Apr 29, 2024

View reviewed changes

timmywil approved these changes May 13, 2024

View reviewed changes

timmywil removed Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels May 13, 2024

mgol merged commit 527fb3d into jquery:main May 20, 2024
13 checks passed

mgol deleted the alert-blur-gh-5459 branch May 20, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event: Increase robustness of an inner native event in leverageNative #5466

Event: Increase robustness of an inner native event in leverageNative #5466

mgol commented Apr 2, 2024 •

edited

mgol Apr 2, 2024

mgol commented Apr 2, 2024

mgol Apr 12, 2024

mgol commented Apr 24, 2024

gibson042 Apr 24, 2024

timmywil Apr 24, 2024

timmywil Apr 24, 2024

mgol Apr 28, 2024

gibson042 Apr 24, 2024

mgol Apr 28, 2024

mgol commented Apr 24, 2024

		@@ -522,11 +522,12 @@ function leverageNative( el, type, isSetup ) {
		if ( ( event.isTrigger & 1 ) && this[ type ] ) {

-			if ( ( event.isTrigger & 1 ) && this[ type ] ) {
+			// This controller function is invoked under multiple circumstances,
+			// differentiated by the stored value in `saved`:
+			// 1. For an outer synthetic .trigger()ed event (detected by
+			//    `event.isTrigger & 1` and non-array `saved`), it records arguments
+			//    as an array and fires an [inner] native event to prompt state
+			//    changes that should be observed by registered listeners (such as
+			//    checkbox toggling and focus updating), then clears the stored value.
+			// 2. For an [inner] native event (detected by `saved` being
+			//    an array), it triggers an inner synthetic event, records the
+			//    result, and preempts propagation to further jQuery listeners.
+			// 3. For an inner synthetic event (detected by `event.isTrigger & 1` and
+			//    array `saved`), it prevents double-propagation of surrogate events
+			//    but otherwise allows everything to proceed (particularly including
+			//    further listeners).
+			if ( ( event.isTrigger & 1 ) && this[ type ] ) {

		// There will always be at least one argument (an event object),
		// so this array will not be confused with a leftover capture object.

Event: Increase robustness of an inner native event in leverageNative #5466

Event: Increase robustness of an inner native event in leverageNative #5466

Conversation

mgol commented Apr 2, 2024 • edited

Summary

Checklist

Choose a reason for hiding this comment

mgol commented Apr 2, 2024

Choose a reason for hiding this comment

mgol commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Apr 24, 2024

mgol commented Apr 2, 2024 •

edited