Bug 19076

Summary:

SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars

Product:

WebKit

Reporter:

Oliver Hunt <oliver>

Component:

JavaScriptCore

Assignee:

Oliver Hunt <oliver>

Status:

RESOLVED FIXED

Severity:

Blocker

CC:

ggaren, mjs, zwarich

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
a fix	ggaren: review+
A better fix, which despite being technically superior is slower. I hate GCC.	none

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+	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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.