In ToT SquirrelFish one of the eval tests in the linked testcase fails :-(
Created attachment 20906 [details] Test case
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*.
Created attachment 20908 [details] Proposed patch Here's a patch that fixes the bug.
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.
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.
(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.
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.
Created attachment 20938 [details] Patch Fixed lexical global object access inside the machine.
(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?
> 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?
(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 on attachment 20938 [details] Patch r=me, although i'd like there to be a layout test
Oliver agreed on IRC that LayoutTests/fast/js/eval-cross-window.html is sufficient.
r 32831