Bug 28209 - Restrict use of FuncDeclNode & FuncExprNode to the parser.
Summary: Restrict use of FuncDeclNode & FuncExprNode to the parser.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2009-08-11 19:43 PDT by Gavin Barraclough
Modified: 2009-08-12 22:28 PDT (History)
2 users (show)

See Also:

The Patch. (28.74 KB, patch)
2009-08-11 19:50 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2009-08-11 19:43:30 PDT
These objects were also being referenced from the CodeBlock.  By changing this to just retain pointers to FunctionBodyNodes these classes can be restricted to use during parsing.
Comment 1 Gavin Barraclough 2009-08-11 19:50:54 PDT
Created attachment 34633 [details]
The Patch.
Comment 2 Gavin Barraclough 2009-08-11 22:34:44 PDT
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.exp
Sending        JavaScriptCore/bytecode/CodeBlock.cpp
Sending        JavaScriptCore/bytecode/CodeBlock.h
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.h
Sending        JavaScriptCore/interpreter/Interpreter.cpp
Sending        JavaScriptCore/jit/JITOpcodes.cpp
Sending        JavaScriptCore/jit/JITStubs.cpp
Sending        JavaScriptCore/jit/JITStubs.h
Sending        JavaScriptCore/parser/Grammar.y
Sending        JavaScriptCore/parser/NodeConstructors.h
Sending        JavaScriptCore/parser/Nodes.cpp
Sending        JavaScriptCore/parser/Nodes.h
Sending        JavaScriptCore/runtime/JSFunction.h
Sending        WebCore/ChangeLog
Sending        WebCore/inspector/JavaScriptDebugServer.cpp
Sending        WebKitTools/Scripts/run-javascriptcore-tests
Transmitting file data ..................
Committed revision 47089.

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def
Sending        JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore_debug.def
Transmitting file data ...
Committed revision 47090.
Comment 3 Kenneth Rohde Christiansen 2009-08-12 08:20:45 PDT
Hi there,

You commit https://trac.webkit.org/changeset/47089
JavaScriptCore: Restrict use of FuncDeclNode & FuncExprNode to the parser.

seems to break the fast/js tests for Qt 64bit linux.

The tests breaking are:

fast/js/const.html expected

Comment 4 Kenneth Rohde Christiansen 2009-08-12 10:18:32 PDT
Here you have the diffs of the failing tests


-PASS f() is f
+FAIL f() should be function g() { g="FAIL"; return g; } (of type function). Was FAIL (of type string).


-PASS: args[0] should be 1 and is.
+FAIL: args[0] should be 1 but instead is undefined.


-PASS: Recursion did not run out of stack space.
+FAIL: Recursion threw an exception: ReferenceError: Can't find variable: f




-PASS var z = 10; var x = eval('(function Named(a,b){ return (!!Named) ? (a + b + z) : -999; })'); x(4,5) is 19
+FAIL var z = 10; var x = eval('(function Named(a,b){ return (!!Named) ? (a + b + z) : -999; })'); x(4,5) should 
-PASS var ctr = 3; var x = (function Named(a,b){ if(--ctr) return 2 * Named(a,b); else return a + b; }); x(5,6) is 44
+FAIL var ctr = 3; var x = (function Named(a,b){ if(--ctr) return 2 * Named(a,b); else return a + b; }); x(5,6) should be 44. Threw exception ReferenceError: Can't find variable: Named


-PASS namedFunctionExpression() is globalObject
+FAIL namedFunctionExpression() should be the global object. Threw exception ReferenceError: Can't find variable: f


 This tests that it's possible to call NPN_InvokeDefault on a JavaScript object. If this test succeeded, the text "SUCCESS" should be shown below.
Comment 5 Gavin Barraclough 2009-08-12 10:47:50 PDT
Gah, sorry, it looks like I've broken the interpreter, can repro on Mac with the JIT disabled.

Will investigate, & will roll the change out if I can't fix this quickly.
Comment 6 Eric Seidel (no email) 2009-08-12 15:33:06 PDT
I can't tell if this is still pending commit.  But I think it's not supposed to be, or?
Comment 7 Gavin Barraclough 2009-08-12 15:35:54 PDT
I'm not sure I understand your question. :-)

This patch is committed, currently pending decommit.
Comment 8 Eric Seidel (no email) 2009-08-12 15:37:21 PDT
Comment on attachment 34633 [details]
The Patch.

Ok. Removing the r+ to remove it from the to-be-committed list.
Comment 9 Gavin Barraclough 2009-08-12 15:42:31 PDT
Hmmm, is removing the r+ flag a good idea? :-/

Are we not losing the information from bugzilla as to who reviewed the patch, and which patches have been approved?

What problem is this solving, does this related to the commit queue?  If so, it seems the commit queue shouldn't care about this bug, since it was not added to the queue.  If there is a problem, are we solving it in the right way here? - does the review queue mechanism need to be smarter?

(Apologies if I'm just misunderstanding something here).

Comment 11 Gavin Barraclough 2009-08-12 15:58:18 PDT
Ah, I see.  Fair enough. :-)
Comment 12 Gavin Barraclough 2009-08-12 22:28:55 PDT
Sorry about that, should be fixed now in r47183.