Bug 28691 – Do not retain ScopeNodes outside of parsing

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

[Gavin Barraclough]

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

[Gavin Barraclough]

Created attachment 38504 [details]
The Patch

[Oliver Hunt]

Comment on attachment 38504 [details]
The Patch

fix the formatting issue i mentioned

[Darin Adler]

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.

[Gavin Barraclough]

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.

[Gavin Barraclough]

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.

[Pavel Feldman]

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.

[Robert Agoston]

Created attachment 38816 [details]
fixed typo

[Gavin Barraclough]

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.