Bug 46330 - Only copy captured variables into activation
Summary: Only copy captured variables into activation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-22 18:28 PDT by Oliver Hunt
Modified: 2010-09-27 13:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.19 KB, patch)
2010-09-22 18:32 PDT, Oliver Hunt
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2010-09-22 18:28:08 PDT
Only copy captured variables into activation
Comment 1 Oliver Hunt 2010-09-22 18:32:00 PDT
Created attachment 68489 [details]
Patch
Comment 2 WebKit Review Bot 2010-09-23 00:48:06 PDT
Attachment 68489 [details] did not build on win:
Build output: http://queues.webkit.org/results/4041105
Comment 3 Oliver Hunt 2010-09-23 10:41:42 PDT
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.
Comment 4 Oliver Hunt 2010-09-23 11:50:50 PDT
Committed r68171
Comment 5 WebKit Review Bot 2010-09-23 15:32:58 PDT
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
Comment 6 Jenn Braithwaite 2010-09-24 16:13:03 PDT
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
Comment 7 Oliver Hunt 2010-09-24 16:25:28 PDT
(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?
Comment 8 Jenn Braithwaite 2010-09-27 11:14:05 PDT
(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.
Comment 9 Oliver Hunt 2010-09-27 11:17:20 PDT
(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
Comment 10 Jenn Braithwaite 2010-09-27 13:08:00 PDT
(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.