Bug 18822 – SQUIRRELFISH: incorrect eval used in some cases

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

In ToT SquirrelFish one of the eval tests in the linked testcase fails :-(

[Cameron Zwarich (cpst)]

Created attachment 20906 [details]
Test case

[Cameron Zwarich (cpst)]

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*.

[Cameron Zwarich (cpst)]

Created attachment 20908 [details]
Proposed patch

Here's a patch that fixes the bug.

[Cameron Zwarich (cpst)]

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.

[Geoffrey Garen]

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.

[Cameron Zwarich (cpst)]

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

[Geoffrey Garen]

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.

[Geoffrey Garen]

Created attachment 20938 [details]
Patch

Fixed lexical global object access inside the machine.

[Cameron Zwarich (cpst)]

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

[Geoffrey Garen]

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

[Cameron Zwarich (cpst)]

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

[Oliver Hunt]

Comment on attachment 20938 [details]
Patch

r=me, although i'd like there to be a layout test

[Geoffrey Garen]

Oliver agreed on IRC that LayoutTests/fast/js/eval-cross-window.html is sufficient.

[Geoffrey Garen]

r 32831