Bug Tracker

Opened 7 years ago

Closed 6 years ago

#14189 closed bug (fixed)

globalEval handling of "use strict" pragma incorrect.

Reported by: tsherif@… Owned by:
Priority: low Milestone: 1.12/2.2
Component: core Version: 2.0.3
Keywords: Cc: jaubourg
Blocked by: Blocking:

Description

The function globalEval() assumes that if a "use strict" pragma is present in the code, it must be the first thing:

if ( code.indexOf("use strict") === 1 ) {
  script = document.createElement("script");
  script.text = code;
  document.head.appendChild( script ).parentNode.removeChild( script );
} else {
  // Otherwise, avoid the DOM node creation, insertion
  // and removal by using an indirect global eval
  indirect( code );
}

This is incorrect, as "use strict" must simply be the first statement, and may come after comments. I was unable to load the three.js library (http://threejs.org/build/three.min.js) through ajax (jQuery.get, jQuery.getScript, jQuery.ajax) because of this.

I think possible solutions would be to strip comments (instead of just trimming whitespace) before doing the test, or to test that "use strict" comes before the first semicolon.

Change History (9)

comment:1 Changed 7 years ago by dmethvin

Seems like this just leads to a deeper rabbit hole. Trying to "parse" the script and ignore leading comments/spaces is going to be messy and time consuming.

comment:2 Changed 7 years ago by tsherif@…

I think the time spent stripping leading comments would likely be less than that of inserting and removing unnecessary dom elements, but if it is too complicated, then it would probably be best to just always insert the script element, since it's the only way, without some preprocessing, to be sure the script will execute in the global context.

comment:3 Changed 7 years ago by jaubourg

For ajax, you can just use crossDomaine: true in your option to force usage of the script transport.

As for a fix for this for globalEval, yes, using inline script tag injection is neat but there is no guarantee it'll execute synchronously. It failed in the past.

comment:4 Changed 6 years ago by tsherif@…

I'm not sure I understand. If script tag injection is problematic, then why is it being used here for strict mode? What I'm suggesting is to always evaluate the code in the same way (injecting script tags) in both cases, and not have this broken test for strict mode. Or to fix the test for strict mode so that it actually works (stripping the comments, or otherwise properly checking for the directive).

Using eval on code in strict mode will not evaluate it in the global context, so this function simply does not work if there are any comments before the "use strict" directive in the code.

comment:5 Changed 6 years ago by dmethvin

To me, any detection of "use strict" in *global* eval is incorrect since it should not be used in global scope. So the remaining question would be why we check for this at all. @jaubourg do you know?

comment:6 Changed 6 years ago by timmywil

Cc: jaubourg added
Component: unfiledcore
Priority: undecidedlow
Status: newopen

comment:7 Changed 6 years ago by jaubourg

After careful consideration, I think we should use script tag injection all the time. I remember asking rick about the two code paths back in the days and he answered something about performance concerns. Not sure the impact of injecting a script tag is that much of a concern here.

comment:8 Changed 6 years ago by dmethvin

Milestone: None1.12/2.2

We're planning to use script tags always in 1.12/2.2, see #14757. I'll leave this one open separately.

comment:9 Changed 6 years ago by m_gol

Resolution: fixed
Status: openclosed

#14757 was fixed so there doesn't seem to be much more to do here. Closing.

Note: See TracTickets for help on using tickets.