Bug 28691

Summary: Do not retain ScopeNodes outside of parsing
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: pfeldman
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
The Patch
oliver: review+
fixed typo barraclough: review+

Description Gavin Barraclough 2009-08-24 16:20:32 PDT
There is now no need for these to exist outside of parsing – their use in the runtime is replaced by Executable types.
Comment 1 Gavin Barraclough 2009-08-24 16:23:06 PDT
Created attachment 38504 [details]
The Patch
Comment 2 Oliver Hunt 2009-08-24 16:28:31 PDT
Comment on attachment 38504 [details]
The Patch

fix the formatting issue i mentioned
Comment 3 Darin Adler 2009-08-24 16:33:29 PDT
Comment on attachment 38504 [details]
The Patch

> +        There is now no need for these to exist outside of parsing â their use in the runtime is replaced by Executable types.

Looks like you had an em dash in some encoding other than UTF-8.

>  ScopeNodeData::ScopeNodeData(ParserArena& arena, SourceElements* children, VarStack* varStack, FunctionStack* funcStack, int numConstants)
> -    : m_numConstants(numConstants)
> +    : m_arena(new ParserArena())

I usually don't use parentheses when there are no arguments to the constructor.

> +    , m_numConstants(numConstants)
>  {
> -    m_arena.swap(arena);
> +    m_arena->swap(arena);

The ParserArena goes out of its way to be "swappable" so you can just store it rather than a pointer to it. If we no longer need that behavior, we could simplify ParserArena and remove a bit of indirection.

> +        static const bool scopeIsFunction = false;

This seems kind of dangerous to me. If this is different in the base class and derived class you could get the wrong answer if you asked the base class.

> +            : m_arena(new ParserArena())

Same thing about parentheses.

> +    StatementVector& children = program->children();
> +    if (children.size() != 1)
> +        return 0;
> +
> +    StatementNode* exprStatement = children[0];

This won't compile any more. You need to merge with my recent changes.

> +    Identifier* parameters = static_cast<Identifier*>(fastMalloc(m_parameterCount * sizeof(Identifier)));
> +    WTF::VectorCopier<false, Identifier>::uninitializedCopy(m_parameters, m_parameters + m_parameterCount, parameters);
> +    return parameters;

You need comments explaining this, but also, it seems unfortunate to use this vector-internal helper here. I'd almost rather just see a loop using placement new instead.
Comment 4 Gavin Barraclough 2009-08-24 19:49:35 PDT
Re the ParseArena changes (pointer to, rather than wholly contained), these were ultimately not necessary as a part of this patch, and caused merge conflicts on update, so I have reverted.  Might still be worth investigating again (along with removing ParserArenaRefCounted), but I will leave this for another patch for now.

> > +        static const bool scopeIsFunction = false;
> 
> This seems kind of dangerous to me. If this is different in the base class and
> derived class you could get the wrong answer if you asked the base class.

To take your concern into account I've moved the false definition from ScopeNode to ProgramNode & EvalNode, so there should be no chance of reading the value from the base by mistake (I'm open to a better solution, if you have any suggestions).

> > +    Identifier* parameters = static_cast<Identifier*>(fastMalloc(m_parameterCount * sizeof(Identifier)));
> > +    WTF::VectorCopier<false, Identifier>::uninitializedCopy(m_parameters, m_parameters + m_parameterCount, parameters);
> > +    return parameters;
> 
> You need comments explaining this, but also, it seems unfortunate to use this
> vector-internal helper here. I'd almost rather just see a loop using placement
> new instead.

I'll comment this as requested - this code is actually just being hoisted up from FunctionBodyNode, and I too don't like it.  This patch also makes what seems like an unnecessary number of copies of the array of parameters, I'd like to fix this by making parameters be a separate ref counted object shared between FunctionExpressions and FunctionBodyNodes, but again I'd like to leave this for another patch.

G.
Comment 5 Gavin Barraclough 2009-08-24 19:54:20 PDT
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/bytecode/EvalCodeCache.h
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.h
Sending        JavaScriptCore/debugger/Debugger.cpp
Sending        JavaScriptCore/debugger/DebuggerCallFrame.cpp
Sending        JavaScriptCore/interpreter/Interpreter.cpp
Sending        JavaScriptCore/jit/JITStubs.cpp
Sending        JavaScriptCore/parser/Nodes.cpp
Sending        JavaScriptCore/parser/Nodes.h
Sending        JavaScriptCore/parser/Parser.cpp
Sending        JavaScriptCore/parser/Parser.h
Sending        JavaScriptCore/runtime/ArrayPrototype.cpp
Sending        JavaScriptCore/runtime/Completion.cpp
Sending        JavaScriptCore/runtime/Executable.cpp
Sending        JavaScriptCore/runtime/Executable.h
Sending        JavaScriptCore/runtime/FunctionConstructor.cpp
Sending        JavaScriptCore/runtime/JSGlobalData.cpp
Sending        JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp
Transmitting file data ...................
Committed revision 47738.
Comment 6 Pavel Feldman 2009-08-25 13:10:33 PDT
This change has introduced regression in web inspector.
1. Open some page
2. Set a breakpoint in code that should execute
3. Make execution hit the breakpoint

Expected: call frame and source line are highlighted
Actual: nothing is highlighted. Continue the execution, open inspector on inspector, observe javascript error.
Comment 7 Robert Agoston 2009-08-31 09:04:45 PDT
Created attachment 38816 [details]
fixed typo
Comment 8 Gavin Barraclough 2009-08-31 12:32:41 PDT
Comment on attachment 38816 [details]
fixed typo

Good catch, thank you!

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/parser/Parser.h
Transmitting file data ..
Committed revision 47902.