Summary: | SquirrelFish: Bogus values enter evaluation when closing over scope with parameter and var with same name | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||
Component: | JavaScriptCore | Assignee: | Maciej Stachowiak <mjs> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Blocker | CC: | ggaren, mjs, zwarich | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
URL: | http://blog.wired.com/games/2008/05/for-wiiware-nin.html | ||||||||||
Attachments: |
|
Description
Oliver Hunt
2008-05-14 05:19:18 PDT
Created attachment 21124 [details]
Testcase
I believe the error is in JSActivation::copyRegisters: int numRegisters = symbolTable().size(); In the case of duplicate entries, the symbol table's size will not equal the number of local registers. I believe the correct behavior is to set numRegisters to CodeBlock::numLocals instead. There are other, more complicated ways to fix this. For example, codegen changes could ensure exact local register allocation, such that we packed all duplicates into the same slot. I had a patch to do that a while back. (In reply to comment #2) > I believe the error is in JSActivation::copyRegisters: > > int numRegisters = symbolTable().size(); > > In the case of duplicate entries, the symbol table's size will not equal the > number of local registers. I believe the correct behavior is to set > numRegisters to CodeBlock::numLocals instead. > > There are other, more complicated ways to fix this. For example, codegen > changes could ensure exact local register allocation, such that we packed all > duplicates into the same slot. I had a patch to do that a while back. I think the solution is simpler than this. We just shouldn't make a local variable for an identifier that also appears as a parameter. > > There are other, more complicated ways to fix this. For example, codegen > > changes could ensure exact local register allocation, such that we packed all > > duplicates into the same slot. I had a patch to do that a while back. > > I think the solution is simpler than this. We just shouldn't make a local > variable for an identifier that also appears as a parameter. That's the more complicated solution I mentioned above: codegen changes to ensure exact register allocation. Seems much easier just to change > > int numRegisters = symbolTable().size(); to > > int numRegisters = codeBlock->numLocals. (In reply to comment #4) > > > There are other, more complicated ways to fix this. For example, codegen > > > changes could ensure exact local register allocation, such that we packed all > > > duplicates into the same slot. I had a patch to do that a while back. > > > > I think the solution is simpler than this. We just shouldn't make a local > > variable for an identifier that also appears as a parameter. > > That's the more complicated solution I mentioned above: codegen changes to > ensure exact register allocation. > > Seems much easier just to change > > > > int numRegisters = symbolTable().size(); > > to > > > > int numRegisters = codeBlock->numLocals. Except that's wrong, because it still has the wrong behaviour with f.arguments. > Except that's wrong, because it still has the wrong behaviour with f.arguments.
Can you elaborate on this?
(In reply to comment #6) > > Except that's wrong, because it still has the wrong behaviour with f.arguments. > > Can you elaborate on this? I thought that something like this would go wrong: function f() { g.arguments[0] = "PASS"; } function g(a) { var a = "FAIL"; f(); print(a); } g("PASS"); However, it seems that it doesn't cause a problem, so my apologies. Anyways, I have a patch that fixes the problem by changing codegen, so I'll post it. Created attachment 21129 [details]
Proposed patch
This algorithm is O(MxN), where M is the number of vars and N is the number of parameters. We try really hard to avoid algorithms like that, even in code that doesn't seem performance-critical, because such algorithms often come back to bite us. I would worry especially about pages generated by obfuscaters, cross-compilers, and/or server-side scripts triggering pathological behavior at compile time. If you want to go with the codegen solution, I think you should use a hashing technique instead. (In reply to comment #9) > This algorithm is O(MxN), where M is the number of vars and N is the number of > parameters. We try really hard to avoid algorithms like that, even in code that > doesn't seem performance-critical, because such algorithms often come back to > bite us. > > I would worry especially about pages generated by obfuscaters, cross-compilers, > and/or server-side scripts triggering pathological behavior at compile time. > > If you want to go with the codegen solution, I think you should use a hashing > technique instead. Yeah, I was worried about that. I guess it makes more sense to with the local storage resizing, at least for now. Is it always going to be correct? I tried using a HashSet<UString::Rep*>, but for some reason it causes an intermittent timeout in/js/gmail-re-re.html. > I guess it makes more sense to with the local
> storage resizing, at least for now. Is it always going to be correct?
Yes, I think so.
JSActivation knows how to talk to a block of registers in order to get/set locals. copyRegisters() just moves the block of registers to a new location in memory: the behavior remains the same.
I realized a similar bug can happen with duplicate parameter names, and only Geoff's suggestion, not Cameron's patch, will fix that case. Created attachment 21199 [details]
geoff's suggested fix plus tests
Comment on attachment 21199 [details]
geoff's suggested fix plus tests
r=me, assuming perf is good.
Landed r33516 |