Bug 134420 – [ftlopt] DebuggerCallFrame::scope() should return a DebuggerScope

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Previously, DebuggerCallFrame::scope() returns a JSActivation (and relevant peers) which the WebInspector will use to introspect CallFrame variables.  Instead, we should be returning a DebuggerScope as an abstraction layer that provides the introspection functionality that the WebInspector needs.  This is the first step towards not forcing every frame to have a JSActivation object just because the debugger is enabled.

[Mark Lam]

Created attachment 234038 [details]
the patch.

This patch has passed the layout tests and a smoke test setting and stepping thru some breakpoints in the WebInspector.

[Geoffrey Garen]

Comment on attachment 234038 [details]
the patch.

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

> Source/JavaScriptCore/debugger/Debugger.cpp:183
> +    globalObject->ensureDebuggerScopeStructureIsInitialized();

I don't think that saving 32 bytes is worth this complexity (unless you measured otherwise).

> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:88
> +    Strong<DebuggerScope> m_scope;

What prevents this from being a reference cycle? Any time you add a Strong<> as a data member, you should include a comment explaining what prevents it from being a reference cycle.

I suspect that this is a reference cycle. One way to solve that problem is to use a Weak<> instead.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:51
> +    if (!scope) {

This isn't right. Every function has a scope, so a null check is not the right way to find out if the function has allocated an activation object.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:138
> +void DebuggerScope::invalidateChain()

Why do we need to do this? I don't think we should do this.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:171
> +JSObject* DebuggerScope::objectAtScope() const
> +{
> +    ASSERT(isValid());
> +    return JSScope::objectAtScope(m_scope.get());
>  }

I think this helper function just obfuscates things. It would be better to write this one line of code at the call site.

> Source/JavaScriptCore/debugger/DebuggerScope.h:95
> +    JSScope* toJSScope() { return m_scope.get(); }

We use the "to" prefix to indicate a conversion -- usually, a type case. In this case, you're just returning a data member, so you should use normal accessor naming: The data member should be named "m_jsScope" and the accessor should be named "jsScope()".

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 234038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234038&action=review
> 
> > Source/JavaScriptCore/debugger/Debugger.cpp:183
> > +    globalObject->ensureDebuggerScopeStructureIsInitialized();
> 
> I don't think that saving 32 bytes is worth this complexity (unless you measured otherwise).

120 bytes (for sizeof Structure) + time to initialize the structure per global.  But I agree that it's probably not worth it.  I'll fix.

> > Source/JavaScriptCore/debugger/DebuggerCallFrame.h:88
> > +    Strong<DebuggerScope> m_scope;
> 
> What prevents this from being a reference cycle? Any time you add a Strong<> as a data member, you should include a comment explaining what prevents it from being a reference cycle.
> 
> I suspect that this is a reference cycle. One way to solve that problem is to use a Weak<> instead.

There's no reference cycle here.  Here's why:

The DebuggerCallFrame is designed to be "valid" only during a debugging session (while the debugger is broken) through the use of a DebuggerCallFrameScope in Debugger::pauseIfNeeded().  Once the debugger resumes from the break, the DebuggerCallFrameScope destructs, and the DebuggerCallFrame will be invalidated.  We can't guarantee (from this code alone) that the Inspector code isn't still holding a ref to the DebuggerCallFrame (though they shouldn't), but by contract, the frame will be invalidated, and any attempt to query it will return null values.

Now, we're adding the DebuggerScope into the picture.  While a single debugger pause session is in progress, the Inspector may request the scope from the DebuggerCallFrame.  While the DebuggerCallFrame is still valid, we want DebuggerCallFrame::scope() to always return the same DebuggerScope object.  This is why we hold on to it with a strong ref.

If we use a weak ref instead, the following cooky behavior can manifest:
1. The Inspector calls Debugger::scope() to get the top scope.
2. The Inspector iterates down the scope chain and is now only holding a reference to a parent scope.  It is no longer referencing the top scope.
3. A GC occurs, and the DebuggerCallFrame's weak m_scope ref to the top scope gets cleared.
4. The Inspector calls DebuggerCallFrame::scope() to get the top scope again but gets a different DebuggerScope instance.
5. The Inspector iterates down the scope chain but never sees the parent scope instance that retained a ref to in step 2 above.  This is because when iterating this new DebuggerScope instance (which has no knowledge of the previous parent DebuggerScope instance), a new DebuggerScope instance will get created for the same parent scope. 

Since the DebuggerScope is a JSObject, it's liveness is determined by its reachability.  However, it's "validity" is determined by the life-cycle of its owner DebuggerCallFrame.  When the owner DebuggerCallFrame gets invalidated, its debugger scope chain (if instantiated) will also get invalidated.  This is why we need the DebuggerScope::invalidateChain() method.  The Inspector should not be using the DebuggerScope instance after its owner DebuggerCallFrame is invalidated.

Previously, I only added assertions of validity in the DebuggerScope methods.  I've now also added explicit checks to do nothing if the scope has been invalidated.  I've also added these comments to the ChangeLog.

> > Source/JavaScriptCore/debugger/DebuggerScope.cpp:51
> > +    if (!scope) {
> 
> This isn't right. Every function has a scope, so a null check is not the right way to find out if the function has allocated an activation object.

The scope in this case is an optional value passed in from DebuggerScope::create().  I'm using it to mean 2 things here:
1. If unspecified (i.e. null), we'll initialize the DebuggerScope instance with the scope of the DebuggerCallFrame's underlying CallFrame.
2. If specified (i.e. not null), we'll initialize the DebuggerScope instance with the specified scope.  We need this when we are creating DebuggerScope instances for "parent" scopes in the scope chain of the same DebuggerCallFrame.  We instantiate these in DebuggerScope::next().

I'll add this detail to the ChangeLog since it looks like it's not obvious from the code.

> > Source/JavaScriptCore/debugger/DebuggerScope.cpp:138
> > +void DebuggerScope::invalidateChain()
> 
> Why do we need to do this? I don't think we should do this.

As stated above, this is to keep the DebuggerScope's validity state consistent with its owner DebuggerCallFrame.  It's also part of its life cycle management so that we can release DebuggerScope instances as soon as they become invalid and unreachable.

> > Source/JavaScriptCore/debugger/DebuggerScope.cpp:171
> > +JSObject* DebuggerScope::objectAtScope() const
> > +{
> > +    ASSERT(isValid());
> > +    return JSScope::objectAtScope(m_scope.get());
> >  }
> 
> I think this helper function just obfuscates things. It would be better to write this one line of code at the call site.

Fixed.

> > Source/JavaScriptCore/debugger/DebuggerScope.h:95
> > +    JSScope* toJSScope() { return m_scope.get(); }
> 
> We use the "to" prefix to indicate a conversion -- usually, a type case. In this case, you're just returning a data member, so you should use normal accessor naming: The data member should be named "m_jsScope" and the accessor should be named "jsScope()".

Fixed.

New patch coming soon.

[Mark Lam]

Created attachment 234198 [details]
patch 2: revised.

[Geoffrey Garen]

Comment on attachment 234198 [details]
patch 2: revised.

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

> Source/JavaScriptCore/debugger/DebuggerScope.h:99
> +    DebuggerCallFrame* m_debuggerCallFrame;

We don't want an m_debuggerCallFrame data member. It doesn't make sense for our caller to point to our callFrame. Also, we only use m_debuggerCallFrame to allocate an activation, which our caller can do for us.

[Geoffrey Garen]

> We don't want an m_debuggerCallFrame data member. It doesn't make sense for our caller to point to our callFrame. Also, we only use m_debuggerCallFrame to allocate an activation, which our caller can do for us.

s/caller/lexical parent/

[Mark Lam]

Created attachment 234219 [details]
patch 3: revert to letting DebuggerCallFrame instantiate the JSActivation object.

[Geoffrey Garen]

Comment on attachment 234219 [details]
patch 3: revert to letting DebuggerCallFrame instantiate the JSActivation object.

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

r=me

> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:85
> +    Strong<DebuggerScope> m_scope;

Comment please.

[Mark Lam]

Thanks.  The requested comment about the strong ref for DebuggerCallFrame::m_scope has been added.

Landed in r170680: <http://trac.webkit.org/r170680>.

[Mark Lam]

This patch was rolled out in r172372: <http://trac.webkit.org/r172372> due to <https://webkit.org/b/135656>.  Re-opening to fix the implementation.

[Mark Lam]

Created attachment 237195 [details]
patch 4: with fix for https://webkit.org/b/135656

[Mark Lam]

Created attachment 237196 [details]
Summary diff between patch 3 and 4

[Geoffrey Garen]

Comment on attachment 237195 [details]
patch 4: with fix for https://webkit.org/b/135656

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

r=me, with some fixes below:

> Source/JavaScriptCore/ChangeLog:52
> +           Since the DebuggerScope is a JSObject, it's liveness is determined by its reachability.

Should be "its".

> Source/JavaScriptCore/ChangeLog:53
> +           However, it's "validity" is determined by the life-cycle of its owner DebuggerCallFrame.

Ditto.

> Source/JavaScriptCore/debugger/DebuggerScope.cpp:160
> +bool DebuggerScope::isFunctionScope() const
> +{
> +    // In the current debugger implementation, every function will create an
> +    // activation object. Hence, an activation object implies a function scope.
> +    return m_scope->isActivationObject();
>  }

This seems wrong. If you eval(), the eval code will run in the scope of its containing function, and it will have an activation object as its scope.

> Source/JavaScriptCore/debugger/DebuggerScope.h:81
> +    Iterator begin();
> +    Iterator end();

"Iterator" should be lower case.

[Mark Lam]

(In reply to comment #13)
> > Source/JavaScriptCore/debugger/DebuggerScope.cpp:160
> > +bool DebuggerScope::isFunctionScope() const
> > +{
> > +    // In the current debugger implementation, every function will create an
> > +    // activation object. Hence, an activation object implies a function scope.
> > +    return m_scope->isActivationObject();
> >  }
> 
> This seems wrong. If you eval(), the eval code will run in the scope of its containing function, and it will have an activation object as its scope.

I've fixed the rest and am looking into this.  While testing for this, I'm seeing some difference in behavior in the Inspector with this change vs the baseline and Chrome.  Note: the baseline and Chrome are in agreement.  I'm investigating.

[Mark Lam]

(In reply to comment #14)
> I've fixed the rest and am looking into this.  While testing for this, I'm seeing some difference in behavior in the Inspector with this change vs the baseline and Chrome.  Note: the baseline and Chrome are in agreement.  I'm investigating.

To clarify, the difference I saw is with a WithScope, not an eval.

[Mark Lam]

(In reply to comment #15)
> (In reply to comment #14)
> > I've fixed the rest and am looking into this.  While testing for this, I'm seeing some difference in behavior in the Inspector with this change vs the baseline and Chrome.  Note: the baseline and Chrome are in agreement.  I'm investigating.
> 
> To clarify, the difference I saw is with a WithScope, not an eval.

The issue with WithScopes not correctly showing the values of some of its properties (e.g. __defineGetter__) is due to the fact that we can only override getOwnPropertySlot() but not getPropertySlot().  The values being read incorrectly comes from the with object's prototype chain.  JSObject::getPropertySlot() ends up reading the prototype chain of the DebuggerScope instead of the with object.

Some solutions:
1. Change JSObject::getPropertySlot() to be overridable.
    This is risky and can result in performance penalties.

2. Create a different Structure for each DebuggerScope instance so that it can embedded a different prototype object based on the scope object it is wrapping.
    This uses more memory (even if it's only by the debugger) and can be tricky e.g. do we use the prototype of the WithScope or the with object within the WithScope?  For correctness, we currently need the prototype of the with object.
    This approach also limits the DebuggerScope to only wrapping the tip object, and not any of its prototype chain.

3. Let the DebuggerScope's getOwnPropertySlot() be responsible for the wrapped object's getPropertySlot(), and present them all as properties of the DebuggerScope.
    The downside of this approach is that we lose information about whether properties are in the tip wrapped scope object or in its prototypes.
    Another complication is: do we want to expose all the properties in the JSScope's prototype chain?

Of all these, I think option 3 is the safest and least intrusive into the system.  As for whether to expose the properties in the JSScope's prototype chain or not, the DebuggerScope could always exercise that discretion e.g. only iterate the prototype chain for with objects.  I will give this approach a try and see if any unforeseen issues manifest.

[Geoffrey Garen]

Since the global object can be in scope, and has a prototype, you probably need to iterate prototype properties for it, too.

Whether it's better to show these properties as "own" properties of the debugger scope (your #3), or as prototype properties of the debugger scope (your #2) just depends on how the inspector uses DebuggerScope. If the Inspector doesn't care about "own" vs prototype properties when displaying what's in scope, then I think you're right that it's best just to show these properties as "own" properties.

[Mark Lam]

(In reply to comment #13)
> > Source/JavaScriptCore/debugger/DebuggerScope.cpp:160
> > +bool DebuggerScope::isFunctionScope() const
> > +{
> > +    // In the current debugger implementation, every function will create an
> > +    // activation object. Hence, an activation object implies a function scope.
> > +    return m_scope->isActivationObject();
> >  }
> 
> This seems wrong. If you eval(), the eval code will run in the scope of its containing function, and it will have an activation object as its scope.

For now, it appears that the DebuggerScope does not need to distinguish between functions and evals.  So, I renamed this to isFunctionOrEvalScope() for now.  We can further distinguish the 2 later is a need arises.

Also, solution 3 (having DebuggerScope::getOwnPropertySlot() call JSObject::getPropertySlot() on its wrapped scope) seems to work fine for the WebInspector's needs.

Updated patch coming.

[Mark Lam]

Created attachment 237328 [details]
patch 5: with getPropertySlot() fix for With and Global scopes.

[Geoffrey Garen]

Comment on attachment 237328 [details]
patch 5: with getPropertySlot() fix for With and Global scopes.

r=me

[Mark Lam]

Thanks for the review.  Landed in r173100: <http://trac.webkit.org/r173100>.