Bug 28209 – Restrict use of FuncDeclNode & FuncExprNode to the parser.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

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.

[Gavin Barraclough]

Created attachment 34633 [details]
The Patch.

[Gavin Barraclough]

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.

[Kenneth Rohde Christiansen]

Hi there,

You commit https://trac.webkit.org/changeset/47089
JavaScriptCore: Restrict use of FuncDeclNode & FuncExprNode to the parser.
https://bugs.webkit.org/show_bug.cgi?id=28209

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

The tests breaking are:

fast/js/const.html expected
fast/js/exception-thrown-from-function-with-lazy-activation.html
fast/js/function-call-register-allocation.html
fast/js/function-dot-arguments-and-caller.html 
fast/js/named-function-expression.html
fast/js/static-scope-object.html
plugins/netscape-invoke-default.html 

Cheers,
Kenneth

[Kenneth Rohde Christiansen]

Here you have the diffs of the failing tests

fast/js/const.html

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

fast/js/exception-thrown-from-function-with-lazy-activation.html

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

fast/js/function-call-register-allocation.html

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

fast/js/function-dot-arguments-and-caller.html 

-PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS,PASS

fast/js/named-function-expression.html

-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

fast/js/static-scope-object.html

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

plugins/netscape-invoke-default.html 

 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.
-SUCCESS
+FAILURE

[Gavin Barraclough]

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.

[Eric Seidel (no email)]

I can't tell if this is still pending commit.  But I think it's not supposed to be, or?

[Gavin Barraclough]

I'm not sure I understand your question. :-)

This patch is committed, currently pending decommit.

[Eric Seidel (no email)]

Comment on attachment 34633 [details]
The Patch.

Ok. Removing the r+ to remove it from the to-be-committed list.

[Gavin Barraclough]

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).

cheers,
G.

[Eric Seidel (no email)]

The query that Maciej sent to the list months ago for listing which bugs need commit:
https://bugs.webkit.org/buglist.cgi?query_format=advanced&short_desc_type=notregexp&short_desc=%5C%5BS60%5C%5D&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B&field0-1-0=noop&type0-1-0=equals&value0-1-0=

Show's bugs if they have r+.

Nah, the commit-queue is smart enough to only look for bugs which actually have a commit-queue+.  It's just we humans who are confused by the r+ on an open bug, and wonder if said open bug should be considered for landing.

[Gavin Barraclough]

Ah, I see.  Fair enough. :-)

[Gavin Barraclough]

Sorry about that, should be fixed now in r47183.