docs.google.com depends on indirect eval

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Tomcat, Assigned: mrbkap)

Tracking

({regression, verified1.8.1.13})

Trunk
x86
All
regression, verified1.8.1.13
Points:
---
Bug Flags:
blocking1.8.1.13 +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
Flags: blocking-firefox3?
Blocks: 382509
Looks like, according to sp3000's findings, that this is caused by bug 382509.
No longer blocks: 382509
Blocks: 382509
Assignee: nobody → general
Component: General → JavaScript Engine
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → general
(Assignee)

Comment 2

10 years ago
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?
(Assignee)

Updated

10 years ago
Summary: Some Spreadsheets from docs.google.com broken in current trunk → docs.google.com depends on indirect eval
That basically makes it impossible to do name-capture optimizations in the future.
(Assignee)

Comment 4

10 years ago
Created attachment 267352 [details] [diff] [review]
Proposed fix

Okay, this backs off of requiring eval to be called "eval"(...).
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #267352 - Flags: review?(brendan)
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

Updated

10 years ago
Attachment #267352 - Flags: review?(brendan) → review+

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
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).
Attachment #267352 - Attachment is obsolete: true
Attachment #267358 - Flags: review+
(Assignee)

Comment 8

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

10 years ago
(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.
(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

10 years ago
(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.
(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

10 years ago
(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

10 years ago
(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.

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

10 years ago
indirect eval covered by js1_4/Eval/eval-00[123].js
Flags: in-testsuite+

Comment 18

10 years ago
verified fixed 1.9.0 linux/mac*/windows
Status: RESOLVED → VERIFIED
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.12?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?

Comment 19

10 years ago
FWIW, sites hosted by google should no longer use indirect eval to maintain compatibility.
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Fixed on the branch in bug 382509.
Keywords: fixed1.8.1.13

Comment 21

9 years ago
v 1.8.1 linux|mac|win32
Keywords: fixed1.8.1.13 → verified1.8.1.13
You need to log in before you can comment on or make changes to this bug.