Last Comment Bug 383381 - docs.google.com depends on indirect eval
: docs.google.com depends on indirect eval
Status: VERIFIED FIXED
: regression, verified1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
http://spreadsheets.google.com/ccc?ke...
Depends on:
Blocks: 382509
  Show dependency treegraph
 
Reported: 2007-06-05 15:26 PDT by Carsten Book [:Tomcat] - PTO-back Sept 4th
Modified: 2008-04-09 21:00 PDT (History)
13 users (show)
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Doc http://spreadsheets.google.com/ccc?key=p-9pXWWpA7Bih6yFR9FYQWw in current nightly (103.56 KB, image/jpeg)
2007-06-05 15:26 PDT, Carsten Book [:Tomcat] - PTO-back Sept 4th
no flags Details
Proposed fix (1.28 KB, patch)
2007-06-05 16:02 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
With some CSE (1.38 KB, patch)
2007-06-05 16:41 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
Details | Diff | Splinter Review

Description Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-06-05 15:26:01 PDT
Created attachment 267341 [details]
Doc http://spreadsheets.google.com/ccc?key=p-9pXWWpA7Bih6yFR9FYQWw in current nightly

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a6pre) Gecko/20070605 Minefield/3.0a6pre ID:2007060504 [cairo]

Somes Spreadsheets from docs.google.com are currently broken in the latest Minefield Trunk. When i login to docs.google.com everything is fine, the Problem is when i try to open a Spreadsheet, see screenshot.

STR: open a docs.google.com doc like http://spreadsheets.google.com/ccc?key=p-9pXWWpA7Bih6yFR9FYQWw

See a broken spreadsheet.

This is a regression since its working in 20070603 and broken in 20070605. Will try to find a better regression range.

Relevant entries from the Error Console:

Error: this.Qc has no properties
Source File: http://spreadsheets.google.com/client/js/3593144427-trix_main.js
Line: 673

Error: function eval must be called directly, and not by way of a function of another name
Source File: http://spreadsheets.google.com/client/js/3593144427-trix_main.js
Line: 629

Reproduces on Windows xp x64 SP2, Linux Fedora FC 7 and Vista
Comment 1 Stephen Donner [:stephend] 2007-06-05 15:36:55 PDT
Looks like, according to sp3000's findings, that this is caused by bug 382509.
Comment 2 Blake Kaplan (:mrbkap) 2007-06-05 15:40:40 PDT
Indeed. They set Da=eval and later: d=Da(e), which will throw. Brendan, perhaps we need to allow eval to be called via other function names, just not on other objects?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2007-06-05 15:51:41 PDT
That basically makes it impossible to do name-capture optimizations in the future.
Comment 4 Blake Kaplan (:mrbkap) 2007-06-05 16:02:10 PDT
Created attachment 267352 [details] [diff] [review]
Proposed fix

Okay, this backs off of requiring eval to be called "eval"(...).
Comment 5 Brendan Eich [:brendan] 2007-06-05 16:24:26 PDT
It would be good to list all the web apps (google or other) that depend on indirect eval called via a name other than eval (either qualified, o.eval(s), or not, eval(s)).

If we can't make optimizations based on lack of e-v-a-l being the identifier of a method invoked from a function, then we will either be unable to optimize, or we'll simply not expose optimized bindings to such an eval (which is what we do today).

/be
Comment 6 Tony Chang (Google) 2007-06-05 16:33:49 PDT
This will likely happen in all google javascript code that uses eval.  It could be prevented if needed, but that would result in an increase in JS code size.
Comment 7 Blake Kaplan (:mrbkap) 2007-06-05 16:41:12 PDT
Created attachment 267358 [details] [diff] [review]
With some CSE

Crowder pointed out on IRC that we want to avoid doing the OBJ_GET_PARENT twice. Note that scopeobj will always be null after the if statement (since if it wasn't, we'd have reported an error and returned).
Comment 8 Blake Kaplan (:mrbkap) 2007-06-05 16:43:11 PDT
Fix checked into trunk.
Comment 9 Blake Kaplan (:mrbkap) 2007-06-05 16:58:32 PDT
(In reply to comment #6)
> This will likely happen in all google javascript code that uses eval.  It could
> be prevented if needed, but that would result in an increase in JS code size.

The codesize savings here are 2 bytes per callsite, or at most 3 (subtract 7 or 8 bytes for the original assignment). I feel like that shouldn't be the main reason to have to do this.
Comment 10 Brendan Eich [:brendan] 2007-06-05 17:01:09 PDT
(In reply to comment #9)
> (In reply to comment #6)
> The codesize savings here are 2 bytes per callsite, or at most 3 (subtract 7 or
> 8 bytes for the original assignment). I feel like that shouldn't be the main
> reason to have to do this.

OTOH, I think this is set in stone, and must be part of ES4/JS2. We can specify that indirecting through a non-global object is an EvalError, and indirecting via a name other than 'eval' scopes to the global object. That's what SpiderMonkey has done for all such misnamed eval calls, unless it had other reasons to deoptimize name to slot binding in the caller.

/be
Comment 11 Igor Bukanov 2007-06-06 04:35:54 PDT
(In reply to comment #10)
> OTOH, I think this is set in stone, and must be part of ES4/JS2. We can specify
> that indirecting through a non-global object is an EvalError, and indirecting
> via a name other than 'eval' scopes to the global object. That's what
> SpiderMonkey has done for all such misnamed eval calls, unless it had other
> reasons to deoptimize name to slot binding in the caller.

I think another possibility is to disallow var statements in the indirect eval. AFAICS it will work for Google and may work for most of the web.
Comment 12 Brendan Eich [:brendan] 2007-06-06 16:35:02 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > OTOH, I think this is set in stone, and must be part of ES4/JS2. We can specify
> > that indirecting through a non-global object is an EvalError, and indirecting
> > via a name other than 'eval' scopes to the global object. That's what
> > SpiderMonkey has done for all such misnamed eval calls, unless it had other
> > reasons to deoptimize name to slot binding in the caller.
> 
> I think another possibility is to disallow var statements in the indirect eval.

It will break real code, including but not limited to

http://www.yui-ext.com/deploy/ext-1.0-alpha3/source/core/DomQuery.js

I don't think we can do this aggressive an incompatible change.

> AFAICS it will work for Google and may work for most of the web.

The ES4 proposal is to have 'use strict' make bindings that would leak out of eval be errors (compile-time for the compilation caused by that eval; runtime since eval is called at runtime).

/be
Comment 13 Igor Bukanov 2007-06-06 16:42:44 PDT
(In reply to comment #12)
> > I think another possibility is to disallow var statements in the indirect eval.
> 
> It will break real code, including but not limited to
> 
> http://www.yui-ext.com/deploy/ext-1.0-alpha3/source/core/DomQuery.js

But that code only uses direct eval, there is no eval indirection there.
Comment 14 Igor Bukanov 2007-06-06 16:46:44 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > It will break real code, including but not limited to
> > 
> > http://www.yui-ext.com/deploy/ext-1.0-alpha3/source/core/DomQuery.js
> 
> But that code only uses direct eval, there is no eval indirection there.

To be precise, my hope is that amount of code that uses both indirect eval and var statements there is sufficiently small.

Comment 15 Brendan Eich [:brendan] 2007-06-06 16:59:47 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > > I think another possibility is to disallow var statements in the indirect eval.
> > 
> > It will break real code, including but not limited to
> > 
> > http://www.yui-ext.com/deploy/ext-1.0-alpha3/source/core/DomQuery.js
> 
> But that code only uses direct eval, there is no eval indirection there.

Right, sorry -- was thinking only of the eval("var ...") case, which is generally poor style. In DomQuery.js it is unnecessary, since the var is single-use and can be eliminated.

I'll add your idea to the ES4 eval wiki page pretty soon, or feel free to do it yourself if you prefer.

/be
Comment 16 Brendan Eich [:brendan] 2007-06-06 17:07:42 PDT
Igor, we should take this offline, but the problem is not just var (or function -- or const if supported) in the eval'ed program. Just seeing outer bindings may be hard for a small implementation to support, since it requires reflecting stack slots back into names bound in environment objects.

Misnamed eval is just a pain for implementations to cope with. I'm to blame, and I have to say that the SpiderMonkey code to be lazy yet consistent when its time to reflect is hairy. For ES4 we would like to avoid requiring this code. Not sure that using the global scope only is enough to tame misnamed eval in this sense (get rid of code), however.

/be
Comment 17 Bob Clary [:bc:] 2007-06-12 14:07:11 PDT
indirect eval covered by js1_4/Eval/eval-00[123].js
Comment 18 Bob Clary [:bc:] 2007-09-18 10:48:59 PDT
verified fixed 1.9.0 linux/mac*/windows
Comment 19 Tony Chang (Google) 2008-01-08 16:00:01 PST
FWIW, sites hosted by google should no longer use indirect eval to maintain compatibility.
Comment 20 Samuel Sidler (old account; do not CC) 2008-03-07 10:27:24 PST
Fixed on the branch in bug 382509.
Comment 21 Bob Clary [:bc:] 2008-03-17 05:55:41 PDT
v 1.8.1 linux|mac|win32

Note You need to log in before you can comment on or make changes to this bug.