Summary: | SQUIRRELFISH: incorrect eval used in some cases | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||
Component: | JavaScriptCore | Assignee: | Geoffrey Garen <ggaren> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ggaren, mjs, oliver, zwarich | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
URL: | http://nerget.com/jstests/eval.html | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 18624 | ||||||||||
Attachments: |
|
Description
Oliver Hunt
2008-04-30 19:28:49 PDT
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 |