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

Switch to script injection instead of indirect eval call #1449

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@markelog
Copy link
Member

commented Nov 29, 2013

Right now, 2.x uses two techincs for code execution in global scope: script injection if "use strict" statement is found and indirect eval call if not. 1.x only uses eval path.

With indirect eval call, we have only one problem - if evaluated code uses strict pragma, it will not be executed in global scope, which subverts function purpose. Although current code tries to circumvent it, but fails on edge cases.

With script injection we have four problems:

  1. It requires browser environments, i.e. DOM
  2. It might be executed asynchronously
  3. Errors do not propagate, but instead would be thrown in different flow, meaning you can't catch them without window.onerror
  4. Potential performance penalty

One common and obvious issue is difference of globalEval functionality between branches. Because now we have all those problems combine, so i propose to start using only one technic, this PR will try to do it with script injection.

Now about issues with this way:

It requires browser env, i.e. DOM

Well, jQuery is DOM-centric library, in other environments you should use other means

It might be executed asynchronously

It's no longer true, since Firefox 4.0, but just to be sure, i added a test for it.

Errors do not propagate

Yes, that would be a incompatible difference

Potential perfomance penalty

It's actually the opposite (it's not my test if anyone asks :-)

I think the important thing is that jQuery should use only one technic and it should be identical between branches, i, personally, could live with any of them.

/cc @dmethvin, @rwaldron, @jdalton, @jaubourg

@jaubourg

This comment has been minimized.

Copy link
Member

commented Nov 29, 2013

Could you remove the unrelated changes in test/unit/ajax.js and test/unit/manipulation.js?

Now that odd async behaviour is clearly a thing of the past, I'm 100% behind you on using script tag injection all the time.

@markelog

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2013

@jaubourg

Could you remove the unrelated changes

That's sad part –

3) Errors do not propagate, but instead would be thrown in different flow, meaning you can't catch them without window.onerror
@jaubourg

This comment has been minimized.

Copy link
Member

commented Nov 29, 2013

Oh yes! I had forgotten that was the thing using eval actually permitted.

@curiousdannii

This comment has been minimized.

Copy link

commented Nov 29, 2013

I assume the Function constructor can't be used?

@markelog

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2013

I assume the Function constructor can't be used?

http://perfectionkills.com/global-eval-what-are-the-options/

@rwaldron

This comment has been minimized.

Copy link
Member

commented Nov 29, 2013

IIRC, I had this for 2.x, I'm glad to see it finally happening :)

var script = document.createElement( "script" );

script.text = code;
document.head.appendChild( script ).parentNode.removeChild( script );

This comment has been minimized.

Copy link
@UncleBill

UncleBill Dec 9, 2013

Hey, how about using Script URL to eval global code?

diff --git a/src/core.js b/src/core.js
index 994fd4c..62ad085 100644
--- a/src/core.js
+++ b/src/core.js
@@ -281,8 +281,7 @@ jQuery.extend({

    // Evaluates a script in a global context
    globalEval: function( code ) {
-       var script,
-           indirect = eval;
+       var indirect = eval;

        code = jQuery.trim( code );

@@ -291,9 +290,7 @@ jQuery.extend({
            // strict mode pragma, execute code by injecting a
            // script tag into the document.
            if ( code.indexOf("use strict") === 1 ) {
-               script = document.createElement("script");
-               script.text = code;
-               document.head.appendChild( script ).parentNode.removeChild( script );
+                location.href = "javascript:" + code;
            } else {
            // Otherwise, avoid the DOM node creation, insertion
            // and removal by using an indirect global eval

This comment has been minimized.

Copy link
@dmethvin

dmethvin Dec 10, 2013

Member

Give it a try with some test cases. I don't think it globally evaluates the script reliably.

This comment has been minimized.

Copy link
@markelog

markelog Dec 10, 2013

Author Member

@UncleBill a lot of problems with this one, in some browsers for some constructions it would indeed change the URL, in some browsers it would be a global eval in some browsers it would not, in some browsers it would be async execution in some browsers it would not.

This comment has been minimized.

Copy link
@UncleBill

UncleBill Dec 11, 2013

OK, I got it.Thanks @markelog

@dmethvin dmethvin added this to the 1.12/2.2 milestone Feb 7, 2014

gibson042 referenced this pull request Feb 18, 2014

@gibson042

This comment has been minimized.

Copy link
Member

commented Feb 19, 2014

  1. Errors do not propagate, but instead would be thrown in different flow, meaning you can't catch them without window.onerror

This has some consequences in both ajax and manipulation. I think we should agree on it as a team before committing anything, and synchronize behavior between both branches to match the decision.

@markelog

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2014

@gibson042

This has some consequences in both ajax and manipulation

It is, that's why i mentioned it, right now we have inconsistency between branches because this consequence already exist if evaluated code uses "use strict" statement, i don't really care what way we choose, but we have to choose one, although

Errors do not propagate

Looks more logical and native, this PR is three months old now, so if you don't have any arguments against i don't really see the point to wait any longer.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2014

The important point about error behavior being changed didn't really register with me. Maybe it's all the discussion about Promises making things clearer. It would be worth some careful thought here about what might be broken and the options we can offer people to get the old behavior back.

@markelog

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2014

It would be worth some careful thought here about what might be broken and the options we can offer people to get the old behavior back

There is only one thing that could be broken - "Errors do not propagate", if we want to save this, i could provide a PR that would work with indirect eval call, i think we just need to choose.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Feb 20, 2014

I agree that on our side, it comes down to "errors do not propagate" but we do not know the impact on the code base of our users. The fact that we weren't able to be consistent here in the past works to our advantage I think,.

There could be situations where someone might have only gotten files from a the same domain and thus depended on catching errors, but I am not sure how we would be able to find those situations and warn people about them. Perhaps we could shim this somehow in jQuery Migrate? In any case this needs to be documented really well since it could break code. (Bad code 👅 )

On the timing of landing this, I suspect we'll want a 1.11.1/2.1.1 release to wrap up a few bug fixes. This would definitely be a 1.12/2.2 thing.

@markelog

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2014

Perhaps we could shim this somehow in jQuery Migrate

We could use the indirect call in there.

I suspect we'll want a 1.11.1/2.1.1 release

Definitely, if we doing that, this change should not be introduced in patch release.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Feb 21, 2014

We could definitely use the indirect call in Migrate, but I was wondering how we could detect and warn about a situation where the new script-tag method would hose them. Those warnings are the main job of Migrate. We could try/catch and warn about it if we saw an error, then rethrow. But any successful code that somehow expected synchronous execution would be hard to detect.

@dmethvin

This comment has been minimized.

Copy link
Member

commented Mar 1, 2014

@mgol

This comment has been minimized.

Copy link
Member

commented May 5, 2014

This will need a similar workaround in manipulation tests for Android 2.3 as in #1573.

@mgol

This comment has been minimized.

Copy link
Member

commented Jul 22, 2015

This PR seems to make Android fail AJAX tests even on compat (previously it failed only on master). That's one of the reasons I created #2483; supporting Android 2.3 fully would sometimes mean reverting things like this PR and I'd like to avoid catering to this browser so much.

@markelog

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2015

Okay, if we leave 2.3, i will investigate, if not, then lets leave it as is.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

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