RESOLVED FIXED 19076
SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars 
https://bugs.webkit.org/show_bug.cgi?id=19076
Summary SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope...
Oliver Hunt
Reported 2008-05-15 05:02:23 PDT
Seen on http://informationliberation.com/ - I cannot workout how to trigger this myself, but the issue is when generating globalcode we use the existence of globals to tell us that we've initialised "this" in the global scope. But if we have re-entered and no variables have been defined yet we re-add this, resulting in us trying to increase the number of globals in the containing scope, leading to badness.
Attachments
a fix (5.88 KB, patch)
2008-05-15 05:19 PDT, Oliver Hunt 		ggaren: review+
DetailsFormatted DiffDiff
A better fix, which despite being technically superior is slower. I hate GCC. (8.94 KB, patch)
2008-05-16 00:04 PDT, Oliver Hunt 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Oliver Hunt
Comment 1 2008-05-15 05:19:12 PDT
Created attachment 21161 [details] a fix This fixes the bug by not using the symboltable for the global this reference, so we don't need to track initialisation as carefully. My system registers it as a 0.1-0.2% regression though :-/
Cameron Zwarich (cpst)
Comment 2 2008-05-15 06:12:00 PDT
Now that Oliver rolled out r33483, this patch is a 0.5% progression for me, bringing us to our best time in quite a while.
Oliver Hunt
Comment 3 2008-05-15 06:13:13 PDT
Comment on attachment 21161 [details] a fix My system also registers this as a progression so r?
Geoffrey Garen
Comment 4 2008-05-15 09:55:11 PDT
Comment on attachment 21161 [details] a fix It's a little squirrely to have a variable named "m_thisRegister" that's only valid if we happen to be compiling program code. I'd recommend setting m_thisRegister when compiling function code and eval code as well. Then, "thisRegister()" can always return m_thisRegister, which makes a lot more sense. Eventually, we'll won't put "this" in the symbol table at all. r=me
Oliver Hunt
Comment 5 2008-05-15 12:46:35 PDT
(In reply to comment #4) > (From update of attachment 21161 [details] [edit]) > It's a little squirrely to have a variable named "m_thisRegister" that's only > valid if we happen to be compiling program code. > > I'd recommend setting m_thisRegister when compiling function code and eval code > as well. Then, "thisRegister()" can always return m_thisRegister, which makes a > lot more sense. > > Eventually, we'll won't put "this" in the symbol table at all. > > r=me > I agree, but that cost of retrieving the this register for function/evalnode codegenerator was a regression -- but that was before we rolled out the patch o' badness
Oliver Hunt
Comment 6 2008-05-16 00:04:22 PDT
Created attachment 21196 [details] A better fix, which despite being technically superior is slower. I hate GCC.
Oliver Hunt
Comment 7 2008-05-16 03:34:44 PDT
Test case: <body onload="document.write('<script' + '>' + 'var x = 0; var y = 1; var z = 2;' + '</script' + '>');"> </body>
Oliver Hunt
Comment 8 2008-05-16 14:53:11 PDT
I have a fix which ggaren is okay with, but it's causing a webkit regression, i'm trying to work out how.
Oliver Hunt
Comment 9 2008-05-16 17:28:41 PDT
M JavaScriptCore/ChangeLog M JavaScriptCore/VM/CodeGenerator.cpp M JavaScriptCore/VM/CodeGenerator.h M JavaScriptCore/VM/Machine.cpp M JavaScriptCore/kjs/ExecState.h M JavaScriptCore/kjs/JSGlobalObject.cpp M JavaScriptCore/kjs/JSGlobalObject.h M JavaScriptCore/kjs/nodes.cpp M LayoutTests/ChangeLog A LayoutTests/fast/js/direct-entry-to-function-code-expected.txt A LayoutTests/fast/js/direct-entry-to-function-code.html Committed r33541
Note You need to log in before you can comment on or make changes to this bug.