Bug 46330 – Only copy captured variables into activation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Only copy captured variables into activation

[Oliver Hunt]

Created attachment 68489 [details]
Patch

[WebKit Review Bot]

Attachment 68489 [details] did not build on win:
Build output: http://queues.webkit.org/results/4041105

[Oliver Hunt]

Comment on attachment 68489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68489&action=review

Review comments by Geoff Garen.

> JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:371
> +        if (nonCapturedVarPass)
> +            codeBlock->m_numCapturedVars = codeBlock->m_numVars;
> +        for (size_t i = 0; i < functionStack.size(); ++i) {
> +            FunctionBodyNode* function = functionStack[i];
> +            const Identifier& ident = function->ident();
> +            if (functionBody->captures(ident) != nonCapturedVarPass) {
> +                m_functions.add(ident.impl());
> +                emitNewFunction(addVar(ident, false), function);
> +            }
> +        }
> +        for (size_t i = 0; i < varStack.size(); ++i) {
> +            const Identifier& ident = *varStack[i].first;
> +            if (functionBody->captures(ident) != nonCapturedVarPass)
> +                addVar(ident, varStack[i].second & DeclarationStacks::IsConstant);
> +        }
> +        if (!nonCapturedVarPass)
> +            codeBlock->m_numCapturedVars = codeBlock->m_numVars;
>      }

The meaning of 'nonCapturedVarPass' in this loop is just too subtle. I think it would be better to unroll this loop instead.

> JavaScriptCore/runtime/JSActivation.cpp:96
> +inline bool JSActivation::symbolTableGet(const Identifier& propertyName, PropertySlot& slot)
> +{
> +    SymbolTableEntry entry = symbolTable().inlineGet(propertyName.impl());
> +    // Cant't access variables that weren't captured
> +    if (!entry.isNull() && entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())) {
> +        slot.setRegisterSlot(&registerAt(entry.getIndex()));
> +        return true;
> +    }
> +    return false;
> +}
> +
> +inline bool JSActivation::symbolTablePut(const Identifier& propertyName, JSValue value)
> +{
> +    ASSERT(!Heap::heap(value) || Heap::heap(value) == Heap::heap(this));
> +    
> +    SymbolTableEntry entry = symbolTable().inlineGet(propertyName.impl());
> +    if (entry.isNull())
> +        return false;
> +    if (entry.isReadOnly())
> +        return true;
> +    // Can't access variables that weren't captured
> +    if (entry.getIndex() >= static_cast<int>(d()->functionExecutable->capturedVariableCount()))
> +        return false;
> +    registerAt(entry.getIndex()) = value;
> +    return true;
> +}

As discussed, it's not possible to get or put a non-captured in an activation. You can change the checks to ASSERTs.

> JavaScriptCore/runtime/JSActivation.cpp:122
> +        return false;

Ditto.

[Oliver Hunt]

Committed r68171

[WebKit Review Bot]

http://trac.webkit.org/changeset/68171 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/68171
http://trac.webkit.org/changeset/68172

[Jenn Braithwaite]

This change may have broken these layout tests. They all fail the newly added assert:

ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())
(/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&))

fast/xmlhttprequest/xmlhttprequest-no-file-access.html
ietestcenter/Javascript/12.14-3.html
ietestcenter/Javascript/12.14-4.html
ietestcenter/Javascript/12.14-5.html
ietestcenter/Javascript/12.14-6.html
ietestcenter/Javascript/12.14-7.html
ietestcenter/Javascript/12.14-8.html

[Oliver Hunt]

(In reply to comment #6)
> This change may have broken these layout tests. They all fail the newly added assert:
> 
> ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())
> (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&))
> 
> fast/xmlhttprequest/xmlhttprequest-no-file-access.html
> ietestcenter/Javascript/12.14-3.html
> ietestcenter/Javascript/12.14-4.html
> ietestcenter/Javascript/12.14-5.html
> ietestcenter/Javascript/12.14-6.html
> ietestcenter/Javascript/12.14-7.html
> ietestcenter/Javascript/12.14-8.html

what revision?

[Jenn Braithwaite]

(In reply to comment #7)
> (In reply to comment #6)
> > This change may have broken these layout tests. They all fail the newly added assert:
> > 
> > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())
> > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&))
> > 
> > fast/xmlhttprequest/xmlhttprequest-no-file-access.html
> > ietestcenter/Javascript/12.14-3.html
> > ietestcenter/Javascript/12.14-4.html
> > ietestcenter/Javascript/12.14-5.html
> > ietestcenter/Javascript/12.14-6.html
> > ietestcenter/Javascript/12.14-7.html
> > ietestcenter/Javascript/12.14-8.html
> 
> what revision?

I was on r68285. Snow Leopard.

[Oliver Hunt]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This change may have broken these layout tests. They all fail the newly added assert:
> > > 
> > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())
> > > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&))
> > > 
> > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html
> > > ietestcenter/Javascript/12.14-3.html
> > > ietestcenter/Javascript/12.14-4.html
> > > ietestcenter/Javascript/12.14-5.html
> > > ietestcenter/Javascript/12.14-6.html
> > > ietestcenter/Javascript/12.14-7.html
> > > ietestcenter/Javascript/12.14-8.html
> > 
> > what revision?
> 
> I was on r68285. Snow Leopard.

Check with a revision >= 68288

[Jenn Braithwaite]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > This change may have broken these layout tests. They all fail the newly added assert:
> > > > 
> > > > ASSERTION FAILED: entry.getIndex() < static_cast<int>(d()->functionExecutable->capturedVariableCount())
> > > > (/work/WebKit/JavaScriptCore/runtime/JSActivation.cpp:75 bool JSC::JSActivation::symbolTableGet(const JSC::Identifier&, JSC::PropertySlot&))
> > > > 
> > > > fast/xmlhttprequest/xmlhttprequest-no-file-access.html
> > > > ietestcenter/Javascript/12.14-3.html
> > > > ietestcenter/Javascript/12.14-4.html
> > > > ietestcenter/Javascript/12.14-5.html
> > > > ietestcenter/Javascript/12.14-6.html
> > > > ietestcenter/Javascript/12.14-7.html
> > > > ietestcenter/Javascript/12.14-8.html
> > > 
> > > what revision?
> > 
> > I was on r68285. Snow Leopard.
> 
> Check with a revision >= 68288

All happy at r68401.