Skip to content

Conversation

@gibson042
Copy link
Member

Fixes gh-3060

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @markelog, @timmywil and @jaubourg to be potential reviewers

@gibson042
Copy link
Member Author

Deeper and deeper... af85a48 broke the Promises/A+ strict-mode assertions, and there are two ways to fix it. The simpler (and smaller) of the two is just letting jQuery run in strict mode, and the more complex involves preserving Callbacks#fireWith context. I've left commits for both on this PR so we can make a fully-informed decision.

My personal inclination is to go strict mode, with justification documented in 97763540:

But as of jQuery 3.0 (2016), strict mode should be common enough that all such attempts [to access strict mode arguments.callee.caller] are guarded in a try block.

However, three years ago trac-13335 was considered a big enough deal that we removed "use strict" just three weeks after introducing it, and today every browser engine except Blink throws an exception when a non-strict function tries to access a strict caller. The question is, has enough time elapsed that we now feel comfortable with reintroducing it and wontfixing any reports about ill-advised code that tries to naïvely callee.caller its way up a stack?

@mgol
Copy link
Member

mgol commented Apr 22, 2016

Id really love us to go to strict mode if possible. The concerns about tracing strict code from sloppy one are permanent as AFAIK the spec forbids such access. We're thinking about moving to ES6 modules one day; if we're going to expose them similarly to how we expose our AMD modules in src/ now, we have to remember ES6 modules are always strict so if we didn't put the pragma in the built file, we'd be basically running our code sometimes in strict, sometimes in sloppy mode.

I'd also like to point out a comment from one Chromium issue (https://bugs.chromium.org/p/v8/issues/detail?id=222#c13):

JavaScript only supports (and is only intended to support) such security concerns for strict code, and for contexts that enforce that all untrusted code must be strict and has no access to any sloppy functions. For these, aFunction.arguments is either poisoned or absent.

More & more libraries use strict mode. Angular, Ember & React do so if you use a JS framework you most likely already have it enabled somewhere. Bootstrap & Foundation do so if you use a popular CSS framework it's taken care of as well. Three.js enables strict mode as well.

I'd argue the majority of our users might be already using strict mode via various libraries that they use. Us not doing that won't help it but it allows external code to mess with jQuery more, introduces potential security concerns and puts us further from ES6 modules.

@mgol
Copy link
Member

mgol commented Apr 22, 2016

Deeper and deeper... af85a48 broke the Promises/A+ strict-mode assertions

Although we still support IE 9 which doesn't support strict mode (AFAIK the only browser we still support that doesn't) and our test suite should still pass in them so perhaps there's no way around that just via enabling strict mode...

@markelog
Copy link
Member

We can try it again and release 3.0.1 if users wouldn't like it

@gibson042

Deeper and deeper... af85a48 broke the Promises/A+ strict-mode assertions, and there are two ways to fix it.

As i understand those tests expect for context to be undefined rather then window? It seems like a bug in the tests though.

@gibson042 gibson042 added this to the 3.0.0 milestone Apr 22, 2016
@gibson042
Copy link
Member Author

The concerns about tracing strict code from sloppy one are permanent as AFAIK the spec forbids such access.

…but doesn't require throwing an exception. Chrome, for example, just returns a null caller: https://jsfiddle.net/d0vaf1uL/

More & more libraries use strict mode.

Right, which serves as good defense for jQuery doing so as well.

Although we still support IE 9 which doesn't support strict mode

But that's fine; it just means "use strict" is a harmless string literal in that environment (and .call() results in global-object window context).

As i understand those tests expect for context to be undefined rather then window? It seems like a bug in the tests though.

It's not a bug. promise.then(strictModeFunction) should have undefined context, even though promise.then(sloppyFunction) has global-object context, because the spec requires callbacks to be invoked as functions and that invocation has observable differences in strict mode: https://promisesaplus.com/#point-69 .

At any rate, it looks like jumping into strict mode is the way to go for 3.0. I'll land it later today.

@markelog
Copy link
Member

markelog commented Apr 22, 2016

It's not a bug.

By that i meant that test should check what mode they are in, if strict - then assertion should check for undefined, if sloppy then for global.

Otherwise, those tests suggesting that Promise/A+ spec is not compatible with sloppy mode, which is not true.

So

promise.then(strictModeFunction) should have undefined context, even though promise.then(sloppyFunction) has global-object context

That exactly what they should be testing

@markelog
Copy link
Member

Like it said in https://promisesaplus.com/#point-69

That is, in strict mode this will be undefined inside of them; in sloppy mode, it will be the global object.

@markelog
Copy link
Member

markelog commented Apr 22, 2016

In any case, i'm not oppose to land, but if we would have issues with strict mode again, then we would have to revert so we would face that issue all over again.

@gibson042
Copy link
Member Author

test should check what mode they are in, if strict - then assertion should check for undefined, if sloppy then for global.

They check both, explicitly: https://github.com/promises-aplus/promises-tests/blob/0d409df278345753c9527539a5558db67d42bb02/lib/tests/2.2.5.js

@dmethvin
Copy link
Member

I'm good with another shot at "use strict" for 3.0. This is another upgrade guide item and one that needs to get attention. I've moved it from the Google doc to https://github.com/jquery/jquery.com/blob/master/pages/upgrade-guide/3.0.md I will add this once it lands.

@mgol
Copy link
Member

mgol commented Apr 23, 2016

Remember that we now need not only to uncomment the pragma in wrapper.js
but also add it to every single AMD module in src.

Michał Gołębiowski

@gibson042
Copy link
Member Author

Landed, and 3a80ba5 should provide a good starting point if it turns out that we do want to revert strict mode without breaking #3060.

Also, and for similar reasons, I decided to separate module-level strict mode adoption into its own issue at #3073.

markelog referenced this pull request Apr 24, 2016
This commit increases the gzipped size by 90 bytes so a better solution
is needed but, at the same time, it disables the very optimizations
that are causing strict mode violations in Firefox 45, Safari 9 & IE 10.

Refs 7608437
Refs mishoo/UglifyJS#1052
@dmethvin
Copy link
Member

I added a note in the upgrade guide.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

6 participants