Bug 18822 - SQUIRRELFISH: incorrect eval used in some cases
Summary: SQUIRRELFISH: incorrect eval used in some cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Geoffrey Garen
URL: http://nerget.com/jstests/eval.html
Keywords:
Depends on:
Blocks: 18624
  Show dependency treegraph
 
Reported: 2008-04-30 19:28 PDT by Oliver Hunt
Modified: 2008-05-02 19:32 PDT (History)
4 users (show)

See Also:


Attachments
Test case (669 bytes, text/html)
2008-04-30 19:47 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (1.56 KB, patch)
2008-04-30 23:19 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch (5.43 KB, patch)
2008-05-02 17:55 PDT, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-04-30 19:28:49 PDT
In ToT SquirrelFish one of the eval tests in the linked testcase fails :-(
Comment 1 Cameron Zwarich (cpst) 2008-04-30 19:47:58 PDT
Created attachment 20906 [details]
Test case
Comment 2 Cameron Zwarich (cpst) 2008-04-30 23:07:24 PDT
This bug occurs because we don't create as many ExecStates as we used to. In particular, the scope chain of the current ExecState in Machine::privateExecute may not be the current scope chain. However, all ExecState::lexicalGlobalObject() does is get the top of its ScopeChain, so we should just replace the use of ExecState::lexicalGlobalObject() with scopeChain->bottom() in op_call_eval.

This also means that other uses of ExecState::lexicalGlobalObject() are wrong for cross-window function calls. Most of them are used for getting prototypes, but there are others, e.g. setting up the scope chain in FunctionObjectImp::construct(). We can't fix all of the others yet, because we pass around an ExecState rather than a ScopeChainNode*.
Comment 3 Cameron Zwarich (cpst) 2008-04-30 23:19:12 PDT
Created attachment 20908 [details]
Proposed patch

Here's a patch that fixes the bug.
Comment 4 Cameron Zwarich (cpst) 2008-05-01 00:02:12 PDT
Comment on attachment 20908 [details]
Proposed patch

This patch is a 0.2% or 0.3% performance regression, and if I move things to a separate function it is much worse.
Comment 5 Geoffrey Garen 2008-05-01 08:31:41 PDT
I'd recommend holding off on this one for now. The real bug here is that Exec::lexicalGlobalObject returns an incorrect value. I'm working on that.
Comment 6 Cameron Zwarich (cpst) 2008-05-01 08:53:03 PDT
(In reply to comment #5)
> I'd recommend holding off on this one for now. The real bug here is that
> Exec::lexicalGlobalObject returns an incorrect value. I'm working on that.

Nice. I'll turn Oliver's tests into layout tests for when you get it working.
Comment 7 Geoffrey Garen 2008-05-02 17:55:00 PDT
I looked at this further, and Cameron was right. (Sorry!) Fixing lexical global object access inside the machine is a separate task from fixing it elsewhere.
Comment 8 Geoffrey Garen 2008-05-02 17:55:39 PDT
Created attachment 20938 [details]
Patch

Fixed lexical global object access inside the machine.
Comment 9 Cameron Zwarich (cpst) 2008-05-02 18:01:03 PDT
(In reply to comment #8)
> Created an attachment (id=20938) [edit]
> Patch
> 
> Fixed lexical global object access inside the machine.

+    inline JSGlobalObject* ScopeChainNode::globalObject() const
+    {
+        JSGlobalObject* globalObject = static_cast<JSGlobalObject*>(bottom());
+        ASSERT(globalObject->isGlobalObject());
+        return globalObject;
+    }

Shouldn't the cast be after the ASSERT?
Comment 10 Geoffrey Garen 2008-05-02 18:29:30 PDT
> Shouldn't the cast be after the ASSERT?

I find that

JSGlobalObject* globalObject = static_cast<JSGlobalObject*>(bottom());
ASSERT(globalObject->isGlobalObject());

reads better than

JSObject* o = bottom();
ASSERT(o->isGlobalObject());
JSGlobalObject* globalObject = static_cast<JSGlobalObject*>(o);

and I don't like 

ASSERT(bottom()->isGlobalObject());
JSGlobalObject* globalObject = static_cast<JSGlobalObject*>(bottom());

because it does a double call in debug builds. (Makes debug builds needlessly slow. Hard to step through.)

Is there an advantage to doing the ASSERT before the cast?
Comment 11 Cameron Zwarich (cpst) 2008-05-02 18:45:56 PDT
(In reply to comment #10)
> Is there an advantage to doing the ASSERT before the cast?

I just asked because it seems to be the prevailing style, and the method you are replacing in ExecState does that. Nothing other than a style question, really.
Comment 12 Oliver Hunt 2008-05-02 18:57:09 PDT
Comment on attachment 20938 [details]
Patch

r=me, although i'd like there to be a layout test
Comment 13 Geoffrey Garen 2008-05-02 19:27:27 PDT
Oliver agreed on IRC that LayoutTests/fast/js/eval-cross-window.html is sufficient.
Comment 14 Geoffrey Garen 2008-05-02 19:32:18 PDT
r 32831