REGRESSION: Use of undefined CallFrame::ScopeChain value

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

The CallFrame ScopeChain slot is still being read and written, even though the JSScope* value is now being stored in an allocated register.  When the DFG inlines functions and then one of those inlined functions OSR exits, the ScopeChain slots for the callee's that were inlined will contain whatever values are in memory.  The memory for the ScopeChain slots will contain values from the registers allocated by the DFG for the main and inlined functions.

The problem is that there is some other residual code elsewhere that assumes that the scope slot of the header contains valid and appropriate JSScope* values.  Currently the main code paths that read and write the ScopeChain slot in the CallFrame header and the code paths for making calls.  They are still copying from the caller’s scope slot to the callee’s scope slot.  The DFG doesn’t know anything special about the scope slots.

The fix is to eliminate all the vestige code that reads / writes the old scope slot in the header and then get rid of the space in the header.

<rdar://problem/19194440>

[Michael Saboff]

Created attachment 243109 [details]
Patch

[Mark Lam]

Comment on attachment 243109 [details]
Patch

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

r=me with suggested cleanup in ChangeLog and CallFrame.h.

> Source/JavaScriptCore/ChangeLog:13
> +        Removed CallFrame::scope() and CallFrame::setScope() and eliminated or changed
> +        all usages of these funcitons.  In some cases the scope is passed in or determined
> +        another way.  In some cases the scope is used to calculate other values.  Lastly
> +        were places where these functions where used that are no longer needed.  For
> +        example when making a call, the caller's ScopeChain was copied to the callee's
> +        ScopeChain.

I think it’s implicitly stated here, but can you please say it explicitly that though you are removing the CallFrame scope getter/setter functions:
1. the ScopeChain slot still exists in the CallFrame
2. it just won’t be used anymore
3. it’ll be removed later

Did I understand this correctly?

> Source/JavaScriptCore/interpreter/CallFrame.h:191
> +        ALWAYS_INLINE void init(CodeBlock* codeBlock, Instruction* vPC, JSScope*,

Why not remove the JSScope* arg altogether?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:905
> +    callOperation(operationCreateActivation, regT0, 0);

It seems wrong to me that we always pass a literal constant 0 for the offset of activation register.  Either:
1. it is contractually fixed to be constant 0 and we don’t need to pass it, or
2. we should “compute” the value.

But I understand that this is pre-existing code, and can be cleaned up in a separate patch.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:676
> +    callOperation(operationCreateActivation, regT0, 0);

Ditto.

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 243109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243109&action=review
> 
> r=me with suggested cleanup in ChangeLog and CallFrame.h.
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        Removed CallFrame::scope() and CallFrame::setScope() and eliminated or changed
> > +        all usages of these funcitons.  In some cases the scope is passed in or determined
> > +        another way.  In some cases the scope is used to calculate other values.  Lastly
> > +        were places where these functions where used that are no longer needed.  For
> > +        example when making a call, the caller's ScopeChain was copied to the callee's
> > +        ScopeChain.
> 
> I think it’s implicitly stated here, but can you please say it explicitly
> that though you are removing the CallFrame scope getter/setter functions:
> 1. the ScopeChain slot still exists in the CallFrame
> 2. it just won’t be used anymore
> 3. it’ll be removed later
> 
> Did I understand this correctly?

You understand correctly and I'll add appropriate comments to the ChangeLog.

> > Source/JavaScriptCore/interpreter/CallFrame.h:191
> > +        ALWAYS_INLINE void init(CodeBlock* codeBlock, Instruction* vPC, JSScope*,
> 
> Why not remove the JSScope* arg altogether?

Will do.

> > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:905
> > +    callOperation(operationCreateActivation, regT0, 0);
> 
> It seems wrong to me that we always pass a literal constant 0 for the offset
> of activation register.  Either:
> 1. it is contractually fixed to be constant 0 and we don’t need to pass it,
> or
> 2. we should “compute” the value.
> 
> But I understand that this is pre-existing code, and can be cleaned up in a
> separate patch.
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:676
> > +    callOperation(operationCreateActivation, regT0, 0);
> 
> Ditto.

The literal 0 in these two locations are existing.  The DFG computes a value, but the baseline always uses 0.  This appears to be by design.

[Michael Saboff]

(In reply to comment #3)

> > > Source/JavaScriptCore/interpreter/CallFrame.h:191
> > > +        ALWAYS_INLINE void init(CodeBlock* codeBlock, Instruction* vPC, JSScope*,
> > 
> > Why not remove the JSScope* arg altogether?

init() was dead code, so I deleted the whole function.

[Michael Saboff]

Committed r177146: <http://trac.webkit.org/changeset/177146>

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> Committed r177146: <http://trac.webkit.org/changeset/177146>

This has broken the GTK and EFL builds: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/53500/steps/compile-webkit/logs/stdio

[Mark Lam]

(In reply to comment #4)
> init() was dead code, so I deleted the whole function.

init() is still in use buy JSGlobalObject.cpp to create the globalExec.  This broke the build.  I restored CallFrame::init() minus the JSScope* arg, and also removed the arg from the caller.

Build fix landed in r177149: <http://trac.webkit.org/r177149>.

[Carlos Alberto Lopez Perez]

(In reply to comment #7)
> (In reply to comment #4)
> > init() was dead code, so I deleted the whole function.
> 
> init() is still in use buy JSGlobalObject.cpp to create the globalExec. 
> This broke the build.  I restored CallFrame::init() minus the JSScope* arg,
> and also removed the arg from the caller.
> 
> Build fix landed in r177149: <http://trac.webkit.org/r177149>.

Worked :) Thanks!

[Michael Saboff]

(In reply to comment #7)
> (In reply to comment #4)
> > init() was dead code, so I deleted the whole function.
> 
> init() is still in use buy JSGlobalObject.cpp to create the globalExec. 
> This broke the build.  I restored CallFrame::init() minus the JSScope* arg,
> and also removed the arg from the caller.
> 
> Build fix landed in r177149: <http://trac.webkit.org/r177149>.

Thanks.  My bad.

[Geoffrey Garen]

This patch really should not have been r+ed without a regression test. Michael, please write a regression test.

[Mark Lam]

(In reply to comment #10)
> This patch really should not have been r+ed without a regression test.
> Michael, please write a regression test.

To follow up, Michael has added the regression test in http://trac.webkit.org/changeset/177203.