Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

saved.shift is not a function #4350

Closed
dsandber opened this issue Apr 11, 2019 · 27 comments

Comments

Projects
None yet
@dsandber
Copy link

commented Apr 11, 2019

My app auto-upgraded to 3.4.0 from 3.3.1 and broke with this error:

saved.shift is not a function

I'll append the browser version in a comment shortly.

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

user_agent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.86 Safari/537.36"

trace: "TypeError: saved.shift is not a function
at HTMLDivElement.handler (...)
at HTMLDivElement.dispatch (...)
at HTMLDivElement.elemData.handle (...)

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Sorry, there is not enough information to go on here. You can find help on https://stackoverflow.com. If after sorting out the issue you find there is a change needed in jQuery core, please open a new issue and follow the instructions in the issue template, including providing a test case from https://jsbin.com.

@timmywil timmywil closed this Apr 11, 2019

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

Sorry, because it wasn't triggered directly by my code, and because it doesn't seem to happen consistently, I don't know how to create a test case.

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Fair enough, but the issue needs to be fleshed out more regardless.

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

Luckily "saved.shift" only occurs once in the source-code. Strange that the comment says IE <= 9 - 11+ but that it is being run on Chrome.

dataPriv.set( this, type, jQuery.event.trigger(
  // Support: IE <=9 - 11+
  // Extend with the prototype to reset the above stopImmediatePropagation()
  jQuery.extend( saved.shift(), jQuery.Event.prototype ),
  saved,
  this
) );

Seems to happen because saved is not an array, example:

> let saved = ''
> saved.shift()
VM116:1 Uncaught TypeError: saved.shift is not a function
@mgol

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@dsandber Unfortunately, we cannot do much without a test case we can run, the error itself doesn't say much. We usually require a small test case without external dependencies as most errors user have is actually a bug on their app part but since this code is new in 3.4.0 & it's one of the most trickiest new part, it's possible we broke something so I'd be open to looking at a slightly bigger test case. But I still need at least a website with this bug, with an easy way to reproduce it myself. Ideally I'd have both one with jQuery 3.3.1 & 3.4.0 to compare but I could try with just 3.4.0 as well if the older one is not available.

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

Yeah, I understand -- probably best to wait until more people report this error and hopefully someone can develop a simple test case. I literally wouldn't know where to start since the stack trace doesn't include my code, the error doesn't happen consistently, and I don't understand what the code I quoted does.

@fancyapps

This comment has been minimized.

Copy link

commented Apr 12, 2019

I noticed this error message after upgrading to 3.4.0., too. I could not figure out when/why, I`ll try to create a minimal example.

@XhmikosR

This comment has been minimized.

Copy link

commented Apr 13, 2019

@mgol we hit this in Bootstrap 4.x https://travis-ci.org/twbs/bootstrap/jobs/519620443

Going back to 3.3.1 our tests pass.

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

Given three independent reports, perhaps the bug should be re-opened to make it easier for others to find? (since closed bugs aren't searched by default)

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Reopening it is fine, but we still don't have a test case. Can we at least get a stack trace?

@dmethvin dmethvin reopened this Apr 13, 2019

@XhmikosR

This comment has been minimized.

Copy link

commented Apr 13, 2019

The stacktrace is shown in the Travis build, but it's using the minified build. AFAICT it only affects IE 10/11, at least in our test suite.

@dsandber

This comment has been minimized.

Copy link
Author

commented Apr 13, 2019

Can we at least get a stack trace?

My second comment includes a stack trace.

AFAICT it only affects IE 10,

Chrome too.

@XhmikosR

This comment has been minimized.

Copy link

commented Apr 13, 2019

I also pushed a patch with the non-minified build here: https://travis-ci.org/twbs/bootstrap/jobs/519638107

@dmethvin

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I was thinking those stack traces were truncated, but they're complete (from the point of event invocation) but just not informative. Some code is triggering an event of some type and it causes an error. If we could fill in the two "some" with specifics we could diagnose the issue.

@XhmikosR

This comment has been minimized.

Copy link

commented Apr 13, 2019

One test that fails is doing the following:

// Key escape
$input.trigger('focus').trigger($.Event('keydown', {
  which: 27
}))
assert.ok(!$dropdown.parent('.dropdown').hasClass('show'), 'dropdown menu is not shown')

BTW I'm also getting a Symbol is not a function error on IE 10 with 3.4.0.

@gibson042

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

As @dsandber observed, the source text occurs only once in jQuery, in a context like this:

	𝘀𝗮𝘃𝗲𝗱 = 𝗱𝗮𝘁𝗮𝗣𝗿𝗶𝘃.𝗴𝗲𝘁( 𝘁𝗵𝗶𝘀, 𝘁𝘆𝗽𝗲 );

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

	// Interrupt processing of the outer synthetic .trigger()ed event
	if ( !saved ) {

		// Store arguments for use when handling the inner native event
		saved = slice.call( arguments );
		𝗱𝗮𝘁𝗮𝗣𝗿𝗶𝘃.𝘀𝗲𝘁( 𝘁𝗵𝗶𝘀, 𝘁𝘆𝗽𝗲, 𝘀𝗮𝘃𝗲𝗱 );
		…
	}
	…

// 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 ) {

	// ...and capture the result
	𝗱𝗮𝘁𝗮𝗣𝗿𝗶𝘃.𝘀𝗲𝘁( this, type, jQuery.event.trigger(

		// Support: IE <=9 - 11+
		// Extend with the prototype to reset the above stopImmediatePropagation()
		jQuery.extend( 𝘀𝗮𝘃𝗲𝗱.𝘀𝗵𝗶𝗳𝘁(), jQuery.Event.prototype ),
		saved,
		this
	) );

type comes from leverageNative as either "click", "focus", or "blur", and dataPriv is a Data map that should be library-internal but is actually exposed to consumer code via jQuery._data.

We expect dataPriv.get( this, type ) to be fully under the control of leverageNative, and thus either false or an array of trigger data set in outermost event handling. But if some other code is using jQuery._data to set e.g. "focus" data to a non-false non-array value, then we would see precisely this kind of error—and I strongly suspect that's what's going on. We specifically addressed this point before landing the new code, but obviously can only protect against internal collisions rather than external.

We need to decide whether to rename the private data key or leave this as a purely consumer issue.
Can everyone experiencing problems please find the use of jQuery._data that messes with "focus" and/or "blur" and/or "click"?

@fancyapps

This comment has been minimized.

Copy link

commented Apr 13, 2019

Hi! This simple test case fails using IE11 after clicking link the second time:

<p>
  <a href="#" id="test-ie11">Click me</a>
</p>

<script>
  $('#test-ie11').click(function(e) {
    $(this).trigger("blur");
    return false;
  });
</script>

Tracing does not give much info (same as in previous comments), but saved variable is Boolean (true) instead of object.

@gibson042

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Thanks so much @fancyapps, this is real and I feel pretty dumb right now. The issue is that in environments where native focus/blur are async, the leverageNative native event handling still sets private data, which results in an error on the next triggered invocation: https://jsbin.com/mizironoha

I think a fix needs to update detection of outer synthetic events to use not just !saved but also absence of sentinel data that can never be returned by the jQuery.event.trigger in native event handling. Before optimizing for size, I'm thinking something like https://jsbin.com/jozeromequ (in which the // UPDATE lines demonstrate prepending leverageNative itself to the saved array for this purpose).

And I also noticed in passing that the focus/blur setup and trigger handlers call leverageNative every time rather than guarding it with a dataPriv.get call as the click setup and trigger handler do, so we should fix that as well to avoid unbounded addition of returnTrue handlers.

@mgol mgol added Bug Event labels Apr 13, 2019

@mgol mgol added this to the 3.4.1 milestone Apr 13, 2019

@mgol

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I added this to the newly created 3.4.1 milestone.

@ewolfman

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hi,
When is the 3.4.1 release date?

@timmywil

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

There's no set date, but it will probably be next week or the week after.

davidbludlow added a commit to graspable/mathquill-webpack that referenced this issue Apr 25, 2019

Downgraded jquery
because of jquery/jquery#4350 .
This commit should be undone when
jquery/jquery#4354 has made it into the latest
version of jquery (probably jquery version 3.4.1)

davidbludlow added a commit to graspable/mathquill-webpack that referenced this issue Apr 25, 2019

Downgraded jquery
because of jquery/jquery#4350 .
This commit should be undone when
jquery/jquery#4354 has made it into the latest
version of jquery (probably jquery version 3.4.1)
@dmnd

This comment has been minimized.

Copy link

commented Apr 25, 2019

I just broke production because of this bug.

GitHub currently pushes recommendations to all repos using jquery <3.4.0 to upgrade due to CVE-2019-5428. I saw that, so I checked out the release blog post which currently says:

There should be no compatibility issues if upgrading from jQuery 3.0+.

So after some brief sanity testing (which seemed fine) I deployed and then errors started arriving.

Given this bug, I recommend you change the wording on the blog post for 3.4.0.

mgol added a commit that referenced this issue Apr 29, 2019

@mgol

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Fixed by PR #4354 (commit 24d71ac).

@mgol mgol closed this in ddfa837 Apr 29, 2019

@vincentwoo

This comment has been minimized.

Copy link

commented May 1, 2019

Any word on when a 3.4.1 release will be cut?

@jcubic

This comment has been minimized.

Copy link

commented May 1, 2019

@vincentwoo I'm also interested when it will be out, It seems that only iOS 10 test are waiting https://github.com/jquery/jquery/milestone/20

@timmywil

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.