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

Deferred: Remove default callback context #3061

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
6 participants
@gibson042
Member

gibson042 commented Apr 15, 2016

Fixes gh-3060

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 15, 2016

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

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

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 22, 2016

Member

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?

Member

gibson042 commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 22, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 22, 2016

Member

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...

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

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 22, 2016

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.

Member

markelog commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 22, 2016

Member

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.

Member

gibson042 commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 22, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 22, 2016

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.

Member

markelog commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 22, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 22, 2016

Member

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

Member

gibson042 commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 22, 2016

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.

Member

dmethvin commented Apr 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 23, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Apr 23, 2016

Member

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.

Member

gibson042 commented Apr 23, 2016

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

Build: Workaround strict mode violations caused by UglifyJS
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/UglifyJS2#1052

@markelog markelog added the Blocker label Apr 25, 2016

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 27, 2016

Member

I added a note in the upgrade guide.

Member

dmethvin commented Apr 27, 2016

I added a note in the upgrade guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment