Changeset 173100 in webkit for trunk/Source/JavaScriptCore/debugger/DebuggerScope.cpp

Timestamp:

Aug 28, 2014, 5:48:59 PM (11 years ago)

Author:

[email protected]

Message:

DebuggerCallFrame::scope() should return a DebuggerScope.
<​https://webkit.org/b/134420>

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

Rolling back in r170680 with the fix for <​https://webkit.org/b/135656>.

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.

1. Instantiate the debuggerScopeStructure as a member of the JSGlobalObject instead of the VM. This allows JSObject::globalObject() to be able to return the global object for the DebuggerScope.

2. On the DebuggerScope's life-cycle management:

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.
This is pre-existing behavior.

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 the DebuggerScope 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, its liveness is determined by its reachability.
However, its "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. If it does,
those methods will do nothing or returned a failed status.

Fix for <​https://webkit.org/b/135656>:

3. DebuggerScope::getOwnPropertySlot() and DebuggerScope::put() need to set m_thisValue in the returned slot to the wrapped scope object. Previously, it was pointing to the DebuggerScope though the rest of the fields in the returned slot will be set to data pertaining the wrapped scope object.

4. DebuggerScope::getOwnPropertySlot() will invoke getPropertySlot() on its wrapped scope. This is because JSObject::getPropertySlot() cannot be overridden, and when called on a DebuggerScope, will not know to look in the ptototype chain of the DebuggerScope's wrapped scope. Hence, we'll treat all properties in the wrapped scope as own properties in the DebuggerScope. This is fine because the WebInspector does not presently care about where in the prototype chain the scope property comes from.

Note that the DebuggerScope and the JSActivation objects that it wraps do
not have prototypes. They are always jsNull(). This works perfectly with
the above change to use getPropertySlot() instead of getOwnPropertySlot().
To make this an explicit invariant, I also changed DebuggerScope::createStructure()
and JSActivation::createStructure() to not take a prototype argument, and
to always use jsNull() for their prototype value.

• debugger/Debugger.h:
• debugger/DebuggerCallFrame.cpp:

(JSC::DebuggerCallFrame::scope):
(JSC::DebuggerCallFrame::evaluate):
(JSC::DebuggerCallFrame::invalidate):

• debugger/DebuggerCallFrame.h:
• debugger/DebuggerScope.cpp:

(JSC::DebuggerScope::DebuggerScope):
(JSC::DebuggerScope::finishCreation):
(JSC::DebuggerScope::visitChildren):
(JSC::DebuggerScope::className):
(JSC::DebuggerScope::getOwnPropertySlot):
(JSC::DebuggerScope::put):
(JSC::DebuggerScope::deleteProperty):
(JSC::DebuggerScope::getOwnPropertyNames):
(JSC::DebuggerScope::defineOwnProperty):
(JSC::DebuggerScope::next):
(JSC::DebuggerScope::invalidateChain):
(JSC::DebuggerScope::isWithScope):
(JSC::DebuggerScope::isGlobalScope):
(JSC::DebuggerScope::isFunctionOrEvalScope):

• debugger/DebuggerScope.h:

(JSC::DebuggerScope::create):
(JSC::DebuggerScope::createStructure):
(JSC::DebuggerScope::iterator::iterator):
(JSC::DebuggerScope::iterator::get):
(JSC::DebuggerScope::iterator::operator++):
(JSC::DebuggerScope::iterator::operator==):
(JSC::DebuggerScope::iterator::operator!=):
(JSC::DebuggerScope::isValid):
(JSC::DebuggerScope::jsScope):
(JSC::DebuggerScope::begin):
(JSC::DebuggerScope::end):

• inspector/JSJavaScriptCallFrame.cpp:

(Inspector::JSJavaScriptCallFrame::scopeType):
(Inspector::JSJavaScriptCallFrame::scopeChain):

• inspector/JavaScriptCallFrame.h:

(Inspector::JavaScriptCallFrame::scopeChain):

• inspector/ScriptDebugServer.cpp:
• runtime/JSActivation.h:

(JSC::JSActivation::createStructure):

• runtime/JSGlobalObject.cpp:

(JSC::JSGlobalObject::reset):
(JSC::JSGlobalObject::visitChildren):

• runtime/JSGlobalObject.h:

(JSC::JSGlobalObject::debuggerScopeStructure):

• runtime/JSObject.cpp:
• runtime/JSObject.h:

(JSC::JSObject::isWithScope):

• runtime/JSScope.h:
• runtime/PropertySlot.h:

(JSC::PropertySlot::setThisValue):

• runtime/PutPropertySlot.h:

(JSC::PutPropertySlot::setThisValue):

• runtime/VM.cpp:

(JSC::VM::VM):

• runtime/VM.h:

Source/WebCore:

No new tests.

Rolling back in r170680 with the fix for <​https://webkit.org/b/135656>.

• bindings/js/ScriptController.cpp:

(WebCore::ScriptController::attachDebugger):

• We should acquire the JSLock before modifying a JS global object.

File:

• trunk/Source/JavaScriptCore/debugger/DebuggerScope.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/debugger/DebuggerScope.cpp

@@ -29 +29 @@
 #include "JSActivation.h"
 #include "JSCInlines.h"
+#include "JSWithScope.h"
 
 namespace JSC {
@@ -36 +37 @@
 const ClassInfo DebuggerScope::s_info = { "DebuggerScope", &Base::s_info, 0, CREATE_METHOD_TABLE(DebuggerScope) };
 
-DebuggerScope::DebuggerScope(VM& vm)
-    : JSNonFinalObject(vm, vm.debuggerScopeStructure.get())
+DebuggerScope::DebuggerScope(VM& vm, JSScope* scope)
+    : JSNonFinalObject(vm, scope->globalObject()->debuggerScopeStructure())
 {
+    ASSERT(scope);
+    m_scope.set(vm, this, scope);
 }
 
-void DebuggerScope::finishCreation(VM& vm, JSObject* activation)
+void DebuggerScope::finishCreation(VM& vm)
 {
     Base::finishCreation(vm);
-    ASSERT(activation);
-    ASSERT(activation->isActivationObject());
-    m_activation.set(vm, this, jsCast<JSActivation*>(activation));
 }
 
@@ -54 +54 @@
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     JSObject::visitChildren(thisObject, visitor);
-    visitor.append(&thisObject->m_activation);
+    visitor.append(&thisObject->m_scope);
+    visitor.append(&thisObject->m_next);
 }
 
 String DebuggerScope::className(const JSObject* object)
 {
-    const DebuggerScope* thisObject = jsCast<const DebuggerScope*>(object);
-    return thisObject->m_activation->methodTable()->className(thisObject->m_activation.get());
+    const DebuggerScope* scope = jsCast<const DebuggerScope*>(object);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return String();
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    return thisObject->methodTable()->className(thisObject);
 }
 
 bool DebuggerScope::getOwnPropertySlot(JSObject* object, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
 {
-    DebuggerScope* thisObject = jsCast<DebuggerScope*>(object);
-    return thisObject->m_activation->methodTable()->getOwnPropertySlot(thisObject->m_activation.get(), exec, propertyName, slot);
+    DebuggerScope* scope = jsCast<DebuggerScope*>(object);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return false;
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    slot.setThisValue(JSValue(thisObject));
+
+    // By default, JSObject::getPropertySlot() will look in the DebuggerScope's prototype
+    // chain and not the wrapped scope, and JSObject::getPropertySlot() cannot be overridden
+    // to behave differently for the DebuggerScope.
+    //
+    // Instead, we'll treat all properties in the wrapped scope and its prototype chain as
+    // the own properties of the DebuggerScope. This is fine because the WebInspector
+    // does not presently need to distinguish between what's owned at each level in the
+    // prototype chain. Hence, we'll invoke getPropertySlot() on the wrapped scope here
+    // instead of getOwnPropertySlot().
+    return thisObject->getPropertySlot(exec, propertyName, slot);
 }
 
 void DebuggerScope::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
 {
-    DebuggerScope* thisObject = jsCast<DebuggerScope*>(cell);
-    thisObject->m_activation->methodTable()->put(thisObject->m_activation.get(), exec, propertyName, value, slot);
+    DebuggerScope* scope = jsCast<DebuggerScope*>(cell);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return;
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    slot.setThisValue(JSValue(thisObject));
+    thisObject->methodTable()->put(thisObject, exec, propertyName, value, slot);
 }
 
 bool DebuggerScope::deleteProperty(JSCell* cell, ExecState* exec, PropertyName propertyName)
 {
-    DebuggerScope* thisObject = jsCast<DebuggerScope*>(cell);
-    return thisObject->m_activation->methodTable()->deleteProperty(thisObject->m_activation.get(), exec, propertyName);
+    DebuggerScope* scope = jsCast<DebuggerScope*>(cell);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return false;
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    return thisObject->methodTable()->deleteProperty(thisObject, exec, propertyName);
 }
 
 void DebuggerScope::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode)
 {
-    DebuggerScope* thisObject = jsCast<DebuggerScope*>(object);
-    thisObject->m_activation->methodTable()->getPropertyNames(thisObject->m_activation.get(), exec, propertyNames, mode);
+    DebuggerScope* scope = jsCast<DebuggerScope*>(object);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return;
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    thisObject->methodTable()->getPropertyNames(thisObject, exec, propertyNames, mode);
 }
 
 bool DebuggerScope::defineOwnProperty(JSObject* object, ExecState* exec, PropertyName propertyName, const PropertyDescriptor& descriptor, bool shouldThrow)
 {
-    DebuggerScope* thisObject = jsCast<DebuggerScope*>(object);
-    return thisObject->m_activation->methodTable()->defineOwnProperty(thisObject->m_activation.get(), exec, propertyName, descriptor, shouldThrow);
+    DebuggerScope* scope = jsCast<DebuggerScope*>(object);
+    ASSERT(scope->isValid());
+    if (!scope->isValid())
+        return false;
+    JSObject* thisObject = JSScope::objectAtScope(scope->jsScope());
+    return thisObject->methodTable()->defineOwnProperty(thisObject, exec, propertyName, descriptor, shouldThrow);
+}
+
+DebuggerScope* DebuggerScope::next()
+{
+    ASSERT(isValid());
+    if (!m_next && m_scope->next()) {
+        VM& vm = *m_scope->vm();
+        DebuggerScope* nextScope = create(vm, m_scope->next());
+        m_next.set(vm, this, nextScope);
+    }
+    return m_next.get();
+}
+
+void DebuggerScope::invalidateChain()
+{
+    DebuggerScope* scope = this;
+    while (scope) {
+        ASSERT(scope->isValid());
+        DebuggerScope* nextScope = scope->m_next.get();
+        scope->m_next.clear();
+        scope->m_scope.clear();
+        scope = nextScope;
+    }
+}
+
+bool DebuggerScope::isWithScope() const
+{
+    return m_scope->isWithScope();
+}
+
+bool DebuggerScope::isGlobalScope() const
+{
+    return m_scope->isGlobalObject();
+}
+
+bool DebuggerScope::isFunctionOrEvalScope() const
+{
+    // In the current debugger implementation, every function or eval will create an
+    // activation object. Hence, an activation object implies a function or eval scope.
+    return m_scope->isActivationObject();
 }